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
