Bug 4356

Summary: windres fails on files with spaces
Product: binutils Reporter: Hendrik Sattler <sattler>
Component: binutilsAssignee: unassigned
Status: REOPENED ---    
Severity: critical CC: bug-binutils, david.macek.0, denpashogai, eliz, gusfl, kai.koehne, nickc, spring, tonyt
Priority: P2    
Version: 2.16   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed:
Attachments: Put filenames from command line in quotes to handle spaces correctly
Patch to quote file names for Windows shells

Description Hendrik Sattler 2007-04-13 15:51:38 UTC
Hi,

Windres is very limited on common windows systems because it cannot handle 
pathes with spaces.
Hint: the default homedir contains spaces

This does not seem to be fixed in latest CVS because the error is most 
obviously in binutils/resrc.c (and similar) in function read_rc_file() and 
look_for_default():
sprintf (cmd, "%s %s %s", preprocessor, preprocargs, filename);

The filename argument MUST be surrounded by "", thus this line should read:
sprintf (cmd, "%s %s \"%s\"", preprocessor, preprocargs, filename);

Easy fix, I'd say. Reproducable on command line using DEFAULT_PREPROCESSOR.

HS
Comment 1 Nick Clifton 2007-04-20 14:24:14 UTC
Subject: Re:  New: windres fails on files with spaces

Hi HS,

> This does not seem to be fixed in latest CVS because the error is most 
> obviously in binutils/resrc.c (and similar) in function read_rc_file() and 
> look_for_default():
> sprintf (cmd, "%s %s %s", preprocessor, preprocargs, filename);
> 
> The filename argument MUST be surrounded by "", thus this line should read:
> sprintf (cmd, "%s %s \"%s\"", preprocessor, preprocargs, filename);
> 
> Easy fix, I'd say.

Would you like to submit a patch that does this then ?

> Reproducable on command line using DEFAULT_PREPROCESSOR.

Please could you provide an example of such a command line ?

Cheers
   Nick


Comment 2 Hendrik Sattler 2007-04-20 16:31:24 UTC
Created attachment 1730 [details]
Put filenames from command line in quotes to handle spaces correctly

> Please could you provide an example of such a command line?

Just specify a full path with spaces in it:
C:\>windres "C:\test with spaces\gui.rc" "c:\test with spaces"\gui.obj
gcc: C:\test: No such file or directory
gcc: with: No such file or directory
gcc: spaces\gui.rc: No such file or directory
gcc: warning: `-x c' after last input file has no effect
gcc: no input files
windres: no resources

Same for the parameter -I. The patch fixes those issue.

NOTE:
This is still broken for file names that use special shell characters.
This is because you use popen() instead of a combination of pipe(), fork(), a
flavour of exec() and fdopen().
The latter one does not have any of those issues because the shell is avoided
(hey, it will even be faster).
So, see this patch as hotfix for common usage. A bit more work is needed to
allow shell specific characters in file name.

HS

PS: popen() is an _evil_ command!
Comment 3 Nick Clifton 2007-06-06 08:07:03 UTC
Hi Hendrik,

  This problem should now be resolved.  A recent patch to resrc.c should ensure
that all special case filenames are quoted when they are used on generated
command lines.

Cheers
  Nick
Comment 4 Oisín 2011-02-28 18:31:10 UTC
It does not seem to be fixed in 2.21; calling windres with the following:

--include-dir "C:\Program Files\Lua\5.1\include"

Produces:

"gcc: Files\Lua\5.1\include: No such file or directory
windres: gcc exited with status 1"

Using single quotes instead of double quotes causes a different error; I'm not sure if they have another special meaning.

I can work around it with:

--include-dir "C:\Progra~1\Lua\5.1\include"

But it'd be better if it worked the normal way.
Comment 5 Hendrik Sattler 2011-03-01 07:56:18 UTC
The fix is equally simple, though, this time in windres.c:
-I%s  -> -I\"%s\"
Comment 6 Hendrik Sattler 2011-03-01 07:57:34 UTC
The include options are also affected, reopening bug...
Comment 7 spring 2014-07-06 21:17:49 UTC
i hit this bug, too...

GNU windres (GNU Binutils) 2.23.1
Comment 8 spring 2014-07-06 21:21:19 UTC
> "C:\Program Files (x86)\CODEBL~1\MinGW\bin\windres.exe"  -O coff D:/spring/rts/icon.rc rts/builds/legacy/CMakeFiles/engine-legacy.dir/__/__/icon.rc.obj
Der Befehl "C:\Program" ist entweder falsch geschrieben oder
konnte nicht gefunden werden.
C:\Program Files (x86)\CODEBL~1\MinGW\bin\windres.exe: preprocessing failed.
Comment 9 gusfl 2015-03-15 12:03:22 UTC
windres -I "C:\Program Files\MinGW\include" "manifest.rc" manifest.o

-> "Files\MinGW\include" : directory not found
Comment 10 Kai Koehne 2020-07-23 05:41:47 UTC
At least the -I escaping issue is still valid in binutils 2.34:

 windres -I"C:/Program Files/OpenSSL-Win64/include"
 gcc: error: Files/OpenSSL-Win64/include: No such file or directory

A workaround is to explicitly mask things twice

  windres -I\""C:/Program Files/OpenSSL-Win64/include"\"
Comment 11 Eli Zaretskii 2021-01-26 19:22:32 UTC
I think that the solutions proposed for this bug are flawed, because they paper over the real problem.  The real problem is the function 'quot': it is there to quote special characters that can confuse the shell, but the method it uses to quote -- escape-protect each special character with a backslash -- works only for Posix shells.  For Windows shells we need to quote "like this", i.e. enclose the whole string in double quotes (and escape-protect any embedded quotes with a backslash).

The attached patch modifies 'quot' to DTRT on MS-Windows.  I tested it, and it works in all the test cases I threw on it.
Comment 12 Eli Zaretskii 2021-01-26 19:24:46 UTC
Created attachment 13160 [details]
Patch to quote file names for Windows shells
Comment 13 Sourceware Commits 2021-01-28 14:58:28 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=cc3edc52747fd8b184ee48f1b0cc1ac0aca7832e

commit cc3edc52747fd8b184ee48f1b0cc1ac0aca7832e
Author: Eli Zaretskii <eliz@gnu.org>
Date:   Thu Jan 28 14:57:33 2021 +0000

    Improve windres's handling of pathnames containing special characters on Windows platforms.
    
     PR 4356
     * windres.c (quot): Use double quotes to protect strings on
     Windows platforms.
Comment 14 Nick Clifton 2021-01-28 14:59:08 UTC
Hi Eli,

  Thanks for the patch.  I have gone ahead and applied it.

Cheers
  Nick
Comment 15 David Macek 2021-03-28 12:21:20 UTC
It seems the latest patch has broken this unsupported use case that had worked on Win32:

    windres "--preprocessor=gcc -E" --use-temp-file [...]

If I don't enable temp files or don't specify the preprocessor, it works.

See <https://gitlab.haskell.org/ghc/ghc/-/issues/16780#note_340986> for an example of such usage.  Given GHC corrected its usage to one that seems more correct, I'd say there's no need for an action here, but at least this note could help someone having the same issue.
Comment 16 Nick Clifton 2021-04-01 14:13:50 UTC
(In reply to David Macek from comment #15)
Hi David,

>     windres "--preprocessor=gcc -E" --use-temp-file [...]

Does this alternative command line work ?

  windres --preprocessor=gcc --preprocessor-arg=-E --use-temp-file [...]

Cheers
  Nick
Comment 17 Nick Clifton 2021-04-01 14:46:15 UTC
Just to be clear, the problem is that windres is trying to run "gcc -E" as if that was the full name of the preprocessor, and not a program name plus argument.  This happens because internally windres puts double quotes around "gcc -E", preventing it from being split up into program+arguments.
Comment 18 Eli Zaretskii 2021-04-01 15:00:32 UTC
I don't think I understand the problem being reported by David Macek.

First, the fix alluded to there changed how the function 'quot' quotes file names. The problematic use case is with the --preprocessor= option, but that option doesn't use 'quote' in the sources I have. Moreover, the documentation says that --preprocessor= accepts the PROGRAM, which means no command-line arguments; the arguments are supposed to come via the companion option --preprocessor-arg=.

So how exactly could the 'quot' change break some use case that worked before, and how could that use case work in the original windres before the change?  Are we talking about the stock windres or about some patched version of windres?
Comment 19 Eli Zaretskii 2021-04-01 15:12:26 UTC
Looking at the linked Huskell issue, I think the problem is not with the --preprocessor= option, it is with --preprocessor-arg= option, where 'quot' IS used.

The problem is that windres.c quotes the argument if it has whitespace, but then 'read_rc_file' quotes it again.  This double quoting is what causes the problem.  The problem was exposed by the fix in 'quot': previously it basically didn't quote the argument, so the extra quoting in 'read_rc_file' didn't do any harm.

So I think removing the fnquotes thingy from 'read_rc_file' should fix the problem.  Please test.
Comment 20 Nick Clifton 2021-04-01 15:50:50 UTC
(In reply to Eli Zaretskii from comment #19)
Hi Eli,

> So I think removing the fnquotes thingy from 'read_rc_file' should fix the
> problem.  Please test.

But... the fnquotes thingy is used to quote the filename, not the preprocessor args:

      sprintf (cmd, "%s %s %s%s%s", preprocessor, preprocargs,
	       fnquotes, filename, fnquotes);
Comment 21 Eli Zaretskii 2021-04-01 18:37:39 UTC
Oops, you are right.  Sorry for misreading the code.

Then I guess I'm back where I started: I don't understand the cause of the problem.  The original issue seems to involve Python, maybe it's related.
Comment 22 David Macek 2021-04-02 09:31:10 UTC
I guess I'm confused as well.

The specific comment I linked said:

> We should probably use --preprocessor-arg instead to pass arguments.

Until we realized that, there were discussions on how to patch windres, but those don't apply anymore AFAICT.

If there is something to be fixed, it might be the inconsistency between `--use-temp-file` and `--no-use-temp-file`.  Apparently without a temp file, windres still interprets a multiple word `--preprocessor` value as a command and arguments.  (Assuming this is not actually just a quirk of our windres build.)
Comment 23 Eli Zaretskii 2021-04-02 12:03:07 UTC
If you can show an example of a windres command that works without temp files but fails with them (or the other way around), I will try to take a look.  But please show an example that doesn't involve any additional programs nor Python, just an example that is invoked from the cmd prompt.

Thanks.
Comment 24 David Macek 2021-04-04 08:15:23 UTC
Here you go.  Can you confirm this behavior?

$ cat a.rc
1 VERSIONINFO
FILEVERSION     1,0,0,0
BEGIN
END

$ ls a.res
ls: a.res: No such file or directory

$ windres -o a.res a.rc "--preprocessor=gcc -E -xc"

$ echo %ERRORLEVEL%
0

$ ls a.res
a.res

$ rm a.res

$ windres -o a.res a.rc "--preprocessor=gcc -E -xc" --use-temp-file
windres: CreateProcess (null): No such file or directory

$ ls a.res
ls: a.res: No such file or directory
Comment 25 Eli Zaretskii 2021-04-04 08:24:32 UTC
Thanks, but this is incorrect usage: --preprocessor= doesn't support command-line arguments.  The command-line arguments should be passed via --preprocessor-arg=.
Comment 26 David Macek 2021-04-04 10:52:58 UTC
Sorry for the confusion, I stand corrected.  I assumed the presented inconsistency, although in an incorrect usage, would translate into an inconsistency with correct usage.  I tried directing windres to something like "C:\path with spaces\gcc", but I wasn't able to break it regardless of temp files.

On the other hand, I was able to break windres by having it itself be in a path with spaces:

$ mklink /j "W:\mingw64 root" "C:\msys64\mingw64"
Junction created for W:\mingw64 root <<===>> C:\msys64\mingw64

$ set PATH=C:\windows\system32;W:\mingw64 root\bin

$ "W:\mingw64 root\bin\windres.exe" a.rc -o a.res --use-temp-file
W:\mingw64 root\bin\windres.exe: CreateProcess (null): No such file or directory

$ "W:\mingw64 root\bin\windres.exe" a.rc -o a.res
'W:\mingw64' is not recognized as an internal or external command,
operable program or batch file.
W:\mingw64 root\bin\windres.exe: preprocessing failed.

$ "W:\mingw64 root\bin\windres.exe" a.rc -o a.res "--preprocessor=W:\mingw64 root\bin\gcc" --preprocessor-arg=-E --preprocessor-arg=-xc
(works)

$ "W:\mingw64 root\bin\windres.exe" a.rc -o a.res "--preprocessor=W:\mingw64 root\bin\gcc" --preprocessor-arg=-E --preprocessor-arg=-xc --use-temp-file
(works)
Comment 27 Eli Zaretskii 2021-04-04 12:26:45 UTC
First, this is unrelated to the original issue.

Second, I cannot reproduce this with windres 2.36 that I built myself (which is the stock 2.36 version with the patch for 'quot' discussed in this bug report).  So my best guess is that there's something wrong with your windres binary, or maybe it has some local patches which cause that.
Comment 28 Eli Zaretskii 2021-04-04 18:35:11 UTC
FTR: I didn't use a junction to reproduce the problem, I actually created a directory with a space and copied there the MinGW installation.  So maybe the problem is somehow related to resolving links and junctions, I'm not sure.
Comment 29 Tony Theodore 2021-04-25 11:11:37 UTC
(In reply to Eli Zaretskii from comment #25)
> Thanks, but this is incorrect usage: --preprocessor= doesn't support
> command-line arguments.  The command-line arguments should be passed via
> --preprocessor-arg=.

May be a little off-topic, but the windres docs[1] state:
  "This option may be used to specify the preprocessor to use, including any leading arguments"

FFmpeg (probably similar to Haskell) use an invocation like:
  $(WINDRES) $(IFLAGS) --preprocessor "$(DEPWINDRES) -E -xc-header -DRC_INVOKED $(CC_DEPFLAGS)"

I imagine this has been the expected behaviour until a different whitespace fix recently[2].

Clarifying this change and the use of `--preprocessor-arg` going forward would be helpful.

Cheers,

Tony


[1] https://sourceware.org/binutils/docs/binutils/windres.html
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=26865
Comment 30 Sourceware Commits 2021-05-10 10:28:58 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=749c700282097cf679ff019a9674d7c762f48619

commit 749c700282097cf679ff019a9674d7c762f48619
Author: Thomas Wolff <towo@towo.net>
Date:   Mon May 10 11:28:15 2021 +0100

    Restore old behaviour of windres so that options containing spaces are not enclosed in double quotes.
    
            PR 4356
            PR 26865
            PR 27594
            * windres.c (quot): Revert previous delta.  Do not use double
            quotes when spaces are detected in options.
            * doc/binutils.texi (windres): Remove suggestion that the
            --preprocessor option can take arguments.
Comment 31 Eli Zaretskii 2021-05-10 13:48:24 UTC
Reverting the change in `quot` is IMO a mistake, I will respond in PR 27594 with the details.