bugmake - Bugs: bug #56449, job.c...

 
 

bug #56449: job.c (construct_command_argv_internal): Must use shell if '%' character is present in recipe line

Submitter:  None
Submitted:  Thu 06 Jun 2019 11:13:33 AM UTC
   
 
Severity:  1 - Wish Item Group:  Bug
Status:  Wont Fix Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Component Version:  4.3 Operating System:  MS Windows
Fixed Release:  None Triage Status:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 10 Sep 2019 04:14:30 PM UTC, comment #13: 

Per recent discussions, I've now reverted the fix for this bug, and I'm closing it as "wont fix".

Eli Zaretskii <eliz>
Group Member
Fri 30 Aug 2019 07:11:12 PM UTC, comment #12: 

comment #10:

> GNU Make emulates the behavior of the shell as if the user typed the commands at the shell's prompt.  It is true that you need to double the % characters in batch files, but GNU Make doesn't behave like batch files do.
>
> So I think this change is for the worse, and should be reverted.


Ack. If I remember correctly, the double % were caused by cmake (unfortunately I cannot verify this in short term). Maybe cmake expects that for MingW Makefiles it needs to convert a single % into a double one. If this is wrong, this behavior should changed in cmake.

Christian Eggers <ceggers01>
Fri 30 Aug 2019 02:14:19 PM UTC, comment #11: 


> I also remember that using echo is not a good idea for demonstration with make.


I also mentioned calling a Windows program with the argument of %%.  The program in this case gets the literal two % characters in its argv[] array.

If you invoke your argv.exe program from cmd.exe prompt with "%%" as a command-line argument, don't you see the same output as what mingw-make.exe produced before this change?  That is what I see, and that tells me that the change is simply wrong, as it makes GNU Make behave like a batch file processor, which is not what's expected.

Eli Zaretskii <eliz>
Group Member
Fri 30 Aug 2019 02:10:05 PM UTC, comment #10: 

GNU Make emulates the behavior of the shell as if the user typed the commands at the shell's prompt.  It is true that you need to double the % characters in batch files, but GNU Make doesn't behave like batch files do.

So I think this change is for the worse, and should be reverted.

Eli Zaretskii <eliz>
Group Member
Fri 30 Aug 2019 12:52:29 PM UTC, comment #9: 

comment #6:

> But the result is wrong when %..% doesn't specify a known variable, most probably because we invoke the command through a batch file.


The question may be, which of both results is the correct one. So if cmd.exe has something like an "interactive" and "batch" mode, I personally would consider the batch mode as the correct one (I would rather enter all my build commands into a long batch file than typing everything manually).

So I would expect that recipe lines in make behave equally to the batch mode...

Christian Eggers <ceggers>
Fri 30 Aug 2019 12:46:23 PM UTC, comment #8: 

comment #5:

> Sorry, I think there's some confusion.  All this patch does is detect if we see a % in the command line and if so we do not take the fast path: instead we take the slow path and invoke command.com.
>
> So we're not trying to handle environment variables etc., we're just saying "hey make, if the recipe contains % then give up and pass it to command.com to take care of".


Yes, that is exactly what I intended to change.

Christian Eggers <ceggers>
Fri 30 Aug 2019 12:42:24 PM UTC, comment #7: 

Hi, I'm the original author of this bug. It's some time ago I wrote this, so maybe I've forgotten some details...

comment #2:

> I see that this bug was fixed and closed, but looking at the result, I'm not sure the fix is correct.  If I invoke a Windows program from the cmd.exe prompt with %% as argument, the program gets "%%" as its argv[1], not just a single %.
>
> So I guess I don't understand why the OP expected to get a single % character.  Am I missing something?


I guess that cmd.exe may distinguish between "interactive mode" and "script mode".
So in interactive mode it will not change double % to a single %. But in
script mode it does. For this moment I have no windows machine, but on
wineconsole I get different output when I

- type "echo %%"                         --> I get double %
- insert "echo %%" into a test.cmd file  --> I get single %

I also remember that using echo is not a good idea for demonstration with make. As make recognizes echo as a shell builtin it always invokes this via a shell.
That is the reason why I sent the demo program.

Christian Eggers <ceggers>
Wed 28 Aug 2019 07:24:21 PM UTC, comment #6: 

But the result is wrong when %..% doesn't specify a known variable, most probably because we invoke the command through a batch file.

Try this from cmd.exe prompt:

 > echo %foo%

You will see %foo%.

Now try with make:

  D:\usr\eli>\gnu\make-4.2.90-guile\GccRel\gnumake -f-
  SHELL=cmd.exe

  all:
          @echo %foo%
  ^Z
  ECHO is off.

In other words, 'echo' was invoked with an empty string, which is not what we want.

Eli Zaretskii <eliz>
Group Member
Wed 28 Aug 2019 07:11:11 PM UTC, comment #5: 

Sorry, I think there's some confusion.  All this patch does is detect if we see a % in the command line and if so we do not take the fast path: instead we take the slow path and invoke command.com.

So we're not trying to handle environment variables etc., we're just saying "hey make, if the recipe contains % then give up and pass it to command.com to take care of".

Paul D. Smith <psmith>
Group administrator
Wed 28 Aug 2019 07:06:47 PM UTC, comment #4: 

It is true that %Path% on a command line should expand to the value of Path in the environment.  It is also true that to have this expansion, we need to invoke the command through the shell.  However, this is only true for any arbitrary %FOO% where an environment variable FOO is defined.  If FOO is not defined, then %FOO% is expanded to just %FOO%, i.e. is not changed at all.  In particular, %% is left alone by the shell.

So to support %FOO% expansion correctly, we need to figure out whether FOO is defined or not.  IOW, we need to call 'getenv', at least.  And even that might not be 100% accurate, since the command could modify the environment as part of itself (but maybe we should ignore this possibility, even if it exists).

Eli Zaretskii <eliz>
Group Member
Wed 28 Aug 2019 06:47:13 PM UTC, comment #3: 

I wasn't thinking about % as an escape character so much as a variable introduction character.

But I keep forgetting that variable expansion in Windows works very differently than in POSIX.

I was assuming that:


all: ; argv1 %Path%


would need to use command.com to ensure that %Path% was expanded.  But maybe that's not correct on Windows.

Paul D. Smith <psmith>
Group administrator
Wed 28 Aug 2019 04:20:23 PM UTC, comment #2: 

I see that this bug was fixed and closed, but looking at the result, I'm not sure the fix is correct.  If I invoke a Windows program from the cmd.exe prompt with %% as argument, the program gets "%%" as its argv[1], not just a single %.

So I guess I don't understand why the OP expected to get a single % character.  Am I missing something?

Eli Zaretskii <eliz>
Group Member
Mon 26 Aug 2019 12:29:01 PM UTC, comment #1: 

Thanks for the bug report!

Paul D. Smith <psmith>
Group administrator
Thu 06 Jun 2019 11:13:33 AM UTC, original submission:  

The slow path must be taken if a shell escape character ('%') is present in the command line. Otherwise escaped characters on the command line will not be properly unescaped by the shell.


Demo:
======

argv1.c:
--------%<----------------------
#include <stdio.h>

int main(int argc, char *argv[])
{
        puts(argv[1]);
        return 0;
}
-------->%----------------------

> gcc -Wall -o argv1.exe argv1.c


Makefile:
--------%<----------------------
$ cat Makefile
.PHONY: all

SHELL = cmd.exe

all:
        @argv1 %%
        @argv1 "%%"
        @argv1 %% > con

-------->%----------------------

* Please run mingw32-make from cmd.exe, not from Msys shell *

> mingw32-make.exe

%%
%%
%


Expected result:
%
%
%

Result after applying attached patch:
%
%
%


Anonymous

 

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

Attach Files:
   
   
Comment:
   

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 ceggers01 (Posted a comment)
  • -email is unavailable- added by ceggers (Posted a comment)
  • -email is unavailable- added by eliz (Posted a comment)
  • -email is unavailable- added by psmith (Posted a comment)
  •  

    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.

    Only logged-in users can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-10-08 psmith Fixed Release4.3 None
    2019-09-10 eliz Severity3 - Normal 1 - Wish
        StatusFixed Wont Fix
        Open/ClosedOpen Closed
        Fixed ReleaseNone 4.3
    2019-08-28 eliz Open/ClosedClosed Open
    2019-08-26 psmith StatusNone Fixed
        Open/ClosedOpen Closed
    2019-06-06 None Attached File- Added 0001-src-job.c-construct_command_argv_internal-Use-shell-.patch, #47045

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code