[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: potential bug in ash
From:       "Denis Vlasenko" <vda.linux () googlemail ! com>
Date:       2007-07-29 13:56:09
Message-ID: 1158166a0707290656y1e73c001t81d4a1c9e709da59 () mail ! gmail ! com
[Download RAW message or body]

On 7/28/07, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> Hi
>
> Here is an excerpt from ash.c:
>                 if (pathopt) {          /* this is a %func directory */
>                         stalloc(strlen(fullname) + 1);
>                         readcmdfile(fullname);
>                         cmdp = cmdlookup(name, 0);
>                         if (cmdp == NULL || cmdp->cmdtype != CMDFUNCTION)
>                                 ash_msg_and_raise_error("%s not defined in %s", name, fullname);
>                         stunalloc(fullname);
>                         goto success;
>                 }
>
> gmail may corrupt the code but you can find the code in function find_command().
>
> fullname is not updated after stalloc(). It is perfectly fine now
> because there isn't any stalloc inside the while loop. But if in
> future you decide to allocate some blocks before that excerpt, the
> return value from stalloc(strlen(fullname) + 1) may no longer be
> fullname and then stunalloc(fullname) will blindly screw up the stack.

Whee, somebody who can read ash.c! :)

That stalloc you talk about is highly non-obvious, but it's not
the only grey code there.

Code seems to have more uses of fullname after stunalloc(fullname):

               stunalloc(fullname);
...
               if (fullname[0] == '/' && idx <= prev) {
...
               while (stat(fullname, &statb) < 0) {

Which is technically valid (I think) as long as you don't stalloc
something.

I will add big comment for now.
--
vda
_______________________________________________
busybox mailing list
busybox@busybox.net
http://busybox.net/cgi-bin/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]