Re: [Toybox] Sigh. Anybody spot the bug?
Rob Landley wrote:
[...]
than count*sizeof(toybuf) and contain no newlines.
Count is initialized in a for loop and starts at 0 on line 164 so I
dunno what this means...
Sorry for the confusion. I meant the number of lines tail is told to
output.
[...]
diff -r f8db1f6ec4ab toys/posix/tail.c
--- a/toys/posix/tail.c Tue Jul 02 00:16:16 2013 -0500
+++ b/toys/posix/tail.c Wed Jul 03 20:54:24 2013 +0200
@@ -168,7 +168,7 @@
}
if (lines) {
- if(try[count] != '\n' count != len-1) continue;
+ if(try[count] != '\n') continue;
Hmmm, I believe that's related to getting the line count right when the
last line doesn't end with a newline? But I don't remember the
details...
See Isaac's replies in this topic. The current condition is just wrong:
$ cat toys.h toys.h toys.h | tail | wc -l
9
This is fixed with the patch. However in some situations it still gives
wrong output since other code still assumes that there are newlines at
the end of each sizeof(toybuf) piece. Maybe it's simpler to rewrite this
part...
Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] Sigh. Anybody spot the bug?
On 07/03/2013 01:56:46 PM, Felix Janda wrote:
Rob Landley wrote:
Tail has a double free somewhere. (Aboriginal's more/buildall.sh is
complaining, that uses toybox in host-tools.) Haven't had time to
track
it down yet, wondering if anybody else could spot it.
From the behavior it's looking like it's on file close...
[...]
It seems reproducible when using tail on non-seekable files which are
bigger
than count*sizeof(toybuf) and contain no newlines.
Count is initialized in a for loop and starts at 0 on line 164 so I
dunno what this means...
~/toybox tail -f /dev/zero
Ok, that shouldn't return immediately.
Ah, cat the android sdk | ./toybox tail -f does indeed segfault.
Reproducible!
The following seems to fix it. I don't really understand the code
though.
It started life as an external submission meaning I could easily have
missed something while cleaning it up.
In fact this command has todo items in it (comments at the end of
tail_main() and do_tail() about -f support). Apparently I was in the
middle of working on it and lost track under the flood of new commands
I don't get to write because I'm cleaning up submissions.
diff -r f8db1f6ec4ab toys/posix/tail.c
--- a/toys/posix/tail.c Tue Jul 02 00:16:16 2013 -0500
+++ b/toys/posix/tail.c Wed Jul 03 20:54:24 2013 +0200
@@ -168,7 +168,7 @@
}
if (lines) {
- if(try[count] != '\n' count != len-1) continue;
+ if(try[count] != '\n') continue;
Hmmm, I believe that's related to getting the line count right when the
last line doesn't end with a newline? But I don't remember the
details...
Poking at it...
Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] Sigh. Anybody spot the bug?
Isaac wrote: On Wed, Jul 03, 2013 at 08:56:46PM +0200, Felix Janda wrote: Rob Landley wrote: Tail has a double free somewhere. (Aboriginal's more/buildall.sh is complaining, that uses toybox in host-tools.) Haven't had time to track it down yet, wondering if anybody else could spot it. From the behavior it's looking like it's on file close... [...] It seems reproducible when using tail on non-seekable files which are bigger than count*sizeof(toybuf) and contain no newlines. I presume this is with TAIL_SEEK=y. No. With TAIL_SEEK=n it's even easier to trigger. Just try to tail any large file. I'm not sure what count*sizeof(toybuf) means since count is a loop counter. Would a 4097-byte file with no \n cause it? Or 512*4096+1? (I'm wanting to test the other proposed solution.) With count I meant the argument to the -n option (default 10). (Sorry for the confusion.) Just do a yes | paste -s | tail to trigger the bug. The other proposed solution can't solve this specific problem since the changed code isn't touched in this situation. Felix ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] Sigh. Anybody spot the bug?
On Sat, Jul 06, 2013 at 03:52:02PM +0200, Felix Janda wrote:
Isaac wrote:
On Wed, Jul 03, 2013 at 08:56:46PM +0200, Felix Janda wrote:
Rob Landley wrote:
Tail has a double free somewhere. (Aboriginal's more/buildall.sh is
complaining, that uses toybox in host-tools.) Haven't had time to track
it down yet, wondering if anybody else could spot it.
From the behavior it's looking like it's on file close...
[...]
It seems reproducible when using tail on non-seekable files which are
bigger
than count*sizeof(toybuf) and contain no newlines.
I presume this is with TAIL_SEEK=y.
No. With TAIL_SEEK=n it's even easier to trigger. Just try to tail any large
file.
I'm not sure what count*sizeof(toybuf) means since count is a loop
counter.
Would a 4097-byte file with no \n cause it? Or 512*4096+1?
(I'm wanting to test the other proposed solution.)
With count I meant the argument to the -n option (default 10). (Sorry for the
confusion.) Just do a
yes | paste -s | tail
to trigger the bug.
Thanks.
A test for this might be good; perhaps something along the lines of
{ for i in `seq 4097`; do printf a; done; } |tail -n 1
with the other side being $(for i in `seq 4097`; do printf a; done)
AFAICT, the old code meant each buffer end is treated as a line end,
which would appear to result in a high line count in some circumstances.
Isaac Dunham
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] Sigh. Anybody spot the bug?
Rob Landley wrote:
Tail has a double free somewhere. (Aboriginal's more/buildall.sh is
complaining, that uses toybox in host-tools.) Haven't had time to track
it down yet, wondering if anybody else could spot it.
From the behavior it's looking like it's on file close...
[...]
It seems reproducible when using tail on non-seekable files which are bigger
than count*sizeof(toybuf) and contain no newlines.
The following seems to fix it. I don't really understand the code though.
diff -r f8db1f6ec4ab toys/posix/tail.c
--- a/toys/posix/tail.c Tue Jul 02 00:16:16 2013 -0500
+++ b/toys/posix/tail.c Wed Jul 03 20:54:24 2013 +0200
@@ -168,7 +168,7 @@
}
if (lines) {
- if(try[count] != '\n' count != len-1) continue;
+ if(try[count] != '\n') continue;
if (lines0) {
if (!++lines) ++lines;
continue;
Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] Sigh. Anybody spot the bug?
On Wed, Jul 3, 2013 at 2:52 AM, Rob Landley r...@landley.net wrote: Tail has a double free somewhere. (Aboriginal's more/buildall.sh is complaining, that uses toybox in host-tools.) Haven't had time to track it down yet, wondering if anybody else could spot it. From the behavior it's looking like it's on file close... For what it's worth, running programs under valgrind tends to catch this sort of problem instantly. ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] Sigh. Anybody spot the bug?
On 4 July 2013 07:00, Avery Pennarun apenw...@gmail.com wrote:
On Wed, Jul 3, 2013 at 2:52 AM, Rob Landley r...@landley.net wrote:
Tail has a double free somewhere. (Aboriginal's more/buildall.sh is
complaining, that uses toybox in host-tools.) Haven't had time to track it
down yet, wondering if anybody else could spot it.
From the behavior it's looking like it's on file close...
For what it's worth, running programs under valgrind tends to catch
this sort of problem instantly.
Valgrind complains about using uninitialised values in llist_traverse,
called from try_lseek:
==15446== Conditional jump or move depends on uninitialised value(s)
==15446==at 0x40AF4F: llist_traverse (llist.c:18)
==15446==by 0x41BF75: try_lseek (tail.c:126)
==15446==by 0x41C029: do_tail (tail.c:150)
==15446==by 0x409C26: loopfiles_rw (lib.c:850)
==15446==by 0x409C7F: loopfiles (lib.c:858)
==15446==by 0x41C260: tail_main (tail.c:221)
==15446==by 0x404945: toy_exec (main.c:104)
==15446==by 0x404A2F: toybox_main (main.c:126)
==15446==by 0x404945: toy_exec (main.c:104)
==15446==by 0x404A2F: toybox_main (main.c:126)
==15446==by 0x404BAA: main (main.c:163)
Having a look at the code, it would appear that tail.c/get_chunk
doesn't set line-next to anything meaningful, so if you're still
working on the first chunk, then it won't be set at all. Changing it
as follows seems to remove the warning.
diff -r 6a37f642b572 toys/posix/tail.c
--- a/toys/posix/tail.c Sat Jun 08 14:11:41 2013 -0500
+++ b/toys/posix/tail.c Thu Jul 04 08:37:31 2013 +1200
@@ -49,6 +49,7 @@
line-data = ((char *)line) + sizeof(struct line_list);
line-len = readall(fd, line-data, len);
+ line-next = NULL;
if (line-len 1) {
free(line);
Not sure if this is what was causing Rob's crash though.
Regards,
Andre
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
