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

2016-01-02 Thread Joey Geralnik
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

2016-01-02 Thread Piotr Grzybowski
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

2016-01-02 Thread 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  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

2016-01-02 Thread Eduardo A . Bustamante López
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/