Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebSVN's diffs do not work on Windows #210

Closed
raspopov opened this issue Jan 24, 2024 · 6 comments · Fixed by #213
Closed

WebSVN's diffs do not work on Windows #210

raspopov opened this issue Jan 24, 2024 · 6 comments · Fixed by #213
Assignees

Comments

@raspopov
Copy link
Contributor

The next code at include\command.php:123:

	// https://github.com/websvnphp/websvn/issues/75
	// https://github.com/websvnphp/websvn/issues/78
	if ($config->serverIsWindows) {
		if (!strpos($cmd, '>') && !strpos($cmd, '|')) {
			$opts = array('bypass_shell' => true);
		} else {
			$cmd = '"'.$cmd.'"'; <-- THIS LINE
		}
	}

produces the wrong escaping of cmd.exe on Windows for example:
cmd.exe /s /c """C:\svn\svn" --non-interactive --config-dir C:\svn\config --trust-server-cert cat -r 15 "file:///C:/Repositories/c4u/trunk/include/CoTaskMem.h@16" > "C:\php\tmp\1DDA.tmp"""
i.e. 3 quotation marks at the beginning and end of the command line instead of 2.
This bug effectively turns off the diffs of the revisions.

PHP 8.2.7, WebSVN 2.8.3

@michael-o
Copy link
Member

Will try to take a look. Using the cmd.exe/sh is ugly in many ways. @slawekjaranowski, we know this crap from m-s-utils and p-utils...

@michael-o michael-o self-assigned this Jan 25, 2024
@michael-o
Copy link
Member

Here is the C code:
https://github.com/php/php-src/blob/b06fedb41d560f756dfb5d964b0d36a224e44dc7/ext/standard/proc_open.c#L645-L649

Did you try to remove the redudant quotes and see what happens?

@michael-o
Copy link
Member

I bet that quoting isn't necessary anymore...

@raspopov
Copy link
Contributor Author

raspopov commented Jan 27, 2024

Did you try to remove the redudant quotes and see what happens?

To fix the diffs functionality in my WebSVN installation I already commented out the above line of code.

The /s option removes the single quotes from both sides of the cmd.exe command line, cmd.exe is added when bypass_shell = false if redirection is required.

@raspopov
Copy link
Contributor Author

raspopov commented Jan 27, 2024

I found when it was changed: ext/standard/proc_open.c.
At the time, this function was an big monster yet and PHP was version 8.0.0 RC2.
This makes sense, I hit into that issue after upgrading from PHP 5 to PHP 8. 🤔

@michael-o
Copy link
Member

I found when it was changed: ext/standard/proc_open.c. At the time, this function was an big monster yet and PHP was version 8.0.0 RC2. This makes sense, I hit into that issue after upgrading from PHP 5 to PHP 8. 🤔

Found the same, 8 had a big hit. Will address this issue.

michael-o added a commit that referenced this issue Jan 30, 2024
With PHP 8.0.0-RC2 the command handling has been simplified and improved.
Quoting can be removed since it is now handled properly internally.

See also php/php-src@9ca449e

This fixes #210 and closes #213
michael-o added a commit that referenced this issue Jan 30, 2024
With PHP 8.0.0-RC2 the command handling has been simplified and improved.
Quoting can be removed since it is now handled properly internally.

See also php/php-src@9ca449e

This fixes #210 and closes #213
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants