[Toybox] Two bugs in tail toy

[luckboy at vp.pl]

W dniu 28.10.2014 o 22:50, Rob Landley pisze:
> On 10/28/14 14:54, luckboy at vp.pl wrote:
>> 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?
> ...
>> I found perfect way to reproduce this bug:
>>
>> seq 1 4096 | ./toybox tail
>>
>> Exemplary output:
>>
>> 4087
>> 4088
>> 4089
>> 4090
>> 4091
>> 4092
>> 4093
>> 4094
>> 4095
>> 4096
>> 8
>>
>> Last character is printed without new line.
> $ ./toybox seq 1 4096 | ./toybox tail | ./toybox od -t x1
> 0000000 34 30 38 37 0a 34 30 38 38 0a 34 30 38 39 0a 34
> 0000020 30 39 30 0a 34 30 39 31 0a 34 30 39 32 0a 34 30
> 0000040 39 33 0a 34 30 39 34 0a 34 30 39 35 0a 34 30 39
> 0000060 36 0a
> 0000062
>
> I'm still not seeing it. That's make defconfig against a clean checkout.
>
> What build environment are you using?
>
> Rob
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
>
I see that you just fixed bug that adding of random character in 
http://www.landley.net/hg/toybox/rev/1d996b0a11c0.

Second bug for `tail -c 10` isn't fixed. You should also fix second bug. 
I again explain these bugs:

***
*** First bug
***

First bug is that tail added random character to stdout end when it read 
from stdin.
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 in one case, this pointer 
shouldn't be
incremented. This case is popping of element from list because element 
is new and data
pointer indicate to new character.

Patch:

@@ -176,9 +177,14 @@
            linepop = try[count] == '\n';

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

Way to reproduce this bug: seq 1 4096 | ./toybox tail

***
*** Second bug
***

Second bug is segmentation fault for 'tail -c 10' for big data from stdin.
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:

@@ -41,6 +41,7 @@
    struct line_list *next, *prev;
    char *data;
    int len;
+  int orig_len;
  };

  static struct line_list *get_chunk(int fd, int len)
@@ -49,7 +50,7 @@

    memset(line, 0, sizeof(struct line_list));
    line->data = ((char *)line) + sizeof(struct line_list);
-  line->len = readall(fd, line->data, len);
+  line->len = line->orig_len = readall(fd, line->data, len);

    if (line->len < 1) {
      free(line);
@@ -156,10 +157,10 @@

        // If tracing bytes, add until we have enough, discarding overflow.
        if (TT.bytes) {
-        bytes += new->len;
+          bytes += new->len;
          if (bytes > 0) {
-          while (list->len <= bytes) {
-            bytes -= list->len;
+          while(list->orig_len <= bytes) {
+            bytes -= list->orig_len;
              free(dlist_pop(&list));
            }
            list->data += bytes;

Please, check occurrence of second bug.

Łukasz Szpakowski.



 1414683524.0