[Toybox] Ls row segfault

[Rob Landley]

Sorry, busy with $DAYJOB through the 22nd.

Sigh: commit 3def73006aa0 included:

@@ -384,11 +359,10 @@ static void listfiles(int dirfd, struct dirtree *indir)
       unsigned c, totlen = columns;

       memset(colsizes, 0, columns*sizeof(unsigned));
-      for (ul=0; ul<dtlen; ul++) {
+      for (ul = 0; ul<dtlen; ul++) {
+        // measure each entry, plus two spaces between filenames
         entrylen(sort[next_column(ul, dtlen, columns, &c)], len);
-        // Add `2` to `totpad` to ensure two spaces between filenames
-        *len += totpad+2;
-        if (c == columns) break;
+        if (c<columns-1) *len += totpad+2;
         // Expand this column if necessary, break if that puts us over budget
         if (*len > colsizes[c]) {
           totlen += (*len)-colsizes[c];

And I could NOT figure out what that if (c == columns ) break; was there for, so
I took it out. (It really seemed like it could never happen, and columns is
counting _down_ with a hard stop at 1 in the enclosing loop...) And I made a
note to fluff out the test suite more to try to figure out what the corner cases
actually _are_, I.E. something like:

$ for i in $(seq 10 100); do echo $i; ../toybox ls -Cw $i || break; done

And the segfault is happening for me with your directory at 84. Alright, what's
going on...

Ah, I remember now. Yes, something like that test needs to go back, the failure
case is that some column widths are impossible to display in vertical sort. (Any
time the number of blank entries in horizontal sort is more than the height
there's nothing left in the last column in vertical sort.)

That's why that test was there.

Rob


On 11/11/19 9:52 AM, enh via Toybox wrote:
> i don't see a crash when i follow your repro steps.
> 
> oh, an ASAN=1 toybox catches it:
> 
> =================================================================
> ==113660==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x611000002a70 at pc 0x56168ee299b1 bp 0x7ffff2586180 sp
> 0x7ffff2586178
> READ of size 8 at 0x611000002a70 thread T0
>     #0 0x56168ee299b0 in listfiles toys/posix/ls.c:364
> 
> 0x611000002a70 is located 8 bytes to the right of 232-byte region
> [0x611000002980,0x611000002a68)
> allocated by thread T0 here:
>     #0 0x7f6e6affc330 in __interceptor_malloc
> (/lib/x86_64-linux-gnu/libasan.so.5+0xe9330)
>     #1 0x56168eddc914 in xmalloc lib/xwrap.c:71
>     #2 0x56168ee29505 in listfiles toys/posix/ls.c:315
>     #3 0x56168ee293b7 in listfiles toys/posix/ls.c:299
>     #4 0x56168ee2b1fe in ls_main toys/posix/ls.c:571
>     #5 0x56168ede0be5 in toy_exec_which /tmp/toybox/main.c:170
>     #6 0x56168ede0c6c in toy_exec /tmp/toybox/main.c:177
>     #7 0x56168ede0cb8 in toybox_main /tmp/toybox/main.c:191
>     #8 0x56168ede0be5 in toy_exec_which /tmp/toybox/main.c:170
>     #9 0x56168ede0c6c in toy_exec /tmp/toybox/main.c:177
>     #10 0x56168ede0cb8 in toybox_main /tmp/toybox/main.c:191
>     #11 0x56168edca549 in main /tmp/toybox/main.c:255
>     #12 0x7f6e6a93f52a in __libc_start_main ../csu/libc-start.c:308
> 
> SUMMARY: AddressSanitizer: heap-buffer-overflow toys/posix/ls.c:364 in listfiles
> Shadow bytes around the buggy address:
>   0x0c227fff84f0: 00 00 00 00 00 00 00 00 00 04 fa fa fa fa fa fa
>   0x0c227fff8500: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
>   0x0c227fff8510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x0c227fff8520: 00 05 fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c227fff8530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> =>0x0c227fff8540: 00 00 00 00 00 00 00 00 00 00 00 00 00 fa[fa]fa
>   0x0c227fff8550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c227fff8560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c227fff8570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c227fff8580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c227fff8590: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> Shadow byte legend (one shadow byte represents 8 application bytes):
>   Addressable:           00
>   Partially addressable: 01 02 03 04 05 06 07
>   Heap left redzone:       fa
>   Freed heap region:       fd
>   Stack left redzone:      f1
>   Stack mid redzone:       f2
>   Stack right redzone:     f3
>   Stack after return:      f5
>   Stack use after scope:   f8
>   Global redzone:          f9
>   Global init order:       f6
>   Poisoned by user:        f7
>   Container overflow:      fc
>   Array cookie:            ac
>   Intra object redzone:    bb
>   ASan internal:           fe
>   Left alloca redzone:     ca
>   Right alloca redzone:    cb
> ==113660==ABORTING
> 
> On Mon, Nov 11, 2019 at 7:42 AM Denys Nykula <nykula at ukr.net> wrote:
>>
>> With 81 character wide terminal, toy ls dislikes my etc dir, my home dir
>> and most other dirs on my netbook, unless given -1 or -l.
>>
>> etc=`mktemp -d`; echo $etc; cd $etc
>> touch ImageMagick-7 Muttrc dbus-1 drirc.d dropbear fontconfig git group \
>> group- lynx.cfg lynx.lss mailcap mime.types mk.conf openssl passwd \
>> passwd- profile rc.d resolv.conf rhashrc sasl2 sgml shadow shadow- \
>> tmux.conf udev wpa_supplicant.conf xml
>> ls
>> # Segmentation fault
>>
>> Ls starts to like the dirs again when I remove some files.
>>
>> rm udev wpa_supplicant.conf xml
>> ls
>> # Okay
>> _______________________________________________
>> Toybox mailing list
>> Toybox at lists.landley.net
>> http://lists.landley.net/listinfo.cgi/toybox-landley.net
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
>