[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Bug binutils/27594] build processes broken by changed space handling
From: |
martin at martin dot st |
Subject: |
[Bug binutils/27594] build processes broken by changed space handling |
Date: |
Mon, 10 May 2021 16:47:32 +0000 |
https://sourceware.org/bugzilla/show_bug.cgi?id=27594
--- Comment #11 from Martin Storsjö <martin at martin dot st> ---
(In reply to Eli Zaretskii from comment #10)
> > So if I want to call windres so that it receives the argument "path
> > to/gcc.exe" -E -DRC_INVOKED, I can call windres with "\"path to/gcc.exe\" -E
> > -DRC_INVOKED" in cmd.exe and in bash - that particular quoting happens to
> > work in both. In bash I could also call it with single quotes and not
> > needing to escape the double quotes, as '"path to/gcc.exe" -E -DRC_INVOKED'.
>
> Here is where I disagree: the "\"path to/gcc.exe\" or '"path to/gcc.exe"' is
> NOT quoting, it's double quoting. Quoting is just "path to/gcc.exe" or (for
> Posix shells) 'path to/gcc.exe'.
Yes, that's true: We need to make sure windres receives a pre-quoted string,
thus the caller has to double quote it.
> > The quot() function isn't called for --preprocessor
> > arguments, it's only called for --preprocessor-arg and -D and similar. It
> > still just wraps the whole --preprocessor argument in double quotes, here:
> > https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/windres.c;
> > h=2b626a29fb68ada1cc012a267a60d72c1335bea9;
> > hb=23182ac0d832477d316547ec2a758d22b43d0837#l889
>
> Yes, and thus I don't understand why the change to `quot` was reverted. If
> that function's job is to quote something, then it should quote correctly on
> MS-Windows, and the only sensible quoting on MS-Windows is "like this". If
> someone wants to argue that arguments with whitespace should not be quoted,
> then why do we still call `quot` on Unix?
Indeed, I think that was a mistake in the revert. I'll try to follow up with a
patch for reverting the right thing in a couple hours, and for clarifying the
documentation regarding it - to codify the existing status quo regarding it.
> > But the core issue - that the --preprocessor argument used to accept
> > multiple arguments (and where spaces would have to be pre-quoted) - do you
> > want to willingly break that, which multiple existing projects rely on?
>
> You just said that the change in `quot` doesn't affect --preprocessor, so
> how can the change in `quot` break that (incorrect) usage of --preprocessor?
> Why was it reverted? What am I missing here?
Yeah the change in quot doesn't affect --preprocessor indeed. The "you" in this
section was more of a generic towards the project, not you (singular) in
particular.
> > Eli - separately, the new quoting behaviour (that was reverted now,
> > needlessly?) is, in principle a good thing, but it's still missing to quote
> > a few tricky cases - namely backslashes and double quotes. And fixing that
> > would be breaking roughly a similar number of projects.
>
> The code does attempt to quote double quotes, but if that code is buggy,
> please show the specific use case where it breaks. If the code is
> incomplete, let's fix that, by all means.
>
> As for backslashes: they don't need to be quoted on native Windows, only if
> you are using a Posix shell, in which case the user should have typed the
> original file name or string argument with backslashes already doubled,
> right?
>
> > If I want to preprocess with the option e.g. -DSTRING="quotedstring", I want
> > windres to call popen with -DSTRING=\"quotedstring\", so I need to call
> > windres with -DSTRING=\\\"quotedstring\\\".
>
> I don't think I agree. The user should say -DSTRING="quotedstring" and the
> code which invokes `popen` should then take care of quoting any arguments
> that have embedded whitespace. Otherwise, we will have to know exactly how
> many "unquoting" levels will the string undergo, and add quotes for each
> level. That way lies madness, because it is not practical to do that -- you
> never know how many such levels will the string undergo. The only sane way
> is for the code which is going to pass a string to a shell command to quote
> it if needed, and as appropriate to the command to be invoked.
>
> > So it would in theory be great
> > if quot() would handle proper escaping of backslashes and double quotes, but
> > if we'd do that change, we'd be breaking those existing users that have
> > codified that case of double escaping for such strings.
>
> I still want to see those existing users which we will break. I suspect
> that either they are already broken (and use loopholes due to bugs in the
> implementation), or will simply be unaffected.
>
> So by all means, let's talk about those use cases one by one, and see what
> we want to do about them.
Yes I agree that the most sane thing to do would be to have the user specify
-DSTRING="quotedstring", but there are projects that have ended up working
around this, so fixing this case does break them.
I'll follow up with a separate bug for that topic in a couple hours, with more
examples and details, to avoid derailing this one further.
--
You are receiving this mail because:
You are on the CC list for the bug.
- [Bug binutils/27594] build processes broken by changed space handling, towo at towo dot net, 2021/05/10
- [Bug binutils/27594] build processes broken by changed space handling, cvs-commit at gcc dot gnu.org, 2021/05/10
- [Bug binutils/27594] build processes broken by changed space handling, eliz at gnu dot org, 2021/05/10
- [Bug binutils/27594] build processes broken by changed space handling, martin at martin dot st, 2021/05/10
- [Bug binutils/27594] build processes broken by changed space handling, eliz at gnu dot org, 2021/05/10
- [Bug binutils/27594] build processes broken by changed space handling,
martin at martin dot st <=
- [Bug binutils/27594] build processes broken by changed space handling, eliz at gnu dot org, 2021/05/10
- [Bug binutils/27594] build processes broken by changed space handling, eliz at gnu dot org, 2021/05/10
- [Bug binutils/27594] build processes broken by changed space handling, eliz at gnu dot org, 2021/05/10
- [Bug binutils/27594] build processes broken by changed space handling, martin at martin dot st, 2021/05/10
- [Bug binutils/27594] build processes broken by changed space handling, eliz at gnu dot org, 2021/05/10
- [Bug binutils/27594] build processes broken by changed space handling, towo at towo dot net, 2021/05/10
- [Bug binutils/27594] build processes broken by changed space handling, eliz at gnu dot org, 2021/05/11
- [Bug binutils/27594] build processes broken by changed space handling, martin at martin dot st, 2021/05/11
- [Bug binutils/27594] build processes broken by changed space handling, eliz at gnu dot org, 2021/05/11
- [Bug binutils/27594] build processes broken by changed space handling, martin at martin dot st, 2021/05/11