Re: [cld patch 1/1] fix crash in cldc_close due to array reallocations

[Jeff Garzik <jeff@garzik.org>]

On 05/15/2010 03:32 PM, Pete Zaitcev wrote:
> A long-running tabled is virtually guaranteed to crash when it's shut
> down, with the traceback like this:
>
> Program terminated with signal 11, Segmentation fault.
> #0  cldc_close (fh=0x10a71c8, copts=0x7fff2fdef780) at cldc.c:1049
> 1049            if (sess->expired) {
> (gdb) where
> #0  cldc_close (fh=0x10a71c8, copts=0x7fff2fdef780) at cldc.c:1049
> #1  0x00007f8a878ed7c7 in ncld_close (fh=0x10a7790) at cldc.c:2215
> #2  0x00007f8a878ed918 in ncld_sess_close (nsess=0x10a6f10) at cldc.c:2247
> #3  0x0000000000406252 in cld_end () at cldu.c:701
> #4  0x000000000040d6c4 in main (argc=<value optimized out>,
>      argv=<value optimized out>) at server.c:1980
> (gdb) print *fh
> $3 = {fh = 140232956496032, sess = 0x0, valid = 176}
>
> Obviously the boolean value should not be 176.
>
> The reason this happens is array reallocation. The API for libcldc
> (used internally by ncld) returns a pointer to struct cldc_fh that
> the user is expected to save. However, this structure is placed
> inside a GArray. As array expands, it can be relocated in memory
> and the file handle pointer is left hanging. Tabled is vulnerable
> to this because 1) it keeps an open handle for the CLD file that
> it locked and 2) it periodically opens and closes CLD files for
> chunkservers, causing array to grow.
>
> We observed that the indexes into the array are not the same
> as file handles. Therefore, we never access elements by index
> and thus it is not necessary to use an array structure.
>
> This patch replaces array with a list, so that pointers to handle
> structures can be stable.
>
> Also, the patch frees the handles when they are closed, thus
> ending the endless growth of the file handle table. This is
> done in a compatible way, so handle is available to callers
> of cldc_close (ncld does not need it, but it may be useful).
>
> Although we change globally visible structures, we only replace
> one ostensibly opaque pointer type (GArray) with another one
> (GList), so applications need not be rebuilt.
>
> Signed-off-by: Pete Zaitcev<zaitcev@redhat.com>

applied