Hello, Eli.

Your patch looks good.  After I changed all instances of "batch_filename_ptr" to "batch_filename_p", I was able to compile the latest version of make from CVS in Windows and I verified that it fixes the crash I was experiencing.

Thank you for looking at this and fixing it!

--David

Hello, Eli.  Thank you for looking at my patch!

Someone else renamed batch_filename_ptr to batch_filename_p, but they did not change every reference to it.  The change I made with regards to batch_filename_ptr is required to make job.c compile under Windows.

The escaping of the shell's file name that I do in this patch is not actually seen by the shell.  It is just an internal implementation detail of the construct_command_argv_internal function.  Therefore the shell does not need to support any particular type of escaping for this patch to work.  I think the type of shell does not matter.

For example, suppose construct_comamnd_argv_internal is called with arguments of shell="C:/Program Files (x86)/Git/bin/sh.exe", shellflags="-c", and line="a&&b".  In make 3.82, the function will crash.  My patch fixes the function so that it returns an argv array consisting of {"C:/Program Files (x86)/Git/bin/sh.exe", "-c", "a&&b"}.  The escaping of the shell name added by this patch happens in the first call to construct_command_argv_internal but is removed by the second call to construct_command_argv_internal.

Surrounding the shell's name with quotes is a good idea and it is a lot simpler than my first solution.  I tested this by taking the source release of Make 3.82 and replacing the appropriate part of command_construct_argv_internal with this:

new_line = alloca (1 + shell_len + 2 + sflags_len + 1
                             + (line_len*2) + 1);
ap = new_line;
*(ap++) = '\'';
memcpy (ap, shell, shell_len);
ap += shell_len;
*(ap++) = '\'';

   
It seems to fix the problem.

I also tried to use double quotes instead of single quotes, but make crashed because of a different bug:  Apparently, make will always use the shell if the command line contains a double-quoted string.  When command_construct_argv_internal encounters the first double quote, it decides that it is "Not inside a string, but it's a special char."  This means that make is using the shell in many cases where it doesn't have to.  This is a separate issue, but until that issue is fixed we can't surround the shell name with double quotes.  Once again, the choice of whether to use double quotes, single quotes, or escaping is an internal implementation detail of command_construct_argv_internal and it doesn't need to be supported by the shell.

In conclusion, I think using single quotes is a good idea.  It is simpler than my original solution and the only drawback is that you can't have a shell name with a single quote in it.  Would you like me to make a new patch for single quotes based on the code above?

--David Grayson

Recent versions of GNU Make, including version 3.82, crash in Windows if the Makefile contains commands that need to be executed using a shell (for example, "a && b") and the path to the shell contains parentheses (for example, "C:\Program Files (x86)\").

This bug in Make will affect any Windows user who installs msysgit and chooses the installation option that puts git's bin directory (which contains sh.exe) on the PATH.

The bug happens in construct_command_argv_internal in job.c.  If this function detects that it needs to use a shell to execute a command line, it creates a new command line be joining three strings:
1) The shell name (e.g. "C:/Program Files (x86)/utils/sh.exe")
2) The shell flags (e.g. "-c").
3) The command line, with all special characters escaped.

The resulting string will be something like this:
C:/Program Files (x86)/utils/sh.exe -c a\ \&\&\ b

Note that spaces and parentheses in the shell name above have NOT been escaped.  After constructing this string, construct_command_argv_internal calls itself and passes that new string as the "line" argument.

The second invocation of the function unfortunately does the same thing: it detects that there are parentheses that are not escaped and not in a string, so it decides that it needs to use a shell.  It appends three strings together, and calls itself...  The result is an infinite loop, which eventually causes Make to crash.

The solution employed in this patch is to escape the shell name in the same way that the command line is escaped.  To do this, I moved the loop that escapes the command line in to its own function called "escape_string", and I call the new function once to escape the shell name and again to escape the command line. I did not change any of the functionality or comments in that escaping loop, I just moved it in to its own function to make it reusable.

This patch also fixes a minor problem that was preventing GNU Make from compiling in Windows: all references to "batch_filename_ptr" were changed to "batch_filename_p" because that is the actual name of this parameter.

I have confirmed that this patch fixes the bug in Windows.

Please let me know if you need any more information or want me to run any tests!