Everything

php

Suggestion for fixing Bug #40928

php.internals

Tzachi Tager

19 years ago

Hi,
I was looking at Bug #40928 - escapeshellarg() does not quote percent
(%) correctly for cmd.exe.
This bug seems to be because escapeshellarg() in  Windows replaces '%'
and '"' with spaces, while assuming there isn't a real escaping method
for command line in Windows. Therefore I'm guessing no one really use
escapeshellarg() or escapeshellcmd() on Windows. And in order to change
this  I suggest to use the command line escaping that does exists
(although looking a bit ugly), as you can see for example here:
http://www.microsoft.com/resources/documentation/windows/xp/all/proddocs
/en-us/ntcmds_shelloverview.mspx?mfr=true , quoting: "You can use most
characters as variable values, including white space. If you use the
special characters <, >, |, &, or ^, you must precede them with the
escape character (^) or quotation marks." - So all special characters
will be replaced with "^<char>".
So this is the diff file that I suggest to use- it for sure fix the
above bug and may improve windows escapeshellcmd():

cvs diff -- exec.c (in directory C:\php-src\ext\standard\)
Index: exec.c
===================================================================
RCS file: /repository/php-src/ext/standard/exec.c,v
retrieving revision 1.113.2.3.2.1
diff -r1.113.2.3.2.1 exec.c
275d274
< 			case '\'':
276a276,277

> 			case '\'':
> #endif

281a283

> #ifndef PHP_WIN32

282a285,289

> #else
> 					cmd[y++] = '"';
> 					cmd[y++] = '^';
> 					cmd[y++] = str[x];
> #endif

286,287d292
< #endif
< 			case '#': /* This is character-set independent
*/
289,290d293
< 			case ';':
< 			case '`':
292,294d294
< 			case '*':
< 			case '?':
< 			case '~':
299a300,313

> #ifdef PHP_WIN32
> 			case '%':
> 				cmd[y++] = '"';
> 				cmd[y++] = '^';
> 				cmd[y++] = str[x];
> 				cmd[y++] = '"';
> 				break;
> #endif
> 			case '#': /* This is character-set independent

*/

> 			case ';':
> 			case '`':
> 			case '*':
> 			case '?':
> 			case '~':

305d318
< 			case '\\':
309,310c322
< 			/* since Windows does not allow us to escape
these chars, just remove them */
< 			case '%':
---

> 				/* since Windows does not allow us to

escape these chars, just remove them */
313a326

> 			case '\\':

347d359
< 		case '%':

Comments will be greatly appreciated.
All the best,
Tzachi Tager.

Frode E. Moe

19 years ago

On Fri, Jul 06, 2007 at 01:29:31 +0300, Tzachi Tager wrote:

> Hi,
> I was looking at Bug #40928 - escapeshellarg() does not quote percent
> (%) correctly for cmd.exe.
> This bug seems to be because escapeshellarg() in  Windows replaces '%'
> and '"' with spaces, while assuming there isn't a real escaping method
> for command line in Windows. Therefore I'm guessing no one really use
> escapeshellarg() or escapeshellcmd() on Windows. And in order to change
> this  I suggest to use the command line escaping that does exists
> (although looking a bit ugly), as you can see for example here:
> http://www.microsoft.com/resources/documentation/windows/xp/all/proddocs
> /en-us/ntcmds_shelloverview.mspx?mfr=true , quoting: "You can use most
> characters as variable values, including white space. If you use the
> special characters <, >, |, &, or ^, you must precede them with the
> escape character (^) or quotation marks." - So all special characters
> will be replaced with "^<char>".
> So this is the diff file that I suggest to use- it for sure fix the
> above bug and may improve windows escapeshellcmd():

Hi, I'm the guy who reported the bug originally.

When I read your post now, I just realized that maybe there should be a
different set of escaping functions for win32 ("escapewin32arg" or
"escapecmdarg"?), so that the behaviour of escapeshellarg() does not change
across platforms. (What if you want to dynamically generate a downloadable
unix shell script, for example.)

Your patch was a bit difficult to read (too little context and not in
unidiff format), so I'll leave the commenting for those more familiar
with the C source.

Thanks for working on the problem, anyway!

Tzachi Tager

19 years ago

Sorry guys,
I've added below the unified and more readable diff.
Frode, about your comment I generally agree, it would be much easier to implement it differently for Windows as a separate code. In this diff I tried my best to avoid hurting Unix functionality.

Still comments are welcome.

Tzachi Tager.


cvs diff -u -- exec.c (in directory C:\php-src\ext\standard\)
Index: exec.c
===================================================================
RCS file: /repository/php-src/ext/standard/exec.c,v
retrieving revision 1.113.2.3.2.1
diff -u -r1.113.2.3.2.1 exec.c
--- exec.c	1 Jan 2007 09:36:08 -0000	1.113.2.3.2.1
+++ exec.c	28 Jun 2007 14:15:42 -0000
@@ -272,45 +272,58 @@
for (x = 0, y = 0; x < l; x++) {
switch (str[x]) {
case '"':
-			case '\'':
#ifndef PHP_WIN32
+			case '\'':
+#endif
if (!p && (p = memchr(str + x + 1, str[x], l - x - 1))) {
/* noop */
} else if (p && *p == str[x]) {
p = NULL;
} else {
+#ifndef PHP_WIN32
cmd[y++] = '\\';
+#else
+					cmd[y++] = '"';
+					cmd[y++] = '^';
+					cmd[y++] = str[x];
+#endif
}
cmd[y++] = str[x];
break;
-#endif
-			case '#': /* This is character-set independent */
case '&':
-			case ';':
-			case '`':
case '|':
-			case '*':
-			case '?':
-			case '~':
case '<':
case '>':
case '^':
case '(':
case ')':
+#ifdef PHP_WIN32
+			case '%':
+				cmd[y++] = '"';
+				cmd[y++] = '^';
+				cmd[y++] = str[x];
+				cmd[y++] = '"';
+				break;
+#endif
+			case '#': /* This is character-set independent */
+			case ';':
+			case '`':
+			case '*':
+			case '?':
+			case '~':
case '[':
case ']':
case '{':
case '}':
case '$':
-			case '\\':
case '\x0A': /* excluding these two */
case '\xFF':
#ifdef PHP_WIN32
-			/* since Windows does not allow us to escape these chars, just remove them */
-			case '%':
+				/* since Windows does not allow us to escape these chars, just remove them */
cmd[y++] = ' ';
break;
#endif
+			case '\\':
cmd[y++] = '\\';
/* fall-through */
default:
@@ -344,7 +357,6 @@
switch (str[x]) {
#ifdef PHP_WIN32
case '"':
-		case '%':
cmd[y++] = ' ';
break;
#else


-----Original Message-----
From: Frode E. Moe [mailto:frode@CoreTrek.no]
Sent: ו 06 יולי 2007 09:42
To: Tzachi Tager
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Suggestion for fixing Bug #40928

On Fri, Jul 06, 2007 at 01:29:31 +0300, Tzachi Tager wrote:

> Hi,
> I was looking at Bug #40928 - escapeshellarg() does not quote percent
> (%) correctly for cmd.exe.
> This bug seems to be because escapeshellarg() in  Windows replaces '%'
> and '"' with spaces, while assuming there isn't a real escaping method
> for command line in Windows. Therefore I'm guessing no one really use
> escapeshellarg() or escapeshellcmd() on Windows. And in order to change
> this  I suggest to use the command line escaping that does exists
> (although looking a bit ugly), as you can see for example here:
> http://www.microsoft.com/resources/documentation/windows/xp/all/proddocs
> /en-us/ntcmds_shelloverview.mspx?mfr=true , quoting: "You can use most
> characters as variable values, including white space. If you use the
> special characters <, >, |, &, or ^, you must precede them with the
> escape character (^) or quotation marks." - So all special characters
> will be replaced with "^<char>".
> So this is the diff file that I suggest to use- it for sure fix the
> above bug and may improve windows escapeshellcmd():

Hi, I'm the guy who reported the bug originally.

When I read your post now, I just realized that maybe there should be a
different set of escaping functions for win32 ("escapewin32arg" or
"escapecmdarg"?), so that the behaviour of escapeshellarg() does not change
across platforms. (What if you want to dynamically generate a downloadable
unix shell script, for example.)

Your patch was a bit difficult to read (too little context and not in
unidiff format), so I'll leave the commenting for those more familiar
with the C source.

Thanks for working on the problem, anyway!

Stanislav Malyshev

19 years ago

> /en-us/ntcmds_shelloverview.mspx?mfr=true , quoting: "You can use most
> characters as variable values, including white space. If you use the
> special characters <, >, |, &, or ^, you must precede them with the
> escape character (^) or quotation marks." - So all special characters
> will be replaced with "^<char>".

I think you are not entirely correct here. What the quote means is that
if you want foo&bar, you should do either "foo&bar" or foo^&bar - not
replace & with "^&".

--
Stanislav Malyshev, Zend Software Architect
stas@zend.com   http://www.zend.com/
(408)253-8829   MSN: stas@zend.com

Tzachi Tager

19 years ago

The reason I'm doing this is because it seems that Windows parser looks for an odd number of '"' before the escaping, otherwise the escaping character is included in the outcome, for example:

string(25) "md "test ^& echo ^%foo^%""  ==> will output a directory called "test ^& echo ^%foo^%"

While:
string(31) "md "test "^&" echo "^%"foo"^%""" ==> will output a directory called "test & echo %foo%"

(I changed the php code and ran again, these are not the runs of the cmd.exe which does the escaping internally).
Which is what I wanted in the first place (and I know it looks ugly...).

Tzachi.

-----Original Message-----
From: Stanislav Malyshev [mailto:stas@zend.com]
Sent: ג 10 יולי 2007 23:51
To: Tzachi Tager
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Suggestion for fixing Bug #40928

> /en-us/ntcmds_shelloverview.mspx?mfr=true , quoting: "You can use most
> characters as variable values, including white space. If you use the
> special characters <, >, |, &, or ^, you must precede them with the
> escape character (^) or quotation marks." - So all special characters
> will be replaced with "^<char>".

I think you are not entirely correct here. What the quote means is that
if you want foo&bar, you should do either "foo&bar" or foo^&bar - not
replace & with "^&".

--
Stanislav Malyshev, Zend Software Architect
stas@zend.com   http://www.zend.com/
(408)253-8829   MSN: stas@zend.com