bug-bash
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Joey Geralnik
Subject: [PATCH][BUG] Improper handling of SIGINT after running wait causes crash
Date: Sat, 2 Jan 2016 15:08:54 +0200

I have found an easily reproducible bug in bash that causes it to crash and have attached a fix.

First, the bug:

address@hidden:~$ wait
address@hidden:~$ ^C

address@hidden:~$ true&
[1] 6961
address@hidden:~$ *** 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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]