gEDA-dev: Stack corruption PCB bug in hid_parse_actions????

[Peter Clifton]

Replying to my own email again.. I took a printout of
src/hid/common/actions.c home, and had a closer look at the code, so
have more to add to this bug report:

On Wed, 2006-09-06 at 18:04 +0100, Peter Clifton wrote:
> On Wed, 2006-09-06 at 17:48 +0100, Peter Clifton wrote:
> 
> > Looks like a nasty stack corruption, as whilst argv[0] moves its
> > location by 2 bytes (and IIRC, its always this ammount), argv[0] still
> > evaluates to the same string.. as if a portion of the stack has been
> > shifted. (Cant have shifted all of it, as the function return appear
> > be ok) (Evaluation of argv[0] as string not shown in output below)

I think I've got the problem spotted...

> int
> hid_parse_actions (const char *rstr,
> 		   int (*function) (const char *, int, char **))
> {
>   static char **list = 0;
>   static int max = 0;
>   int num = 0;
>   static char *str = NULL;
>   char *sp, *aname, *sp2;
>   int maybe_empty = 0;

The three static variables above aren't recursion save..

>   if (function == NULL)
>     function = hid_actionv;
> 
>   if (str != NULL)
>     {
>       free (str);
>       str = NULL;
>     }

And when ActionExecuteFile calls hid_parse_actions, that second entry
now free's the argument string in use by the original entry (leaving
ActionExecuteFile printing log messages based upon free'd memory).
Similarly, the argument list is over-written, since num = 0 at the start of
the function. (And list is static)

[code snip]

> 	  if (num >= max)
> 	    {
> 	      max += 10;
> 	      if (list)
> 		list = (char **) realloc (list, max * sizeof (char *));
> 	      else
> 		list = (char **) malloc (max * sizeof (char *));
> 	    }
> 	  /* Strip leading whitespace.  */
> 	  while (*sp && isspace ((int) *sp))
> 	    sp++;
> 	  list[num++] = sp;

[code snip].


I think a fix for this would be to make the static variables non-static,
remove the conditional free at the start of the function, and instead
free the string and argument list at all exit points from the function.

We could use a goto to gather any error, or success exit points
together.

Does anyone have any input as to whether this will fix the problem I'm
experiencing?

Does someone more familiar with the code wish to make these (or other)
changes, or should I get on and write this patch tomorrow?

Regards

Peter Clifton