Re: Memory leaks found by DMALLOC?

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
> > 
> > 
> >
>