Re: Memory leaks found by DMALLOC?

As it turns out, this was found (and fixed) by an engineer here a couple
months back, I just didn't remember it.  I really need to get a new
relase out.  Our fix is to correctly pass on the error from
cgiwrap_writevf with nerr_pass in cgiwrap_writef.

I hadn't noticed that all the calls to cgiwrap_writef don't check the
return code, so I'll have to fix that.

I really need to get a release out, I've been waiting on a few things
people have been working on before doing it, so I'll push on them.

Brandon

On 10/27/08 raphael.huck uttered the following other thing:
> I've found the problem a long time ago, and forgot to post it.
> 
> I just stumbled on my old message, so I figured I'd post a followup :)
> 
> In fact the problem comes from cgiwrap_writef in cgi/cgiwrap.c:
> 
> NEOERR *cgiwrap_writef (const char *fmt, ...)
> {
>   va_list ap;
> 
>   va_start (ap, fmt);
>   cgiwrap_writevf (fmt, ap);
>   va_end (ap);
>   return STATUS_OK;
> }
> 
> cgiwrap_writevf returns a NEOERR * which isn't checked in
> cgiwrap_writef. If cgiwrap_writevf returns STATUS_OK, everything is
> fine, if not, the allocated structure is never freed. That is the
> memory leak I was talking about in my previous mail.
> 
> If we have a look at the code of cgiwrap_writevf, we can see:
> 
>     r = GlobalWrapper.writef_cb (GlobalWrapper.data, fmt, ap);
>     if (r)
>       return nerr_raise_errno (NERR_IO, "writef_cb returned %d", r);
> 
> Thus, if the writef callback you have given to cgiwrap_init_emu
> doesn't return 0, an error structure will be allocated and returned,
> but never handled.
> 
> The quick solution is to simply have the writef callback always return
> 0: cgiwrap_writevf will then never allocate an error structure, so no
> memory leak.
> 
> So instead of doing this (which triggers the memory leak):
> 
> int writef_cb(void *ptr, const char *format, va_list ap)
> {
> 	UNUSED(ptr);
> 
> 	return FCGI_vprintf(format, ap);
> }
> 
> do this:
> 
> int writef_cb(void *ptr, const char *format, va_list ap)
> {
> 	UNUSED(ptr);
> 
> 	FCGI_vprintf(format, ap);
> 
> 	return 0;
> }
> 
> 
> The cleaner solution would be to modify cgiwrap_writef in order to
> handle the error returned by cgiwrap_writevf.
> 
> But then each call to cgiwrap_writef should have to check the return
> value, and deal with the potential error structure, or there would be
> even more memory leaks.
> 
> In cgi/cgi.c there are several calls to cgiwrap_writef which don't
> check the return value...
> 
> So I don't know if the clean solution is a good idea.
> 
> What do you think Brandon?
> 
> Thanks in advance for your advice.
> 
> --Raphaël
> 
> >
> > > I'll try to find more information about the leaked memory.
> > > 
> > > I forgot to mention that when I comment out the following lines:
> > > 
> > >   	err = cgi_display(cgi, "test.cst");
> > >   	nerr_ignore(&err);
> > > 
> > > there are no longer memory leaks, so it seems to come from
> cgi_display.
> > 
> > I've found more information.
> > 
> > I'm using the following wrappers for FastCGI:
> > 
> > /* FCGI_fread wrapper for ClearSilver CGI */
> > int read_cb(void *ptr, char *data, int size)
> > {
> > 	UNUSED(ptr);
> > 
> > 	return FCGI_fread(data, sizeof(char), size, FCGI_stdin);
> > }
> > 
> > /* FCGI_vprintf wrapper for ClearSilver CGI */
> > int writef_cb(void *ptr, const char *format, va_list ap)
> > {
> > 	UNUSED(ptr);
> > 
> > 	return FCGI_vprintf(format, ap);
> > }
> > 
> > /* FCGI_fwrite wrapper for ClearSilver CGI */
> > int write_cb(void *ptr, const char *data, int size)
> > {
> > 	UNUSED(ptr);
> > 
> > 	return FCGI_fwrite((void *) data, sizeof(char), size, FCGI_stdout);
> > }
> > 
> > and then I call:
> > 
> > cgiwrap_init_emu(NULL, &read_cb, &writef_cb, &write_cb, NULL, NULL,
> NULL);
> > 
> > The error I get is:
> > 
> > in cgiwrap.c:0192:cgiwrap_writevf - writef_cb returned 27: [134] 
> > Transport endpoint is not connected
> > 
> > 
> > Then I had a look at cgi/fcgi_hello.c and saw this line (by the way, I 
> > think 'environ' should be replaced by 'envp' in this line):
> > 
> > cgiwrap_init_std(argc, argv, environ);
> > 
> > and added this line to my code before the call to cgiwrap_init_emu, but 
> > I still have the same error.
> > 
> > Then I've tried using the following from cgi/fcgi_hello.c instead:
> > 
> > int cs_printf(void *ctx, const char *s, va_list args)
> > {
> > 	UNUSED(ctx);
> > 
> > 	return printf(s, args);
> > }
> > 
> > int cs_write(void *ctx, const char *s, int n)
> > {
> > 	UNUSED(ctx);
> > 
> > 	return fwrite(s, n, 1, FCGI_stdout);
> > }
> > 
> > and calling this:
> > 
> > cgiwrap_init_emu(NULL, NULL, cs_printf, cs_write, NULL, NULL, NULL);
> > 
> > but it makes things worse as this gives me 2 errors then:
> > 
> > in cgiwrap.c:0192:cgiwrap_writevf - writef_cb returned 27: [134] 
> > Transport endpoint is not connected
> > 
> > in cgiwrap.c:0210:cgiwrap_write - write_cb returned 1<2488: [134] 
> > Transport endpoint is not connected
> > 
> > 
> > --Raphael
> > 
> > >> On 08/02/07 Raphaël HUCK uttered the following other thing:
> > >>> I've made a simple example to try and understand where these
> leaks come 
> > >>> from:
> > >>>
> > >>> void fcgi_begin(void)
> > >>> {
> > >>> 	NEOERR *err;
> > >>>
> > >>> 	err = cgi_init(&cgi, NULL);
> > >>> 	nerr_ignore(&err);
> > >>> }
> > >>>
> > >>> void fcgi_end(void)
> > >>> {
> > >>> 	cgi_destroy(&cgi);
> > >>> }
> > >>>
> > >>> void fcgi_dispatch(void)
> > >>> {
> > >>> 	NEOERR *err;
> > >>>
> > >>> 	err = hdf_read_file(cgi->hdf, "test.hdf");
> > >>> 	nerr_ignore(&err);
> > >>>
> > >>> 	err = cgi_display(cgi, "test.cst");
> > >>> 	nerr_ignore(&err);
> > >>>
> > >>> 	return;
> > >>> }
> > >>>
> > >>> int main(int argc, char **argv)
> > >>> {
> > >>> 	UNUSED(argc);
> > >>> 	UNUSED(argv);
> > >>>
> > >>> 	while (FCGI_Accept() >= 0)
> > >>> 	{
> > >>> #ifdef DMALLOC
> > >>> 		unsigned long mark;
> > >>> 		/* get the current dmalloc position */
> > >>> 		mark = dmalloc_mark();
> > >>> #endif
> > >>>
> > >>> 		fcgi_begin();
> > >>> 		fcgi_dispatch();
> > >>> 		fcgi_end();
> > >>>
> > >>> #ifdef DMALLOC
> > >>> 		/*
> > >>> 		* log unfreed pointers that have been added to
> > >>> 		* the heap since mark
> > >>> 		*/
> > >>> 		dmalloc_log_changed(mark,
> > >>> 			1 /* log unfreed pointers */,
> > >>> 			0 /* do not log freed pointers */,
> > >>> 			1 /* log each pnt otherwise summary */);
> > >>> #endif
> > >>> 	}
> > >>>
> > >>> 	return 0;
> > >>> }
> > >>>
> > >>> And I get the following results with dmalloc:
> > >>>
> > >>> $ dmalloc_summarize.pl fastcgi < fastcgi.3393
> > >>>       size      count      gross function
> > >>>                    37      10036 total
> > >>>        284         35       9940 ra=0x419748
> > >>>         80          1         80 _err_alloc+64> and ends at
> 0x41974c 
> > >>> <_err_alloc [neo_err.c:59]
> > >>>         16          1         16 check_resize+76> and ends at
> 0x41b8d8 
> > >>> <check_resize [ulist.c:37]
> > >>>
> > >>>
> > >>>
> > >>> --Raphael
> > >>>
> > >>>> Anything that returns a NEOERR is allocating memory on error. 
> You must
> > >>>> check the return value and call one of the methods to free the
> data,
> > >>>> usually one of:
> > >>>>
> > >>>> nerr_log_error
> > >>>> nerr_ignore
> > >>>> nerr_handle
> > >>>>
> > >>>> See util/neo_err.h for details.
> > >>>>
> > >>>> The uListInit one is probably from nerr_init, which allocates a
> very
> > >>>> small amount of memory once.
> > >>>>
> > >>>> Brandon
> > >>>>
> > >>>> On 07/31/07 raphael.huck uttered the following other thing:
> > >>>>> Hi,
> > >>>>>
> > >>>>> Here's what I've found with DMALLOC:
> > >>>>>
> > >>>>> $ dmalloc_summarize.pl fastcgi < fastcgi.10983 
> > >>>>>      size      count      gross function
> > >>>>>                 1494     401044 total
> > >>>>>       284       1364     387376 ra=0x436818
> > >>>>>      1176          9      10584 _err_alloc+64> and ends at
> 0x43681c 
> > >>>>> <_err_alloc [neo_err.c:59] 
> > >>>>>       240          2        480 hcreate_r 
> > >>>>>        16          4         64 check_resize+76> and ends at 
> > >>>>> 0x4389a8 <check_resize [ulist.c:37] 
> > >>>>>        20          3         60 uListInit+68> and ends at
> 0x438a5c 
> > >>>>> <uListInit [ulist.c:61] 
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> The part which leaks the most memory is around 0x436818, which is:
> > >>>>>
> > >>>>> $ mips-linux-objdump -S fastcgi | grep -A 21 -B 25 436818
> > >>>>> static NEOERR *_err_alloc(void)
> > >>>>> {
> > >>>>>  4367d0:       3c1c0fbd        lui     gp,0xfbd
> > >>>>>  4367d4:       279c2c10        addiu   gp,gp,11280
> > >>>>>  4367d8:       0399e021        addu    gp,gp,t9
> > >>>>>  4367dc:       27bdffe0        addiu   sp,sp,-32
> > >>>>>  4367e0:       afbf0018        sw      ra,24(sp)
> > >>>>>  4367e4:       afbc0010        sw      gp,16(sp)
> > >>>>>  NEOERR *err;
> > >>>>>
> > >>>>>  if (!UseFreeList || FreeList == NULL)
> > >>>>>  4367e8:       8f828018        lw      v0,-32744(gp)
> > >>>>>  4367ec:       8f878018        lw      a3,-32744(gp)
> > >>>>>  {
> > >>>>>    err = (NEOERR *)calloc (1, sizeof (NEOERR));
> > >>>>>  4367f0:       8f9988dc        lw      t9,-30500(gp)
> > >>>>>  4367f4:       8c4325d0        lw      v1,9680(v0)
> > >>>>>  4367f8:       2405011c        li      a1,284
> > >>>>>  4367fc:       10600004        beqz    v1,436810 <_err_alloc+0x40>
> > >>>>>  436800:       24040001        li      a0,1
> > >>>>>  436804:       8ce625cc        lw      a2,9676(a3)
> > >>>>>  436808:       14c0000d        bnez    a2,436840 <_err_alloc+0x70>
> > >>>>>  43680c:       8fbf0018        lw      ra,24(sp)
> > >>>>>  436810:       0320f809        jalr    t9
> > >>>>>  436814:       00000000        nop
> > >>>>>  436818:       8fbc0010        lw      gp,16(sp)
> > >>>>>    if (err == NULL)
> > >>>>>    {
> > >>>>>      ne_warn ("INTERNAL ERROR: Unable to allocate memory for 
> > >>>>> NEOERR");
> > >>>>>      return INTERNAL_ERR;
> > >>>>>    }
> > >>>>>    return err;
> > >>>>>  43681c:       00402821        move    a1,v0
> > >>>>>  436820:       8f848024        lw      a0,-32732(gp)
> > >>>>>  436824:       8f998aa4        lw      t9,-30044(gp)
> > >>>>>  436828:       1040000f        beqz    v0,436868 <_err_alloc+0x98>
> > >>>>>  43682c:       24844130        addiu   a0,a0,16688
> > >>>>>  }
> > >>>>>  else
> > >>>>>  {
> > >>>>>    err = FreeList;
> > >>>>>    FreeList = FreeList->next;
> > >>>>>  }
> > >>>>>  err->flags |= NE_IN_USE;
> > >>>>>  err->next = NULL;
> > >>>>>  return err;
> > >>>>> }
> > >>>>>
> > >>>>> These seem to be memory leaks. Can anyone confirm?
> > >>>>>
> > >>>>> --Raphaël HUCK
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> Yahoo! Groups Links
> > >>>>>
> > >>>>>
> > >>>>>
> > > 
> > > 
> > > 
> > >  
> > > Yahoo! Groups Links
> > > 
> > > 
> > >
> >
> 
> 
> 
> ------------------------------------
> 
> Yahoo! Groups Links
> 
> 
> 
-- 
 "The only thing nude playing cards are good for is playing solitaire."
        -- Harry Anderson, PI 1/7/98
                                           http://www.fiction.net/blong/