Skip to content

Conversation

janstarke
Copy link

Hi,

is there any reason why you replace the DEFAULT_SLASH to which ptr points to, by '/'? If you do so, you may make the directory name unreadable for the current system :-/

@smalyshev
Copy link
Contributor

Could you add better explanation of what this patch actually does and why?

@janstarke
Copy link
Author

Hi,

this patch has no effect when php is run on unix/linux systems. Is is
important when PHP runs on Windows: the before-version of this codes
replaces all backslashes one after the other by slashes, so that e.g.
c:\wwwroot\sample becomes c:/wwroot/sample. Regarding Windows, this is not
really a problem, because Windows understands both; slash and backslash.
But be aware: When compiling in Windows; '/' != DEFAULT_SLASH.

However; this code is a search function and should not change the input
string: if I pass a string buffer to the function, the buffer should have
the same value after calling this function. So, when you (temporarily)
replace a character by '\0', you should "repair" this modification
correctly. The assumption that all systems handle '/' equally to
DEFAULT_SLASH limits the portability of PHP and creates an undocumented
side effect of php_cgi_ini_activate_user_config.

Regards, Jan

2015-04-19 2:19 GMT+02:00 Stanislav Malyshev notifications@github.com:

Could you add better explanation of what this patch actually does and why?


Reply to this email directly or view it on GitHub
#1237 (comment).

@jpauli
Copy link
Member

jpauli commented May 12, 2015

You got some #ifdebug macros to remove in your code

@janstarke
Copy link
Author

Okay, I'll fix this as soon as possible...

@krakjoe
Copy link
Member

krakjoe commented Jan 4, 2017

Since we have waited more than 18 months for this PR to be updated, and so it's reasonable to assume it has been abandoned by the author, and since it targets a security fix only branch, I'm closing this PR.

Please take this action as encouragement to open a clean PR against a supported branch.

@krakjoe krakjoe closed this Jan 4, 2017
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 this pull request may close these issues.

4 participants