patchmake - Patches: patch #7522, Fix crash in Windows when shell...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #7522: Fix crash in Windows when shell name has parentheses

Submitter:  David Grayson <davidegrayson>
Submitted:  Sun 03 Apr 2011 08:05:57 PM UTC
   
 
Category:  None Priority:  7 - High
Status:  Done Privacy:  Public
Assigned to:  eliz Open/Closed:  Closed
Fixed Release:  CVS

Tue 24 May 2011 06:09:44 AM UTC, comment #4: 

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

David Grayson <davidegrayson>
Sat 07 May 2011 08:50:20 AM UTC, comment #3: 

Answering my own question: yes, this is specific to Unixy shells only.  Windows shells have special characters that cannot possibly appear in Windows file names, so this particular problem cannot happen with them.

Therefore, I solved the problem with a slightly different, but similar change, which escape-protects special characters in SHELL when they are copied into the new_line variable in preparation to recursive invocation of construct_command_argv_internal.  I didn't want to use '..' quoting, because some day they could be added to the characters special to the shell.  I also consider the original patch too radical for such a simple problem.  So I installed a simpler change.

This issue happens to be the subject of Savannah bug #23922.  See there for the patch I actually committed.

Thank you for your contribution.

Eli Zaretskii <eliz>
Group Member
Mon 25 Apr 2011 12:10:35 AM UTC, comment #2: 

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

David Grayson <davidegrayson>
Tue 19 Apr 2011 03:57:53 PM UTC, comment #1: 

Is this problem specific to unixy shells only?  Because the method of protecting shell file names with spaces used by the nw function (and its code that was extracted from construct_command_argv_internal) will only work with Unixy shells; Windows shells don't support escaping characters with backslashes.

Perhaps it's better to enclose the file name of the shell in double quotes, that would be easier programmatically and will work with any shell.

I also don't understand the renaming of batch_filename_ptr to batch_filename_p.  Is that required?

Eli Zaretskii <eliz>
Group Member
Sun 03 Apr 2011 08:05:57 PM UTC, original submission:  

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!

David Grayson <davidegrayson>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by psmith (Updated the item)
  • -email is unavailable- added by eliz (Posted a comment)
  • -email is unavailable- added by davidegrayson (Submitted the item)
  • -email is unavailable- added by davidegrayson
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-05-07 psmith Assigned toNone eliz
    2011-05-07 eliz StatusNone Done
        Open/ClosedOpen Closed
        Fixed ReleaseNone CVS
    2011-04-03 davidegrayson Attached File- Added win32_crash_fix.patch, #23093
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code