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 <jgeral...@gmail.com> 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. > > This is my first time contributing, so let me know if I need to do something > further to get the patch merged. > --Joey