RE: Memory leaks in windres

[Borut Razem]

Hello,


> This one isn't such a good idea if "count" can be large.  Allocating
> large stack frames can result in errors on machines with limited
> stack size.

I agree with the decision. I was too fast with the proposition :-(


> I'm not willing to apply the above two patches either.  Some code
> somewhere may rely on the presence of u.n.name pointing to a zero
> word when u.n.length is zero.  I couldn't find any such code with
> a quick scan over the relevant files, but this isn't an area of
> binutils I'm that familiar with.  You'll need to convince me, or
> wait for another maintainer more familiar with the code to commit
> it.

If you don't agree with the proposed solution, here is an other one:
the resource id should be deallocated before it is overwritten.
OLD CODE:
  n = define_control (0, id, x, y, 0, 0, CTL_STATIC, style, exstyle);
  n->text = iid;
NEW CODE:
  n = define_control (0, id, x, y, 0, 0, CTL_STATIC, style, exstyle);
  delete_res_id(&n->text);
  n->text = iid;

Here is the patch:

cvs -z9 diff windres.c (in directory D:\cvs\src\binutils)
Index: windres.c
===================================================================
RCS file: /cvs/src/src/binutils/windres.c,v
retrieving revision 1.13
diff -r1.13 windres.c
345a346,354
> /* Deallocate the resource ID.  */
> 
> void
> delete_res_id (struct res_id *id)
> {
>   if (id != NULL && id->named && id->u.n.name != NULL)
>     free(id->u.n.name);
> }
> 

cvs -z9 diff windres.h (in directory D:\cvs\src\binutils)
Index: windres.h
===================================================================
RCS file: /cvs/src/src/binutils/windres.h,v
retrieving revision 1.7
diff -r1.7 windres.h
780a781
> extern void delete_res_id PARAMS ((struct res_id *id));

cvs -z9 diff resrc.c (in directory D:\cvs\src\binutils)
Index: resrc.c
===================================================================
RCS file: /cvs/src/src/binutils/resrc.c,v
retrieving revision 1.19
diff -r1.19 resrc.c
869a870
>   delete_res_id(&n->text);

I will spend some more time to investigate my first solution, because
I still believe that is more elegant then this one (yes, I agree that
is also more risky). Maybe I will convince you with more arguments...

There is an other problem with the heap handling: the struct res_directory
tree, created by read_rc_file(), is never deallocated. This is fine for
windres utility, which just reads the resource file and generate an
output from it, but it is a problem if the resource is red on startup
of an interactive application, which continue to run, but the res_directory
tree is not needed any more.

I already wrote the code which deallocates the res_diractory tree, but I
don't know if you are willing to integrate it. If you are interested on
it, I will send it.


> rcparse.y was broken.  Parser lookahead meant that at least `id' and
> one other token was read before the parser could decide between the
> rules for `input'.  This delays `newcmd' action (string discarding)
> after the point where the lexer has read strings for the current
> command.

With your fixes there is probably no need to call rcparse_discard_strings()
after yyparse() in resrc.c any more, but it does not make any harm anyway,
so we can leave it there.


> Ask address@hidden

Thank you for the information.


Borut Razem