Re: [PATCH][BUG] Improper handling of SIGINT after running wait causes crash

[Piotr Grzybowski]

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 <jgeralnik@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