W dniu 28.10.2014 o 22:50, Rob Landley pisze:
On 10/28/14 14:54, [email protected] wrote:
W dniu 28.10.2014 o 01:22, Rob Landley pisze:
On 10/27/14 05:21, [email protected] 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
[email protected]
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.
_______________________________________________
Toybox mailing list
[email protected]
http://lists.landley.net/listinfo.cgi/toybox-landley.net
