Issue 1689
| Description: |
Opened: Tue Jul 10 07:06:00 -0800 2007 |
Sort by: Oldest first | Newest first |
As discussed on the ubuntu-devel list[1] on POSIX platforms without spawnvpe but
which do have the env utility SCons uses that together with os.system() to pass
the environment to child processes. When doing so it does fully not escape
special characters in the environment, causing build failures. This is
particularly common where SConstructs blindly pass the entire system environment
into the Environment.
The Ubunutu build daemons trigger this by defining an environment variable
HASH(0x12345678).
[1] https://lists.ubuntu.com/archives/ubuntu-devel/2007-July/023901.html
------- Additional comments from Greg Noel Thu Jun 5 15:34:55 -0800 2008 -------
Bug party triage: Need to find out exactly where this is taking place (command
execution, backtick, something else?) and start replacing these instances with
subprocess.
------- Additional comments from Mark Brown Fri Jun 6 01:22:09 -0800 2008 -------
This is caused by not quoting the environment when passing it to env in the
os.system() call - os.system() runs things using the shell so needs appropriate
quoting for that. The offending code is engine/SCons/Platform/posix.py.
I'm not sure how good the benchmark results were but it wasn't clear to me that
it was even worth trying to use this method if you have to do quoting since the
time taken to quote the strings may remove any benefit.
------- Additional comments from Gary Oberbrunner Fri Jun 6 03:33:10 -0800 2008 -------
From looking at the launchpad bug, it looks like env_spawn is only quoting the
VALUES, not the NAMES of the env vars. The bug comes from having an env var
with a name of "HASH(0x12345678)" (who even knew that was possible!)
------- Additional comments from Mark Brown Fri Jun 6 04:24:19 -0800 2008 -------
I rather suspect that more characters need to be escaped even in the values, but
ICBW.
In any case, this is something that systems support so SCons should handle it.
------- Additional comments from Gary Oberbrunner Fri Jun 6 05:17:58 -0800 2008 -------
Hi Mark; I was just looking at that and I'm pretty sure you're right. The
default escape() function in posix.py only backslash-escapes backslash,
double-quote, and dollar. I think that would be correct *if* the result were
going to be used in a double-quoted string, but it's not; it just gets passed to
the shell as is.
Two ways to fix it, then: (1) make escape() escape all shell metachars, or (2)
doublequote the results of escape(). I'm not sure of the ramifications of
either. We have to be very careful in this area.
------- Additional comments from Greg Noel Fri Jun 6 09:31:17 -0800 2008 -------
Hi Mark, Gary -- The question that came up during the bug party was who should
take the bug. If you had encountered it from ParseConfig/ParseFlages (which
call backtick), it made sense for me to work on it. If you encountered it when
executing a command, Steven made sense. If it was interwoven with other quoting
issues, Jim made sense. At that point, I hadn't studied the code, so I wasn't sure.
Now that I've been pushed to look at the code, I want to run a couple of timing
tests, but I'd bet that quoting the environment values makes os.system slower
than fork/exec in all cases, in which case, something like your patch makes a
lot of sense (although I'd take out the referenced functions as well).
Now if we could only cure that race between import and fork...
BTW, Gary, all *IX shells are absolutely uniform in the way they do quoting,
which means that quoting with a backslash is the only absolutely reliable way.
The simplest (but extreme) way is to quote every character; a less extreme way
is to quote only the shell metacharacters. From your comment, the set of quoted
characters is insufficient, and if the escape() routine survives, it should be
expanded to handle all shell metacharacters.
------- Additional comments from Mark Brown Fri Jun 6 10:22:40 -0800 2008 -------
This was encountered executing jobs.
If the patch were integrated upstream it'd need to drop the referenced
functions, yes. The patch doesn't do that since when carrying a diff in a
distribution it's better to carry the minimum diff since this avoids unneeded
rejects due to changes in the upstream code when porting diffs forwards.
------- Additional comments from Greg Noel Thu Sep 4 22:38:18 -0800 2008 -------
This patch is a hack to solve the immediate symptoms. It passes the regression
tests, but needs a test case of its own, so this issue shouldn't be closed until
that is complete.
------- Additional comments from Greg Noel Thu Sep 4 22:46:07 -0800 2008 -------
Oh, the escape function should be extended to include any other characters that
need to be quoted: <>{}[]!#&*~`?'| and possibly others. It's possible that
those could have side-effects, so I didn't do them for the initial patch.
Patch committed as revision 3340.
------- Additional comments from Steven Knight Sat Sep 6 06:35:03 -0800 2008 -------
Reassigning to gregnoel to record who fixed it.
