Re: gawk: other double free(_wstr)

[Karel Zak]

On Mon, Jan 15, 2007 at 09:50:29AM -0500, Andrew J. Schorr wrote:
> On Mon, Jan 15, 2007 at 01:03:07PM +0100, Karel Zak wrote:
> > On Sat, Jan 13, 2007 at 03:17:20PM -0500, Andrew J. Schorr wrote:
> > > On Sat, Jan 13, 2007 at 08:52:30PM +0200, Aharon Robbins wrote:
> > > > I think I'm going to undo the part of free_wstr that only zeros the
> > > > fields if the flag is set.
> > > 
> > > Hmmm, this code that you plan to restore (zeroing wstptr even if the
> > > WSTRCUR flag is not set) seems to conflict with a statement you made
> > > back in July:
> > 
> >  I agree with Aharon. It's more robust. There are places in code which
> >  expect this behavior.
> 
> Perhaps I'm misunderstanding the logic, but I believe that changing free_wstr
> back to the old behavior was completely orthogonal to fixing the bug that you
> discovered.  The problem, in the bug that you found, was that the WSTRCUR flag

 It's not way how to fix the problem. The fix is add free_wstr() call
 to the rebuild_record() (already done CVS by Aharon).

> was in fact set, but free_wstr was being called after the NODE had been copied
> to another NODE that had the old values -- so the pointer was being freed
> inside unref's call to free_wstr, but the copied NODE still had the freed

 Yes. You're right.

> pointer with the WSTRCUR flag set.  The fix that Arnold applied was to
> call free_wstr before copying the fields_arr[i] node into tmp.  This fixes
> the bug regardless of the change to free_wstr.
> 
> Just to be sure, I reran your test case with both versions of free_wstr,
> and valgrind reports the same double free error in both cases.  So I claim
> that the change to free_wstr has nothing to do with fixing the bug.

 Yes. Move back to previous version of free_wstr() is a paranoid option
 and not any bug fix :-)

 For example, see node.c: mk_number()

     getnode(r);     <-- allocate new node
     ...
     free_wstr(r);   <-- __zeroize__  MB stuff in the node

 (see also gawk code before free_wstr() implementation. There was more
 places where MB stuff was zeroed although WSTRCUR wasn't set.)

> So are you sure that there are actually places in the code that depend on this
> behavior (free_wstr zeroing the wstptr fields even if WSTRCUR is not set)?

 We don't talk about wstptr __deallocation__. There is still in free_wstr()

    if ((n->flags & WSTRCUR) != 0)
        free(n->wstptr);

 .. so the wide string is deallocated only if WSTRCUR is set.

 We talk about __zeroing__  of wstptr, wstlen and flags. If you call
 the free_wstr() which always zeroing MB stuff you can by always sure
 that the NODE is in expected state. (Yes, it's paranoid.)

 I don't see any real problem with free_wstr(r) which also zeroing
 independently on WSTRCUR.

    Karel

-- 
 Karel Zak  <address@hidden>