[samba4] thread safety in charcnv & iconv

[Michael B Allen]

On Fri, 2 Sep 2005 11:05:21 -0400
Karl Melcher <kmelcher at gmail.com> wrote:

> I have been experiencing a segfault in a test client that uses RPC pipes via 
> samba4. This client opens multiple threads to different hosts using 
> dcerpc_pipe_connect(). The problem occurs during a system call to iconv() in 
> lib/charset/iconv.c inside sys_iconv(). According to the man pages for 
> iconv, it changes the state of the passed in descriptor during the 
> conversion. 
> 
> The table of descriptors used (conv_handles[][]) is initialized on demand 
> and is stored in a static array shared between threads. If two or more 
> client threads are using the same descriptor that ultimately is used in a 
> call to iconv, upon exit from iconv the return code is 0 (OK), but the 
> outbytesleft argument is a very large (random) number. A later segfault 
> occurs due to large string size. 
> 
> I don't yet have a patch for this, but think that sharing the descriptors 
> may be a bad idea. A synchronization object may help, but if the state is in 
> the descriptor, then they should never be shared between threads. 

Once initialized I think the state in the iconv descriptor is really
only used to track when conversion stops in the middle of a multibyte
sequence (like mbstate_t w/ mbs functions). So unless your iconv calls
are being used to iteratively convert data where a chunk may not land
on a character boundry, then I think it is ok to share the descriptor
between threads. Of course you cannot operate on the same descriptor
*concurrently* so you need to at least protect the conversion with a
lock. There shouldn't be much performance impact with that as character
conversion is a largely cpu bound process.

But yes, it would be purer to associate the conversion context with the
thread specific object. For example you could decide that each thread
must create it's own pipe context and then either protect everthing from
there down with a lock (associated with the object it's protecting) or
give the pipe context it's own object eliminating the need to protect
it with a lock at all.

Also, note that you do not need multiple locks of course. You could
create one lock system wide (e.g. a system V semaphore) and use it to
protect *everything*. You just can't block while it's locked. Remember
the Big Kernel Lock (BKL) in Linux?

Mike