[PATCH][BUG] Improper handling of SIGINT after running wait causes crash
I have found an easily reproducible bug in bash that causes it to crash and have attached a fix. First, the bug: joey@charmander:~$ wait joey@charmander:~$ ^C joey@charmander:~$ true& [1] 6961 joey@charmander:~$ *** stack smashing detected ***: bash terminated Aborted (core dumped) All you have to do is run wait, press ctrl+C and enter (notice how the ^C is entered on the line as if it were a character instead of jumping to the next line), and immediately run any command in the background. What's happening here? In builtins/wait.def we have the code: code = setjmp (wait_intr_buf); if (code) { status = 128 + wait_signal_received; WAIT_RETURN (status); } The problem is that longjmp is being called to wait_intr_buf after the function already returned, so the stack is garbage. This happens because of this code in trap.c: if (this_shell_builtin && (this_shell_builtin == wait_builtin)) { wait_signal_received = sig; ... } and the CHECK_WAIT_INTR macro called in jobs.c and defined in quit.h: #define CHECK_WAIT_INTR \ do { \ if (wait_signal_received && this_shell_builtin && (this_shell_builtin == wait_builtin)) \ longjmp (wait_intr_buf, 1); \ } while (0) The root of the problem is that this_shell_builtin is not updated when a command finishes running. So if we put it all together: When wait is called this_shell_builtin is set to wait_builtin. The wait call finishes, and we send the program SIGINT. Since this_shell_builtin is still builtin_wait, wait_signal_received is set. We run another command in the background (since it's run in the background it doesn't change this_shell_builtin), and when the child command exits the waitchld command in jobs.c is called which calls CHECK_WAIT_INTR which longjumps back into the wait function that returned long ago. CRASH! The fix is relatively simple: just unset this_shell_builtin when commands finish running. Here's a patch: diff --git a/builtins/exit.def b/builtins/exit.def index 34728eb..2bdf078 100644 --- a/builtins/exit.def +++ b/builtins/exit.def @@ -126,11 +126,10 @@ exit_or_logout (list) if (stopmsg) { - /* This is NOT superfluous because EOF can get here without - going through the command parser. Set both last and this - so that either `exit', `logout', or ^D will work to exit - immediately if nothing intervenes. */ - this_shell_builtin = last_shell_builtin = exit_builtin; + /* This is NOT superfluous because EOF can get here without going + through the command parser. Set last_shell_builtin so that if you + try to exit again immediately after ^D the program will exit. */ + last_shell_builtin = exit_builtin; return (EXECUTION_FAILURE); } } diff --git a/execute_cmd.c b/execute_cmd.c index 9cebaef..5959ed2 100644 --- a/execute_cmd.c +++ b/execute_cmd.c @@ -4073,7 +4073,6 @@ execute_simple_command (simple_command, pipe_in, pipe_out, async, fds_to_close) if (words->word->word[0] == '%' && already_forked == 0) { this_command_name = async ? "bg" : "fg"; - last_shell_builtin = this_shell_builtin; this_shell_builtin = builtin_address (this_command_name); result = (*this_shell_builtin) (words); goto return_result; @@ -4105,7 +4104,6 @@ execute_simple_command (simple_command, pipe_in, pipe_out, async, fds_to_close) { run_unwind_frame ("simple-command"); this_command_name = "fg"; - last_shell_builtin = this_shell_builtin; this_shell_builtin = builtin_address ("fg"); started_status = start_job (job, 1); @@ -4128,7 +4126,6 @@ run_builtin: if (func == 0 && builtin == 0) builtin = find_shell_builtin (this_command_name); - last_shell_builtin = this_shell_builtin; this_shell_builtin = builtin; if (builtin || func) @@ -4240,6 +4237,10 @@ run_builtin: } discard_unwind_frame ("simple-command"); this_command_name = (char *)NULL; /* points to freed memory now */ + + last_shell_builtin = this_shell_builtin; + this_shell_builtin = (sh_builtin_func_t*)NULL; + return (result); } --- Since this_shell_builtin is zeroed after every function, handling of setting last_shell_builtin was moved to right before that. The only place that uses last_shell_builtin is in builtin_exit, which checks if the last call was exit in order to force quit even if there are stopped jobs. This code only needs to set last_shell_builtin, not this_shell_builtin, so the code and comment there were updated accordingly. This is my first time contributing, so let me know if I need to do something further to get the patch merged. --Joey
Re: autocd doesn't invoke customized `cd` wrapper
hi, I am not sure if autocd is supposed and expected to run just anything called "cd", but for what its worth, the following patch does what you want. diff --git a/execute_cmd.c b/execute_cmd.c index daa08c2..9a29da0 100644 --- a/execute_cmd.c +++ b/execute_cmd.c @@ -4297,6 +4297,11 @@ run_builtin: if (autocd && interactive && words->word && is_dirname (words->word->word)) { + if ((func = find_function("cd")) != NULL) +{ + words = make_word_list (make_word ("cd"), words); + goto run_builtin; +} words = make_word_list (make_word ("--"), words); words = make_word_list (make_word ("cd"), words); xtrace_print_word_list (words, 0); cheers, pg On Fri, Jan 1, 2016 at 10:41 PM, wrote: > Configuration Information [Automatically generated, do not change]: > Machine: x86_64 > OS: linux-gnu > Compiler: gcc > Compilation CFLAGS: -DPROGRAM='bash' -DCONF_HOSTTYPE='x86_64' > -DCONF_OSTYPE='linux-gnu' -DCONF_MACHTYPE='x86_64-pc-linux-gnu' > -DCONF_VENDOR='pc' -DLOCALEDIR='/usr/share/locale' -DPACKAGE='bash' -DSHELL > -DHAVE_CONFIG_H -I. -I../. -I.././include -I.././lib -D_FORTIFY_SOURCE=2 > -g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat > -Werror=format-security -Wall > uname output: Linux laptop 3.13.0-24-generic #47-Ubuntu SMP Fri May 2 > 23:30:00 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux > Machine Type: x86_64-pc-linux-gnu > > Bash Version: 4.3 > Patch Level: 11 > Release Status: release > > Description: > If `cd` is a function or an alias in my environment, I think autocd ought > to invoke > that instead of the raw `command cd` (like it does now). > > Personally, I keep a stack of recent directories and I record frequences of > the directories I'm > in in my `cd` wrapper, and it'd be nice if autocd didn't circumvent the > wrapper. > > Repeat-By: > > Steps to reproduce: > $ cd(){ echo cding; command cd "$@"; } > $ shopt -s autocd > $ /etc > cd /etc > # cd /var > cding > > Fix: > [Description of how to fix the problem. If you don't know a > fix for the problem, don't include this section.] >
Re: [PATCH][BUG] Improper handling of SIGINT after running wait causes crash
hi, nice one. reproducible on: #bash -version GNU bash, version 4.3.42(1)-release (x86_64-apple-darwin10.8.0) not reproducible on the same version on i686-pc-linux-gnu pg On Sat, Jan 2, 2016 at 2:08 PM, Joey Geralnik wrote: > I have found an easily reproducible bug in bash that causes it to crash and > have attached a fix. > > First, the bug: > > joey@charmander:~$ wait > joey@charmander:~$ ^C > > joey@charmander:~$ true& > [1] 6961 > joey@charmander:~$ *** stack smashing detected ***: bash terminated > Aborted (core dumped) > > All you have to do is run wait, press ctrl+C and enter (notice how the ^C is > entered on the line as if it were a character instead of jumping to the next > line), and immediately run any command in the background. > > What's happening here? In builtins/wait.def we have the code: > > code = setjmp (wait_intr_buf); > if (code) > { > status = 128 + wait_signal_received; > WAIT_RETURN (status); > } > > The problem is that longjmp is being called to wait_intr_buf after the > function already returned, so the stack is garbage. This happens because of > this code in trap.c: > > if (this_shell_builtin && (this_shell_builtin == wait_builtin)) > { > wait_signal_received = sig; > ... > } > > and the CHECK_WAIT_INTR macro called in jobs.c and defined in quit.h: > > #define CHECK_WAIT_INTR \ > do { \ > if (wait_signal_received && this_shell_builtin && (this_shell_builtin == > wait_builtin)) \ > longjmp (wait_intr_buf, 1); \ > } while (0) > > The root of the problem is that this_shell_builtin is not updated when a > command finishes running. > > So if we put it all together: > When wait is called this_shell_builtin is set to wait_builtin. The wait call > finishes, and we send the program SIGINT. Since this_shell_builtin is still > builtin_wait, wait_signal_received is set. We run another command in the > background (since it's run in the background it doesn't change > this_shell_builtin), and when the child command exits the waitchld command > in jobs.c is called which calls CHECK_WAIT_INTR which longjumps back into > the wait function that returned long ago. CRASH! > > The fix is relatively simple: just unset this_shell_builtin when commands > finish running. Here's a patch: > > diff --git a/builtins/exit.def b/builtins/exit.def > index 34728eb..2bdf078 100644 > --- a/builtins/exit.def > +++ b/builtins/exit.def > @@ -126,11 +126,10 @@ exit_or_logout (list) > >if (stopmsg) > { > - /* This is NOT superfluous because EOF can get here without > - going through the command parser. Set both last and this > - so that either `exit', `logout', or ^D will work to exit > - immediately if nothing intervenes. */ > - this_shell_builtin = last_shell_builtin = exit_builtin; > + /* This is NOT superfluous because EOF can get here without going > + through the command parser. Set last_shell_builtin so that if you > + try to exit again immediately after ^D the program will exit. */ > + last_shell_builtin = exit_builtin; >return (EXECUTION_FAILURE); > } > } > diff --git a/execute_cmd.c b/execute_cmd.c > index 9cebaef..5959ed2 100644 > --- a/execute_cmd.c > +++ b/execute_cmd.c > @@ -4073,7 +4073,6 @@ execute_simple_command (simple_command, pipe_in, > pipe_out, async, fds_to_close) >if (words->word->word[0] == '%' && already_forked == 0) > { >this_command_name = async ? "bg" : "fg"; > - last_shell_builtin = this_shell_builtin; >this_shell_builtin = builtin_address (this_command_name); >result = (*this_shell_builtin) (words); >goto return_result; > @@ -4105,7 +4104,6 @@ execute_simple_command (simple_command, pipe_in, > pipe_out, async, fds_to_close) > { >run_unwind_frame ("simple-command"); >this_command_name = "fg"; > - last_shell_builtin = this_shell_builtin; >this_shell_builtin = builtin_address ("fg"); > >started_status = start_job (job, 1); > @@ -4128,7 +4126,6 @@ run_builtin: >if (func == 0 && builtin == 0) > builtin = find_shell_builtin (this_command_name); > > - last_shell_builtin = this_shell_builtin; >this_shell_builtin = builtin; > >if (builtin || func) > @@ -4240,6 +4237,10 @@ run_builtin: > } >discard_unwind_frame ("simple-command"); >this_command_name = (char *)NULL; /* points to freed memory now */ > + > + last_shell_builtin = this_shell_builtin; > + this_shell_builtin = (sh_builtin_func_t*)NULL; > + >return (result); > } > > > --- > > Since this_shell_builtin is zeroed after every function, handling of setting > last_shell_builtin was moved to right before that. The only place that uses > last_shell_builtin is in builtin_exit, which checks if the last call was > exit in order to force quit even if there are stopped jobs. This code only > needs to set last_shell_builtin, not this_shell_builtin, so the code and > comment there were updated accordingly. >
Re: [PATCH][BUG] Improper handling of SIGINT after running wait causes crash
On Sat, Jan 02, 2016 at 03:08:54PM +0200, Joey Geralnik wrote: > I have found an easily reproducible bug in bash that causes it to crash and > have attached a fix. I think this was already reported here: - https://lists.gnu.org/archive/html/bug-bash/2015-07/msg00022.html And I think the fix is in: commit bce12dd773b5dbf2a470b463a1461dcd86a0f266 Author: Chet Ramey Date: Wed Jul 29 16:15:40 2015 -0400 commit bash-20150710 snapshot Which has this message: +. Fixed a bug that could cause SIGCHLD handling to be delayed after + running `wait' with no arguments and interrupting it with ^C without + a trap handler installed. + I couldn't reproduce it with the devel branch. I'd still wait for Chet to review it, maybe I'm missing something. -- Eduardo Bustamante https://dualbus.me/