#498 Dosbox shell crashes on key combination

[Bruenor41]

By fixing other stuff I came across funny crash in dosbox console, is happens in 0.74 and the same is 4180 svn. I am not sure how much worth it is to fix, well, I put it here, maybe someone will be interested, if I find some free time then I can look patch (if it will not be fixed).

How to reproduce it :
Be in dosbox black console. Press and hold CTRL, with second finger press key 'T' and let duplicate letter to second line or third line. Release 'T' and tap 2-3 times on key 'I'. If it still did not crash then release CTRL and press 'Enter'.

[Dash]

it happens after 82 paragarph signs(ctrl+t) and then searching using the ctrl+I

the log it brings

*** buffer overflow detected ***: ./dosbox terminated
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7fe6965517e5]
/lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x5c)[0x7fe6965f315c]
/lib/x86_64-linux-gnu/libc.so.6(+0x117160)[0x7fe6965f1160]
/lib/x86_64-linux-gnu/libc.so.6(+0x116405)[0x7fe6965f0405]
./dosbox[0x60d968]
./dosbox[0x60540a]
./dosbox[0x604f55]
./dosbox(main+0x54a)[0x40896a]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7fe6964fa830]
./dosbox[0x409969]

by gdb the trace is

 0x00007ffff5ffe428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff600002a in __GI_abort () at abort.c:89
#2  0x00007ffff60407ea in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x7ffff615849f "*** %s ***: %s terminated\n") at ../sysdeps/posix/libc_fatal.c:175
#3  0x00007ffff60e215c in __GI___fortify_fail (msg=<optimized out>, msg@entry=0x7ffff6158430 "buffer overflow detected") at fortify_fail.c:37
#4  0x00007ffff60e0160 in __GI___chk_fail () at chk_fail.c:28
#5  0x00007ffff60df405 in __stpcpy_chk (dest=dest@entry=0x7fffffffcda0 '\024' <repeats 79 times>, "*.*", src=src@entry=0x7fffffffcef0 '\024' <repeats 80 times>, destlen=destlen@entry=80)
    at stpcpy_chk.c:31
#6  0x000000000060d968 in strcpy (__src=0x7fffffffcef0 '\024' <repeats 80 times>, __dest=0x7fffffffcda0 '\024' <repeats 79 times>, "*.*") at /usr/include/x86_64-linux-gnu/bits/string3.h:110
#7  DOS_Shell::InputCommand (this=this@entry=0x37b3ab0, line=line@entry=0x7fffffffcef0 '\024' <repeats 80 times>) at shell_misc.cpp:258
#8  0x000000000060540a in DOS_Shell::Run (this=0x37b3ab0) at shell.cpp:343
#9  0x0000000000604f55 in SHELL_Init () at shell.cpp:750
#10 0x000000000040896a in main (argc=<optimized out>, argv=<optimized out>) at sdlmain.cpp:2181

it seems that the cause of the issue is mask in

(gdb) print mask
$1 = 2147483647

so the issue seems to be around this bit in src/shell/shell_misc.cpp

char *p_completion_start = strrchr(line, ' ');

                    if (p_completion_start) {
                        p_completion_start ++;
                        completion_index = (Bit16u)(str_len - strlen(p_completion_start));
                    } else {
                        p_completion_start = line;
                        completion_index = 0;
                    }

                    char *path;
                    if ((path = strrchr(line+completion_index,'\\'))) completion_index = (Bit16u)(path-line+1);
                    if ((path = strrchr(line+completion_index,'/'))) completion_index = (Bit16u)(path-line+1);

                    // build the completion list
                    char mask[DOS_PATHLENGTH];

                    if (p_completion_start) {
                        strcpy(mask, p_completion_start);

adding limit to the condition in if (p_completion_start) that check if p_condition_start is in the scope of DOS_PATHLENGTH would be the basic solution though it seems that adding a check if the string is empty would be more comprehensive

[Qbix]

I'll check. thanks for the initial analysis Dash.

I misinterpreted the initial report wrong, I though Bruenor was talking about the console window (on windows) that can be hidden with -noconsole and couldn't reproduce the problem at all.

[Dash]

no problem, it's been awhile since i've touched c++ but i've checked locally to verify that this is the source of the issue, and changing

if (p_completion_start)

to

if (p_completion_start && strlen(p_completion_start) < DOS_PATHLENGTH)

seems to fix the crush and append the search function after the paragraph signs + whatever letter is after them as the code designate in the else case.(DOS_PATHLENGTH being the limit given to the string "mask" which casued the buffer overflow),

[Qbix]

Yeah, that is the location where it goes wrong, but your fix is not 100% correct.
As 3 characters are added to the p_completions_start in some cases

strcat(mask,"*.*");

so

strlen(p_completion_start) +3 < DOS_PATHLENGTH 

would be more accurate.

However, I am not sure if using the else for when near the DOS_PATHLENGTH is the right way.
As we would be generating unrelevant results (as mask would only contain "." then). Might be better to not generate a completion_list at all in that case.

[Bruenor41]

Thank you