You are viewing a plain text version of this content. The canonical link for it is here.
Posted to bugs@apr.apache.org by bu...@apache.org on 2015/07/11 05:00:01 UTC
[Bug 58123] New: apr_proc_create does not quote APR_SHELLCMD
argument strings correctly on Unix
https://bz.apache.org/bugzilla/show_bug.cgi?id=58123
Bug ID: 58123
Summary: apr_proc_create does not quote APR_SHELLCMD argument
strings correctly on Unix
Product: APR
Version: HEAD
Hardware: PC
OS: Mac OS X 10.1
Status: NEW
Severity: normal
Priority: P2
Component: APR
Assignee: bugs@apr.apache.org
Reporter: jared.breeden@gmail.com
On unix, when executing an APR_SHELLCMD[_ENV] cmd_type, `apr_proc_create` does
not quote the arguments given to the shell correctly.
EXAMPLE:
- `apr_proc_create` is called with ["ruby", "-e", "exit 1"]
- In the code shown below, this is converted to [SHELL_PATH, "-c", "ruby -e
exit 1"]
- Notice that "exit 1" is not quoted, and so will be evaluated as two
arguments.
>From unix/proc.c, with comments added & some lines omitted:
```
if (attr->cmdtype == APR_SHELLCMD ||
attr->cmdtype == APR_SHELLCMD_ENV) {
/*
* ...
* Constucting `newargs` as [SHELL_PATH, '-c', onearg]
* ...
*/
default:
{
char *ch, *onearg;
ch = onearg = apr_palloc(pool, onearg_len);
i = 0;
while (args[i]) {
size_t len = strlen(args[i]);
/*
* Each argument is appended onto `onearg` without quotes.
*/
memcpy(ch, args[i], len);
ch += len;
*ch = ' ';
++ch;
++i;
}
--ch; /* back up to trailing blank */
*ch = '\0';
newargs[2] = onearg;
}
}
/*
* ...
* Later, when execv is called, the shell will split `onearg` on the spaces,
* possible separating any args that simply had spaces in them.
*/
execv(SHELL_PATH, (char * const *)newargs);
```
NOTE:
Not tested, but the code for Windows seems to handle this correctly...
>From win32/proc.c
```
cmdline = "";
for (i = 1; args && args[i]; ++i) {
if (has_space(args[i]) || !args[i][0]) {
cmdline = apr_pstrcat(pool, cmdline, " \"", args[i], "\"", NULL);
}
else {
cmdline = apr_pstrcat(pool, cmdline, " ", args[i], NULL);
}
}
```
WORKAROUND:
Passing the argument vector as a single string (ie: ["ruby -e \"exit 1\"])
seems to work fine.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org
[Bug 58123] apr_proc_create does not quote APR_SHELLCMD argument
strings correctly on Unix
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58123
--- Comment #4 from jared.breeden@gmail.com ---
It seems the quoting done in the Windows case is a bit too naive, as well. It
blindly wraps the arguments in quotes if (and only if) they contain a space. It
ignore the possibility that the arguments contain a " character themselves,
which requires escaping (and, I believe, to escape the double quote you have to
also wrap the whole arg in double quotes, regardless of whether there is a
space in the arg or not).
I seem to be bumping into a lot of problems using apr_create_proc, and I'm
having to perform platform specific quoting, catering not only to the platform
but to APR's behavior (eg I have to know that it's going to double quote my arg
on windows if it has a space and act accordingly - by quoting it myself if it
has a quote but no space in it, or not quoting if it *does* have a space. Plus
I'm doing the escaping). I don't believe I'm using the API incorrectly, but
that's possible.
I would think the general process for arguments would go:
Shell Cmd?
# The user already knows this is going
# to be shell interpreted, that's what they asked for.
# So don't process the args. Really, we could
# expect them to always pass only a single string.
# That would avoid having to decipher shell metacharacters
# and determine if the user expected them to be escaped or not.
cmdline = argv.join(' ')
On Windows?
# Comments in APR say cmd.exe expects the cmdline to be wrapped
# in quotes, but I don't find this to be the case (Windows 8 at least)
CreateProcess with command = "cmd.exe /C #{cmdline}"
On Unix?
execl('/bin/sh', '-c', cmdline);
Program Cmd?
On Unix?
# Great, the user passed us an argv that we can pass directly to exec*
On Windows?
# Make a cmdline that CommandLineToArgv will deconstruct back into our
# argv in the child process.
cmdline = progname
for each arg in argv
arg has double quote or space?
arg = escape quotes, escape \'s, wrap whole arg in double quotes
cmdline += " #{arg}"
end
CreateProcess with cmdline & the dozen other required arguments
end
end
This is the approach I'm using with APR at the moment. I use only APR_PROGRAM*
cmdtype (never APR_SHELLCMD*), and I build the cmdline strings myself if I'm
running a shell cmd. The results are much more aligned with my expectations
(and the results from typing directly into the shell) than the current
implementation.
If you guys agree there are some issues to resolve in this area, I'd be happy
to submit a patch proposal. For now, I'm going to stick with my workarounds
since I don't really want to maintain a fork. I know apr2 is being developed
now though... so this may be a good time to address these issues.
If I've just totally lost it, any guidance would be appreciated!
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org
[Bug 58123] apr_proc_create does not quote APR_SHELLCMD argument
strings correctly on Unix
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58123
jared.breeden@gmail.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #32895|0 |1
is obsolete| |
--- Comment #3 from jared.breeden@gmail.com ---
Created attachment 32896
--> https://bz.apache.org/bugzilla/attachment.cgi?id=32896&action=edit
Patch as diff run from root
Contribution guide reads "We should be able to feed the patch directly into the
"patch" program and have it update the file"
Remade the same patch from the root folder for this purpose. (Sorry for all the
updates!)
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org
[Bug 58123] apr_proc_create does not quote APR_SHELLCMD argument
strings correctly on Unix
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58123
jared.breeden@gmail.com changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jared.breeden@gmail.com
--- Comment #2 from jared.breeden@gmail.com ---
Created attachment 32895
--> https://bz.apache.org/bugzilla/attachment.cgi?id=32895&action=edit
Proposed patch
Here is a proposed patch to the proc file. It's basically taking the same code
from the win32/proc.c file. `has_space` is duplicated, which is probably not
ideal. However, it was tiny and static so it seemed ok. I can refactor to
remove the dup if desired. Just let me know where to put it!
Ran `testall` before & after the patch. Same results each time (2 failures on
my system, neither with procs) shown below. This fixes `apr_proc_create` for my
use case.
[jared:~/projects/apr/test] ./testall
testatomic : SUCCESS
testdir : SUCCESS
testdso : SUCCESS
testdup : SUCCESS
testenv : SUCCESS
testescape : SUCCESS
testfile : SUCCESS
testfilecopy : SUCCESS
testfileinfo : SUCCESS
testflock : SUCCESS
testfmt : SUCCESS
testfnmatch : SUCCESS
testargs : SUCCESS
testhash : SUCCESS
testhooks : SUCCESS
testipsub : SUCCESS
testlock : SUCCESS
testcond : SUCCESS
testlfs : SUCCESS
testmmap : SUCCESS
testnames : SUCCESS
testoc : SUCCESS
testpath : SUCCESS
testpipe : SUCCESS
testpoll : FAILED 1 of 24
testpools : SUCCESS
testproc : SUCCESS
testprocmutex : \default_timedlock() not implemented,-flock_timedlock()
not implemented,|sysvsem_timedlock() not implemented,\posix_timedlock() not
implemented,-fcntl_timedlock() not implemented,-default_timed not
implemented,FAILED 1 of 6
testrand : SUCCESS
testsleep : SUCCESS
testshm : SUCCESS
testsock : SUCCESS
testsockets : SUCCESS
testsockopt : SUCCESS
teststr : SUCCESS
teststrnatcmp : SUCCESS
testtable : SUCCESS
testtemp : SUCCESS
testthread : SUCCESS
testtime : SUCCESS
testud : SUCCESS
testuser : SUCCESS
testvsn : SUCCESS
teststrmatch : SUCCESS
testuri : SUCCESS
testuuid : SUCCESS
testbuckets : SUCCESS
testpass : SUCCESS
testbase64 : SUCCESS
testmd4 : SUCCESS
testmd5 : SUCCESS
testcrypto : SUCCESS
testdbd : SUCCESS
testdate : SUCCESS
testmemcache : SUCCESS
testxml : SUCCESS
testxlate : SUCCESS
testrmm : SUCCESS
testdbm : SUCCESS
testqueue : SUCCESS
testreslist : SUCCESS
testlfsabi : SUCCESS
testskiplist : SUCCESS
Failed Tests Total Fail Failed %
===================================================
testpoll 24 1 4.17%
testprocmutex 6 1 16.67%
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org
[Bug 58123] apr_proc_create does not quote APR_SHELLCMD argument
strings correctly on Unix
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58123
--- Comment #1 from jared.breeden@gmail.com ---
Forgot to close a quote in the workaround. Sorry for not supplying real code
examples. I'm working with the code through mruby bindings, and I didn't think
posting Ruby was helpful.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org