Fix GH-10992: Improper long path support for relative paths #16687
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Relative paths are passed to the ioutils APIs, these are not properly converted to long paths. If the path length already exceeds a given threshold (usually 259 characters, but only 247 for `mkdir()`), the long path prefix is prepended, resulting in an invalid path, since long paths have to be absolute. If the path length does not exceed that threshold, no conversion to a long path is done, although that may be necessary. Thus we take the path length of the current working directory into account when checking the threshold, and prepend it to the filename if necessary. Since this is only relevant for NTS builds, and using the current working directory of the process would be erroneous for ZTS builds, we skip the new code for ZTS builds.
|
I don't think there is an ABI break, and even no API break (except that consumers would need to be rebuilt to get the new functionality). Still, something to consider. |
nielsdos
reviewed
Nov 4, 2024
|
I've applied to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relative paths are passed to the ioutils APIs, these are not properly converted to long paths. If the path length already exceeds a given threshold (usually 259 characters, but only 247 for
mkdir()), the long path prefix is prepended, resulting in an invalid path, since long paths have to be absolute. If the path length does not exceed that threshold, no conversion to a long path is done, although that may be necessary.Thus we take the path length of the current working directory into account when checking the threshold, and prepend it to the filename if necessary.
Since this is only relevant for NTS builds, and using the current working directory of the process would be erroneous for ZTS builds, we skip the new code for ZTS builds.
Note that I chose the least intrusive solution I could think of since I would like to fix this issue for stable branches; still, I'm not absolutely sure that this patch may not have inadvertent side effects, so I've targeted PHP-8.4 for now.
I think in the long run the whole ioutil code should be reviewed and overhauled. I doubt that the heavy inlining is reasonable, especially considering the many (re-)allocations of buffers (all using native Windows
malloc()and friends which are supposed to be relatively slow), and some operations might be simplified (possibly at the cost of some runtime performance loss, which may not really matter here). Furthermore, the "module" dependencies are not quite clear (ioutils, Zend, TSRM is all a bit mixed up).cc @nielsdos