On Sat, Dec 08, 2001 at 09:48:13PM -0800, Guy Harris wrote:
> It appears that "gen_arth()" frees "a1->regno" but not "a0->regno" - it
> just overwrites "a0->regno".
>
> I'll have to see whether the actual *code* overwrites it, so that the
> old value in "a0->regno" can be freed.
It doesn't - it's overwriting the "register" (scratch memory location)
referred to by "a1->regno".
If a given "struct arth" is used more than once, so that you can't
overwrite its "register" until the *last* load of it is generated, it's
not safe to free up "a0->regno".
On the other hand, it's not clear that it's safe to free up "a1->regno",
either, unless there's some guarantee that if a given "struct arth" is
passed as the third argument to "gen_arth()", it's never passed to
anything else to use.
"struct arth"s are generated by:
gen_load()
gen_loadlen()
gen_loadi()
gen_neg()
gen_arth()
Inside the parser, a given "$n" that refers (points) to a "struct arth"
is never passed more than once to any function, so the parser can't
(unless I'm completely missing something) generate more than one
reference to a "struct arth".
"gen_inbound()" is the only routine in "gencode.c" that calls any of the
"gen_load*" routines; it also doesn't use the results of any of those
calls more than once. Nothing in "gencode.c" uses "gen_neg()" or
"gen_arth()".
The routines that take "struct arth *"s as arguments don't store that
argument anywhere.
So it *appears* that a "struct arth" won't be used more than once, and
therefore that it's safe to add "free_reg(a0->regno);" before the
"free_reg(a1->regno);" in "gen_arth()" ("gen_relation()" frees the
registers for both of its "struct arth *" arguments in that fashion).
This would break, of course, if somebody added new code that *did* save
a "struct arth *" and used it more than once; if they did that, we'd
probably have to treat the elements of the "regused[]" array as
counters, not booleans, with "alloc_reg()" still setting it to 1,
"free_reg()" decrementing it (aborting if it's 0, as that means you're
freeing a free register), and another routine bumping the use count, for
use by routines that use a "struct arth *" more than once.
A patch to change "gen_arth()" is attached; Ken, could you try applying
that patch (after removing the "reset_regs()" stuff), and see if it
1) plugs the leak caused by repeated compilation;
2) plugs any leaks discovered by compilation of a sufficiently
complex expression (if you've found any of those);
3) breaks anything you notice?
Index: gencode.c
===================================================================
RCS file: /tcpdump/master/libpcap/gencode.c,v
retrieving revision 1.160
diff -c -r1.160 gencode.c
*** gencode.c 2001/11/30 07:25:48 1.160
--- gencode.c 2001/12/09 06:01:04
***************
*** 3589,3594 ****
--- 3589,3595 ----
sappend(a1->s, s0);
sappend(a0->s, a1->s);
+ free_reg(a0->regno);
free_reg(a1->regno);
s0 = new_stmt(BPF_ST);
