[PATCH] windres: add quotes around preprocessor cmd if needed

[PATCH] windres: add quotes around preprocessor cmd if needed

[Clément Chigot]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

This patch ensures that the gcc binary called by windres is quoted if
needed. Otherwise, errors can occur if the gcc is under a folder having
a name containing a space (eg "Program Files").

binutils/
        * resrc.c (DEFAULT_PREPROCESSOR): Split into...
        (DEFAULT_PREPROCESSOR_CMD): that...
        (DEFAULT_PREPROCESSOR_ARGS): and that.
        (look_for_default): Add quotes around the command if needed.
        (read_rc_file): Adapt to new defines.


Thanks,
Clément

[-- Attachment #2: 0001-windres-add-quotes-around-preprocessor-cmd-if-needed.patch --]
[-- Type: text/x-patch, Size: 2870 bytes --]

From 3c6afcd33eed6806df0e632b1d52e946d277cc89 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Chigot?= <chigot@adacore.com>
Date: Thu, 16 Jun 2022 13:43:26 +0200
Subject: [PATCH] windres: add quotes around preprocessor cmd if needed

This patch ensures that the gcc binary called by windres is quoted if
needed. Otherwise, errors can occur if the gcc is under a folder having
a name containing a space (eg "Program Files").

binutils/
	* resrc.c (DEFAULT_PREPROCESSOR): Split into...
	(DEFAULT_PREPROCESSOR_CMD): that...
	(DEFAULT_PREPROCESSOR_ARGS): and that.
	(look_for_default): Add quotes around the command if needed.
	(read_rc_file): Adapt to new defines.

---
 binutils/resrc.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/binutils/resrc.c b/binutils/resrc.c
index 249eb9a87ed..de39e133a0a 100644
--- a/binutils/resrc.c
+++ b/binutils/resrc.c
@@ -75,7 +75,8 @@
 
 /* The default preprocessor.  */
 
-#define DEFAULT_PREPROCESSOR "gcc -E -xc -DRC_INVOKED"
+#define DEFAULT_PREPROCESSOR_CMD "gcc"
+#define DEFAULT_PREPROCESSOR_ARGS "-E -xc -DRC_INVOKED"
 
 /* We read the directory entries in a cursor or icon file into
    instances of this structure.  */
@@ -378,17 +379,13 @@ static FILE *
 look_for_default (char *cmd, const char *prefix, int end_prefix,
 		  const char *preprocargs, const char *filename)
 {
-  char *space;
   int found;
   struct stat s;
   const char *fnquotes = (filename_need_quotes (filename) ? "\"" : "");
 
   strcpy (cmd, prefix);
 
-  sprintf (cmd + end_prefix, "%s", DEFAULT_PREPROCESSOR);
-  space = strchr (cmd + end_prefix, ' ');
-  if (space)
-    *space = 0;
+  sprintf (cmd + end_prefix, "%s", DEFAULT_PREPROCESSOR_CMD);
 
   if (
 #if defined (__DJGPP__) || defined (__CYGWIN__) || defined (_WIN32)
@@ -410,10 +407,16 @@ look_for_default (char *cmd, const char *prefix, int end_prefix,
 	}
     }
 
-  strcpy (cmd, prefix);
+  if (filename_need_quotes (cmd))
+    {
+      char *cmd_copy = xmalloc (strlen (cmd));
+      strcpy (cmd_copy, cmd);
+      sprintf (cmd, "\"%s\"", cmd_copy);
+      free (cmd_copy);
+    }
 
-  sprintf (cmd + end_prefix, "%s %s %s%s%s",
-	   DEFAULT_PREPROCESSOR, preprocargs, fnquotes, filename, fnquotes);
+  sprintf (cmd + strlen (cmd), " %s %s %s%s%s",
+	   DEFAULT_PREPROCESSOR_ARGS, preprocargs, fnquotes, filename, fnquotes);
 
   if (verbose)
     fprintf (stderr, _("Using `%s'\n"), cmd);
@@ -490,10 +493,9 @@ read_rc_file (const char *filename, const char *preprocessor,
     {
       char *dash, *slash, *cp;
 
-      preprocessor = DEFAULT_PREPROCESSOR;
-
       cmd = xmalloc (strlen (program_name)
-		     + strlen (preprocessor)
+		     + strlen (DEFAULT_PREPROCESSOR_CMD)
+		     + strlen (DEFAULT_PREPROCESSOR_ARGS)
 		     + strlen (preprocargs)
 		     + strlen (filename)
 		     + strlen (fnquotes) * 2
-- 
2.25.1

Re: [PATCH] windres: add quotes around preprocessor cmd if needed

[Jan Beulich]

On 28.06.2022 09:35, Clément Chigot via Binutils wrote:
> This patch ensures that the gcc binary called by windres is quoted if
> needed. Otherwise, errors can occur if the gcc is under a folder having
> a name containing a space (eg "Program Files").
> 
> binutils/
>         * resrc.c (DEFAULT_PREPROCESSOR): Split into...
>         (DEFAULT_PREPROCESSOR_CMD): that...
>         (DEFAULT_PREPROCESSOR_ARGS): and that.
>         (look_for_default): Add quotes around the command if needed.
>         (read_rc_file): Adapt to new defines.

Commenting is a little difficult without you providing the patch (also)
inline: Shouldn't you also (optionally) quote the pre-processor string
if that came into read_rc_file() as non-NULL? Everything else looks
okay to me.

Jan

Re: [PATCH] windres: add quotes around preprocessor cmd if needed

[Clément Chigot]

On Tue, Jun 28, 2022 at 11:00 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.06.2022 09:35, Clément Chigot via Binutils wrote:
> > This patch ensures that the gcc binary called by windres is quoted if
> > needed. Otherwise, errors can occur if the gcc is under a folder having
> > a name containing a space (eg "Program Files").
> >
> > binutils/
> >         * resrc.c (DEFAULT_PREPROCESSOR): Split into...
> >         (DEFAULT_PREPROCESSOR_CMD): that...
> >         (DEFAULT_PREPROCESSOR_ARGS): and that.
> >         (look_for_default): Add quotes around the command if needed.
> >         (read_rc_file): Adapt to new defines.
>
> Commenting is a little difficult without you providing the patch (also)
> inline:

IIRC, it should be done directly but the "é" in my name makes the
mailservers think my patches are binaries...

Shouldn't you also (optionally) quote the pre-processor string
> if that came into read_rc_file() as non-NULL? Everything else looks
> okay to me.

Yeah. I didn't run into this issue but looking at the code it should happen
too. Thanks for pointing it out !

Clément

Re: [PATCH] windres: add quotes around preprocessor cmd if needed

[Jan Beulich]

On 28.06.2022 11:37, Clément Chigot wrote:
> On Tue, Jun 28, 2022 at 11:00 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 28.06.2022 09:35, Clément Chigot via Binutils wrote:
>>> This patch ensures that the gcc binary called by windres is quoted if
>>> needed. Otherwise, errors can occur if the gcc is under a folder having
>>> a name containing a space (eg "Program Files").
>>>
>>> binutils/
>>>         * resrc.c (DEFAULT_PREPROCESSOR): Split into...
>>>         (DEFAULT_PREPROCESSOR_CMD): that...
>>>         (DEFAULT_PREPROCESSOR_ARGS): and that.
>>>         (look_for_default): Add quotes around the command if needed.
>>>         (read_rc_file): Adapt to new defines.
>>
>> Commenting is a little difficult without you providing the patch (also)
>> inline:
> 
> IIRC, it should be done directly but the "é" in my name makes the
> mailservers think my patches are binaries...

I don't think that's what is the issue. There may be mail clients which
display text attachments, but then it's still an attachment, and other
clients may only ever handle attachments as such (mine, for example,
might display such an attachment, but would not consider it part of the
reply context when first setting up a reply mail, yet having it there
is what is the primary goal when talking about commenting on patches).
If you can't configure your client such that inlined patches make it
through ungarbled, it would still be helpful if you could _also_ inline
the patch, indicating that it may not be well-formed and hence for
applying the attached variant should be used.

Jan

Re: [PATCH] windres: add quotes around preprocessor cmd if needed

[Clément Chigot]

On Tue, Jun 28, 2022 at 12:16 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.06.2022 11:37, Clément Chigot wrote:
> > On Tue, Jun 28, 2022 at 11:00 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 28.06.2022 09:35, Clément Chigot via Binutils wrote:
> >>> This patch ensures that the gcc binary called by windres is quoted if
> >>> needed. Otherwise, errors can occur if the gcc is under a folder having
> >>> a name containing a space (eg "Program Files").
> >>>
> >>> binutils/
> >>>         * resrc.c (DEFAULT_PREPROCESSOR): Split into...
> >>>         (DEFAULT_PREPROCESSOR_CMD): that...
> >>>         (DEFAULT_PREPROCESSOR_ARGS): and that.
> >>>         (look_for_default): Add quotes around the command if needed.
> >>>         (read_rc_file): Adapt to new defines.
> >>
> >> Commenting is a little difficult without you providing the patch (also)
> >> inline:
> >
> > IIRC, it should be done directly but the "é" in my name makes the
> > mailservers think my patches are binaries...
>
> I don't think that's what is the issue. There may be mail clients which
> display text attachments, but then it's still an attachment, and other
> clients may only ever handle attachments as such (mine, for example,
> might display such an attachment, but would not consider it part of the
> reply context when first setting up a reply mail, yet having it there
> is what is the primary goal when talking about commenting on patches).
> If you can't configure your client such that inlined patches make it
> through ungarbled, it would still be helpful if you could _also_ inline
> the patch, indicating that it may not be well-formed and hence for
> applying the attached variant should be used.

Alright. Thanks for the inputs.
I'm planning to setup to another mailer for GNU mailing lists and
"git sendemail" too. So it should fix that.
Meanwhile I'll try to remember to inline them manually.

Thanks anyway.
Clément

Re: [PATCH] windres: add quotes around preprocessor cmd if needed

[Clément Chigot]

On Tue, Jun 28, 2022 at 11:37 AM Clément Chigot <chigot@adacore.com> wrote:
>
> On Tue, Jun 28, 2022 at 11:00 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 28.06.2022 09:35, Clément Chigot via Binutils wrote:
> > Shouldn't you also (optionally) quote the pre-processor string
> > if that came into read_rc_file() as non-NULL? Everything else looks
> > okay to me.
>
> Yeah. I didn't run into this issue but looking at the code it should happen
> too. Thanks for pointing it out !

Actually the space issue is already handled by windres.c:
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/windres.c;h=4df821ee8451bda70490d6626707c2bdd66eeb5f;hb=HEAD#l887
It's not as complete as it should be. But apart of "%" I'm not sure
any other characters caught by "filename_need_quotes" can happen
in a folder name under Windows.
Making "filename_need_quotes" more global might be a good idea
anyway.

Clément

Re: [PATCH] windres: add quotes around preprocessor cmd if needed

[Jan Beulich]

On 28.06.2022 14:26, Clément Chigot wrote:
> On Tue, Jun 28, 2022 at 11:37 AM Clément Chigot <chigot@adacore.com> wrote:
>>
>> On Tue, Jun 28, 2022 at 11:00 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 28.06.2022 09:35, Clément Chigot via Binutils wrote:
>>> Shouldn't you also (optionally) quote the pre-processor string
>>> if that came into read_rc_file() as non-NULL? Everything else looks
>>> okay to me.
>>
>> Yeah. I didn't run into this issue but looking at the code it should happen
>> too. Thanks for pointing it out !
> 
> Actually the space issue is already handled by windres.c:
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/windres.c;h=4df821ee8451bda70490d6626707c2bdd66eeb5f;hb=HEAD#l887
> It's not as complete as it should be. But apart of "%" I'm not sure
> any other characters caught by "filename_need_quotes" can happen
> in a folder name under Windows.

Oh, I see. I'm not sure I see a strong need for dealing with %, so
I'm wondering: Are you intending to make an updated patch, or do
you want to commit the one you've got (which I'm okay with given
your observation)?

> Making "filename_need_quotes" more global might be a good idea
> anyway.

Right, but that could be a separate change then.

Jan

Re: [PATCH] windres: add quotes around preprocessor cmd if needed

[Clément Chigot]

On Tue, Jun 28, 2022 at 3:52 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.06.2022 14:26, Clément Chigot wrote:
> > On Tue, Jun 28, 2022 at 11:37 AM Clément Chigot <chigot@adacore.com> wrote:
> >>
> >> On Tue, Jun 28, 2022 at 11:00 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>
> >>> On 28.06.2022 09:35, Clément Chigot via Binutils wrote:
> >>> Shouldn't you also (optionally) quote the pre-processor string
> >>> if that came into read_rc_file() as non-NULL? Everything else looks
> >>> okay to me.
> >>
> >> Yeah. I didn't run into this issue but looking at the code it should happen
> >> too. Thanks for pointing it out !
> >
> > Actually the space issue is already handled by windres.c:
> > https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/windres.c;h=4df821ee8451bda70490d6626707c2bdd66eeb5f;hb=HEAD#l887
> > It's not as complete as it should be. But apart of "%" I'm not sure
> > any other characters caught by "filename_need_quotes" can happen
> > in a folder name under Windows.
>
> Oh, I see. I'm not sure I see a strong need for dealing with %, so
> I'm wondering: Are you intending to make an updated patch, or do
> you want to commit the one you've got (which I'm okay with given
> your observation)?

Honestly, if this patch fits you, I would merge it as is. This issue is already
a corner case, but as it's hidden inside windres code and because of
the existence of the "Program Files" folder can be problematic.
The other has to be triggered by someone explicitly passing "--preprocessor"
with an unusual path. So honestly I don't think it would ever happen.

> > Making "filename_need_quotes" more global might be a good idea
> > anyway.
>
> Right, but that could be a separate change then.

Agreed.

Re: [PATCH] windres: add quotes around preprocessor cmd if needed

[Jan Beulich]

On 28.06.2022 16:18, Clément Chigot wrote:
> On Tue, Jun 28, 2022 at 3:52 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 28.06.2022 14:26, Clément Chigot wrote:
>>> On Tue, Jun 28, 2022 at 11:37 AM Clément Chigot <chigot@adacore.com> wrote:
>>>>
>>>> On Tue, Jun 28, 2022 at 11:00 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>
>>>>> On 28.06.2022 09:35, Clément Chigot via Binutils wrote:
>>>>> Shouldn't you also (optionally) quote the pre-processor string
>>>>> if that came into read_rc_file() as non-NULL? Everything else looks
>>>>> okay to me.
>>>>
>>>> Yeah. I didn't run into this issue but looking at the code it should happen
>>>> too. Thanks for pointing it out !
>>>
>>> Actually the space issue is already handled by windres.c:
>>> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/windres.c;h=4df821ee8451bda70490d6626707c2bdd66eeb5f;hb=HEAD#l887
>>> It's not as complete as it should be. But apart of "%" I'm not sure
>>> any other characters caught by "filename_need_quotes" can happen
>>> in a folder name under Windows.
>>
>> Oh, I see. I'm not sure I see a strong need for dealing with %, so
>> I'm wondering: Are you intending to make an updated patch, or do
>> you want to commit the one you've got (which I'm okay with given
>> your observation)?
> 
> Honestly, if this patch fits you, I would merge it as is.

Feel free to commit it then.

Jan

> This issue is already
> a corner case, but as it's hidden inside windres code and because of
> the existence of the "Program Files" folder can be problematic.
> The other has to be triggered by someone explicitly passing "--preprocessor"
> with an unusual path. So honestly I don't think it would ever happen.
> 
>>> Making "filename_need_quotes" more global might be a good idea
>>> anyway.
>>
>> Right, but that could be a separate change then.
> 
> Agreed.

Re: [PATCH] windres: add quotes around preprocessor cmd if needed

[Clément Chigot]

On Tue, Jun 28, 2022 at 4:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.06.2022 16:18, Clément Chigot wrote:
> > On Tue, Jun 28, 2022 at 3:52 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 28.06.2022 14:26, Clément Chigot wrote:
> >>> On Tue, Jun 28, 2022 at 11:37 AM Clément Chigot <chigot@adacore.com> wrote:
> >>>>
> >>>> On Tue, Jun 28, 2022 at 11:00 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>
> >>>>> On 28.06.2022 09:35, Clément Chigot via Binutils wrote:
> >>>>> Shouldn't you also (optionally) quote the pre-processor string
> >>>>> if that came into read_rc_file() as non-NULL? Everything else looks
> >>>>> okay to me.
> >>>>
> >>>> Yeah. I didn't run into this issue but looking at the code it should happen
> >>>> too. Thanks for pointing it out !
> >>>
> >>> Actually the space issue is already handled by windres.c:
> >>> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/windres.c;h=4df821ee8451bda70490d6626707c2bdd66eeb5f;hb=HEAD#l887
> >>> It's not as complete as it should be. But apart of "%" I'm not sure
> >>> any other characters caught by "filename_need_quotes" can happen
> >>> in a folder name under Windows.
> >>
> >> Oh, I see. I'm not sure I see a strong need for dealing with %, so
> >> I'm wondering: Are you intending to make an updated patch, or do
> >> you want to commit the one you've got (which I'm okay with given
> >> your observation)?
> >
> > Honestly, if this patch fits you, I would merge it as is.
>
> Feel free to commit it then.

Committed. Thanks for the review.

Clément