Re: patch

[Martin Pool <martinp mincom com>]

The current implementation in gmem.c never frees memory when
ENABLE_MEM_CHECK is set.  I thought this was a bug, but now I see it may
be intentional.  Can somebody please tell me which it is?

Tim Janik wrote:

> uhm, i didn't really think about matters the last time i wrote to you,
> you can't

> > ***************
> > *** 405,415 ****
> >         g_warning ("freeing previously freed memory\n");
> >         *t += 1;
> >         mem = t;
> >
> > -       memset ((guchar*) mem + 8, 0, size);
> > - #else /* ENABLE_MEM_CHECK */
> >         free (mem);
> > - #endif /* ENABLE_MEM_CHECK */
> >       }
> >   }
> 
> free the memory here and later
> 
> >
> > --- 409,419 ----
> >         g_warning ("freeing previously freed memory\n");
> >         *t += 1;
> >         mem = t;
> > +
> > +       memset ((guchar*) mem + 2 * SIZEOF_LONG, GARBAGE_FILL, size);
> > + #endif /* ENABLE_MEM_CHECK */
> >
> >         free (mem);
> >       }
> >   }
> 
> access it during the garbage fill, that allmost certainly will screw
> the malloc's internal pointrers and lead to segfaults on most systems.

I think you may have misread the patch.  The new version of the
function, shown in the chunk (409,419) first clobbers the memory, and
then frees it.  It definitely doesn't access the memory after it's been
freed.  MEM_CHECK does leave behind the two debugging fields unaltered,
but it doesn't _write_ to them after the memory's been freed.

If the malloc implementation writes over the first few bytes of a freed
block -- which actually seems pretty likely -- then the debugging fields
won't work properly, though they shouldn't crash the allocator.  This
behaviour was always in gmem.

> what you can do to improve the current behaviour would be, partly
> freeing the memory and clobber it before the free, e.g. (editing your diff)

I do clobber it before the free() and not afterwards.  Admittedly I
clobber it with a nonzero value, but aside from that it's no different
to the existing implementation.

>  ***************
> > *** 405,415 ****
> >         g_warning ("freeing previously freed memory\n");
> >         *t += 1;
> >         mem = t;
> >
> > -       memset ((guchar*) mem + 8, 0, size);
> > +       memset ((guchar*) mem + 2 * SIZEOF_LONG, GARBAGE_FILL, size);
> > +       realloc ((guchar*) mem, 2 * SIZEOF_LONG);
> 
> i.e. free only the cluttered area

Isn't it possible that some allocators will move blocks when they
shrink? 
<http://www.gnu.org/manual/glibc-2.0.6/html_node/libc_25.html#SEC25>
seems to suggest so.

There's no way to safely shrink the block down to just the debugging
fields.

> and later on, the code should
> 
> >
> > --- 409,419 ----
> > -       g_warning ("freeing previously freed memory\n");
> > +       g_warning ("will not free previously freed (%d) memory\n", *t);
> >         *t += 1;
> >         mem = t;
> >
> > -       free (mem);
> 
> to avoid double-frees that will give trouble anyways.

Sure, that sounds good.

> but since this is changing semantics of ENABLE_MEM_CHECK in major
> ways, we should discuss this on gtk-devel-list@redhat.com first
> (feel free to Cc: to that list on replies).

Arguably the right place to do this stuff is in the malloc library, and
we should just suggest that people use a debugging-malloc or
ElectricFence to test their code.

-- 
 /\\\  Mincom | Martin Pool          | martinp@mincom.com
// \\\        | Software Engineer    | Phone: +61 7 3303-3333
\\ ///        | Mincom Pty. Ltd.     | 
 \///         | Teneriffe, Brisbane  | Speaking for myself only