Completion crashes when using the vi line editor.

[0branch]

Description of problem:
ksh crashes on tab completion when using the vi line editor.

Ksh version:
Current HEAD, 8e4c8f09, i.e.

version         sh (AT&T Research) 2020.0.0-beta1-158-g8e4c8f09

How reproducible:
Consistently reproducible on my macOS and OpenBSD systems.

Steps to reproduce:

1. Select the vi line editor (set -o vi)
2. Hit tab twice at the prompt to trigger completion
3. Observe crash

Actual results:
Abort trap with a core dump on OpenBSD; bus error on macOS.

Additional info:
This assignment seems to be the immediate cause:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x1002a5ffc)
    frame #0: 0x000000010001d66d ksh`ed_expand(ep=0x0000000100800000, outbuff="", cur=0x000000010080016c, eol=0x0000000100800170, mode=61, count=-1) at completion.c:261:13
   258 	        wchar_t *cp;
   259 	        cp = (wchar_t *)outbuff + *cur;
   260 	        c = *cp;
-> 261 	        *cp = 0;
   262 	        *cur = ed_external((wchar_t *)outbuff, (char *)stkptr(shp->stk, 0));
   263 	        *cp = c;
   264 	        *eol = ed_external((wchar_t *)outbuff, outbuff);

(lldb on macOS 10.14.6)

[0branch]

PS—No such crash with set -o emacs.

[krader1961]

The command completion behavior is extremely user unfriendly. This works in ksh93v- but on my system produces 2805 lines of output before displaying another prompt with the cursor incorrectly positioned. You have to type the number of one of the displayed commands then press [tab] to place it, incorrectly, on the current line and allow typing more arguments. If you just press [enter] the shell exits.

The problem is that ed_expand() is called with cur_virt == -1. Thus causing an attempt to modify the value before the start of virtual/outbuff. TBD is why that isn't a problem in the ksh93v- code.

P.S., I had forgotten how painful, and slow, building the ksh93v- version is (12 minutes on my system). This makes debugging issues where comparison is between the current and ksh93v- code extremely slow and painful.

[krader1961]

This failure mode is unlikely to occur in the ksh93v- release due to its use of AST Vmalloc. Which we know masks a large number of memory management bugs because we've already fixed a huge number that were exposed when we switched from AST Vmalloc to the system malloc.

The failure is due to a really bad anti-pattern we've seen elsewhere in the code. Specifically, temporarily modifying the byte immediately before or after a dynamically allocated buffer (in this case before). The SIGBUS on macOS is because the buffer is page aligned and the previous VM region is a 4KiB page containing malloc metadata that can be read but not modified. That's why the c = *cp; succeeds but the *cp = 0; fails.

Here is what we learn when running with ASAN enabled:

==48713==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6310000007fc at pc 0x0001005e63fa bp 0x7ffeef66d290 sp 0x7ffeef66d288
READ of size 4 at 0x6310000007fc thread T0
    #0 0x1005e63f9 in ed_expand completion.c:263
    #1 0x10064adfd in vi_textmod vi.c:1723
    #2 0x100640fc2 in cntlmode vi.c:404
    #3 0x10063a542 in vi_getline vi.c:928
    #4 0x100632c1a in ed_viread vi.c:254
    #5 0x1006bdf26 in slowread io.c:2013
    #6 0x100946a6e in sfrd sfrd.c:236
    #7 0x10091fa94 in _sffilbuf sffilbuf.c:100
    #8 0x10094b152 in sfreserve sfreserve.c:137
    #9 0x100735f19 in exfile main.c:472
    #10 0x10073ad42 in sh_main main.c:314
    #11 0x10058945b in main pmain.c:39
    #12 0x7fff63c7c3d4 in start (libdyld.dylib:x86_64+0x163d4)

0x6310000007fc is located 4 bytes to the left of 65537-byte region [0x631000000800,0x631000010801)
allocated by thread T0 here:
    #0 0x100c0a9f3 in wrap_malloc (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x5e9f3)
    #1 0x1008ac71b in ast_malloc vmbusy.c:25
    #2 0x1006bb3a1 in sh_iostream io.c:324
    #3 0x1006ba179 in sh_ioinit io.c:236
    #4 0x1006a6460 in sh_init init.c:1136
    #5 0x1007378ed in sh_main main.c:114
    #6 0x10058945b in main pmain.c:39
    #7 0x7fff63c7c3d4 in start (libdyld.dylib:x86_64+0x163d4)

[krader1961]

Also, I built an instrumented version of ksh93v- to report critical values like cur_virt which is the value set to -1 that is causing the out of bound buffer access. I confirmed it has the same value at critical junctures as the current master branch source. So this is not a bug we introduced -- it's a day zero bug.

[0branch]

As noted on the pull request thread (#1437), while @krader1961's change seems to fix the original issue I've observed a related ed_expand crash in the following situation:

• Launch ksh -p (or ksh) for an interactive session; set -o vi
• Type a single letter (e.g. l)
• Hit TAB to generate a completion list
• Observe crash

In master,

  $ lProcess 90586 stopped
  * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x1002a5ffc)
      frame #0: 0x000000010001d66d ksh`ed_expand(ep=0x0000000100800000, outbuff="", cur=0x000000010080016c, eol=0x0000000100800170, mode=61, count=-1) at completion.c:261:13
     258 	        wchar_t *cp;
     259 	        cp = (wchar_t *)outbuff + *cur;
     260 	        c = *cp;
  -> 261 	        *cp = 0;
     262 	        *cur = ed_external((wchar_t *)outbuff, (char *)stkptr(shp->stk, 0));
     263 	        *cp = c;
     264 	        *eol = ed_external((wchar_t *)outbuff, outbuff);

In the command-completion branch,

  $ Process 90711 stopped
  * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x1002a5fff)
      frame #0: 0x000000010001e7ea ksh`ed_expand(ep=0x0000000100800000, outbuff="", cur=0x000000010080016c, eol=0x0000000100800170, mode=0, count=-1) at completion.c:485:23
     482 	        // First re-adjust cur.
     483 	        int c, n = 0;
     484 	        c = outbuff[*cur];
  -> 485 	        outbuff[*cur] = 0;
     486 	        for (out = outbuff; *out; n++) mb1char(&out);
     487 	        outbuff[*cur] = c;
     488 	        *cur = n;
  Target 0: (ksh) stopped.

Would you like me to open a separate issue?

[krader1961]

Would you like me to open a separate issue?

Normally, yes. But it just so happens the new failure is so closely related to the original failure we might as well track both in this issue. I obviously neglected to check if there was any other place in ed_expand() which used *cur without validating it was greater than zero. What's weird is I don't see why this particular temporary buffer modification is even necessary. This looks suspiciously like the person was simply accustomed to using a hammer to drive a screw and inappropriately used the hammer here when a screwdriver would have made more sense.

[krader1961]

Gah! Looking at the code block containing the new failure made me upchuck my lunch. I sincerely hope the person who wrote this code has learned not to do things like this or is no longer writing code.

[krader1961]

Please explain to me the purpose of this statement:

ast/src/cmd/ksh93/edit/completion.c

Line 267 in ffbfaf6

out = outbuff + *cur + (sh_isoption(shp, SH_VI) != 0);

Specifically, why it is predicating adding an additional one on the current edit mode being vi. And why is this function seemingly using space in the char outbuff[] argument for temporary use? How in the world is that safe?

[krader1961]

The completion code is badly broken. Using ksh93v-, and attempting to complete commands containing (or beginning with?) l, includes every single command in $PATH. The presence of the l in the buffer does not limit the matching commands.

P.S., Interestingly, ksh93u+ handles l[tab][tab] correctly when in vi mode while ksh93v- does not. So the AST team introduced a vi mode bug after the ksh93u+ release.