[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: gawk: other double free(_wstr)
From: |
Karel Zak |
Subject: |
Re: gawk: other double free(_wstr) |
Date: |
Mon, 15 Jan 2007 23:01:56 +0100 |
User-agent: |
Mutt/1.5.11 |
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>
Re: gawk: other double free(_wstr), Aharon Robbins, 2007/01/13
Re: gawk: other double free(_wstr), Aharon Robbins, 2007/01/16
Re: gawk: other double free(_wstr), Aharon Robbins, 2007/01/18
Re: gawk: other double free(_wstr), Aharon Robbins, 2007/01/27