[Bug binutils/27594] build processes broken by changed space handling

[martin at martin dot st]

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.