multiple frees in awk, sometimes leading to crash

[Micah Cowan]

This bug is described to some detail at
https://bugs.launchpad.net/ubuntu/+source/gawk/+bug/58256

The problem can be seen in an example like the following (doesn't crash
on my system, but valgrind will complain):

  printf '\n\na\nb\n' | valgrind gawk '{length($1)}'

(A "print" may be inserted before the length() call, if desired.) See
the link above for a version that crashes, at least on some systems.

The problem is that do_length() invokes str2wstr() on its argument. In
the case of the initial blank line, above, this results in
Null_field->wstptr being given a pointer, and Null_field->flags |= WSTRCUR.

On the next line gawk encounters, the $1 reference, since it exists this
time, will cause gawk to grow fields_arr[], copying the value of
*Null_field to the new element. After this copy, set_field() copies the
new text ("a") into the element, and resets the flags (clearing
WSTRCUR). str2wstr(), seeing a pointer value in n->wstptr, but no
WSTRCUR flag, will free(n->wstptr).

On the final line, reset_fields() will ensure that the value of
*Null_field is copied over the previous value of fields_arr[1] (still
with the same value for n->wstptr as before). When set_field() is called
again to set it with the "b" text, the flags will again be cleared, and
str2wstr() will end up freeing the very same wstptr value (double free).

I can think of a couple of ways to solve this. One would be to have
r_get_lhs() return a new node with a copy of the value from Null_field,
rather than &Null_field itself, when get_field() returns &Null_field
(I'm guessing that this shouldn't be done in get_field() itself, as
sometimes it may be used only for reading?). This will prevent the
actual value of Null_field from being modified by str2wstr().

That has the disadvantage that every time length() is called on a
nonexistent field, we are invoking a decode operation (and allocating
associated space) to create a zero-length wide-string, just to get its
length, and then discard it. A solution for this is that we can have
do_length() detect zero-length strings, and automatically return zero
for them, short-circuiting the useless wide-string conversion (and,
indeed, we should do this anyway). Since this would prevent the
conversion in all cases for Null_field, it would never get a value
written to its wstptr, so problem solved...

Except that (1) this doesn't self-document enough to help people
understand the problem should they decide to alter code in that area
again, and (2) I haven't verified this, but I believe do_match() may
have the same issue (when it sets rlength), in which case we'll have
logic duplication: two fragile places where the code could be broken again.

I suppose another, somewhat kludgy fix, would be for str2wstr() itself
to detect zero-length strings, in which case it sets wstlen to 0, and
wstptr to NULL. In this case, it may not really even matter whether
WSTRCUR gets set; and free(NULL) is guaranteed well-defined. This also
has the advantage of automatically resolving the
useless-allocation-for-zero-length-strings inefficiency.

I'm not sure I completely like any of these fixes. What do you think?

-- 
Micah J. Cowan
Programmer, musician, typesetting enthusiast, gamer...
http://micah.cowan.name/