[Toybox] Two bugs in tail toy

Tue Oct 28 12:54:03 PDT 2014

W dniu 28.10.2014 o 01:22, Rob Landley pisze:
> On 10/27/14 05:21, luckboy at vp.pl wrote:
>> I again write mail to you because I found two bugs in tail toy and wrote
>> about these bugs to you and you didn't reply me.
> Sorry, a little overwhelmed with todo item backlog. (And spent the whole
> weekend working on sed...)
I understand it.
>> These bugs:
>> First bug is that tail added random character to stdout end when it read
>> from stdin.
> It's a little non-obvious from staring at it which change fixes which
> bug, and I got distracted actually testing the result because in commit
> 1523 (back on the 14th) I broke loopfiles subtly (the test for "are we
> in read only mode" was confused by O_CLOEXEC so it was using stdout
> instead of stdin, which broke simple stuff like _cat_ and I didn't
> notice for a bit. My bad. I'd wondered why aboriginal stopped building
> but hadn't had time to track it down this weekend due to banging on sed.)
>
>> Second bug is segmentation fault for 'tail -c 10' for big data from stdin.
> Indeed, a use after free error looks like. Good catch, thanks.
>
> That part I understand. It's the added random character I'm not seeing,
> and I don't understand what the orig_len change is doing?
Look at the following code at 159-167 lines:

         bytes += new->len; // add the field.
         if (bytes > 0) {
           while (list->len <= bytes) {
             bytes -= list->len; // there can subtract incorrect value 
from the byte variable
             free(dlist_pop(&list));
           }
           list->data += bytes;
           list->len -= bytes; // changed len that will used at the 
above loop.
         }

Fist instruction adds the value of the len field onto the bytes variable 
but last instruction
changes this len field and then, this changed value subtract from the 
bytes variable.
There should subtract the original value of the len field from the bytes 
variable because
the original value of the field was added onto the bytes variables. I 
added the orig_len
field onto the line_list structure because this value must be saved and 
then used at the
while loop.

>
>> Patch that fixes these bugs:
>> https://github.com/luckboy/toyroot/blob/master/patch/toybox-0.5.0-tail-n-c.patch
>>
>>
>> You can check occurrence of first bug by invoke command in the toybox
>> directory (toybox-0.5.0):
>>
>> cat toys/posix/cp.c | ./toybox tail
>>
>> Exemplary output:
>> ...
>> 00000140  69 6e 28 29 3b 0a 7d 0a  75 |in();.}.u|
>>
>> Last character is 'u'!
> Not happening for me here. I note that "cp.c" has changed several times
> in the past month, but testing with "hg cat -r 1472 toys/*/cp.c" (and
> 1521, and 1454, and that gets us back to august) isn't showing it either.
>

I found perfect way to reproduce this bug:

seq 1 4096 | ./toybox tail

Exemplary output:

Last character is printed without new line.

I try explain this bug. Look at the following code at 179-181 lines:

             do {
               if (!--(list->len)) free(dlist_pop(&list));
             } while (*(list->data++) != '\n');

The data pointer always be incremented but one case shouldn't be 
incremented. This
case is popping of element from list because element is new and data 
pointer indicate to
new character.

>> Also, you can occurrence of second bug by invoke commands in toybox
>> directory:
>>
>> dd if=/dev/urandom of=test.bin bs=1024 count=8
>> hexdump test.bin | ./toybox tail -c 10
>> Segmentation fault
>>
>> Łukasz Szpakowski.
> I checked in the use after free error. I'm not against applying the
> other one but I don't currently understand what it's doing and can't
> reproduce the issue.
>
> Thanks,
>
> Rob
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
Łukasz Szpakowski.

 1414526043.0