Skip to content

Fixed bug #79470: PHP incompatible with 3rd party file system on demand #5379

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

Closed
wants to merge 1 commit into from

Conversation

gilbertjgonzales
Copy link

Update tsrm_realpath_r() to handle unknown reparse tags with
default behavior that matches how
IO_REPARSE_TAG_DEDUP, IO_REPARSE_TAG_CLOUD,
IO_REPARSE_TAG_ONEDRIVE were handled previously.
In general the default behavior for 3rd party files systems
with non public reparse tags will be using default behavior
instead of having to be added explicitly to PHP in order to
be supported.

This change keeps existing behavior and expands the behavior to
future or non-public "Fetch On Demand" file systems.

Update tsrm_realpath_r() to handle unknown reparse tags with
default behavior that matches how
IO_REPARSE_TAG_DEDUP, IO_REPARSE_TAG_CLOUD,
IO_REPARSE_TAG_ONEDRIVE were handled previously.
In general the default behavior for 3rd party files systems
with non public reparse tags will be using default behavior
instead of having to be added explicitly to PHP in order to
be supported.

This change keeps existing behavior and expands the behavior to
future or non-public "Fetch On Demand" file systems.
@KalleZ
Copy link
Member

KalleZ commented Apr 13, 2020

@cmb69

@cmb69
Copy link
Member

cmb69 commented Apr 15, 2020

Thanks for the PR (and also the information in the bugtracker)!

It seems to me that we must not assume that any reparse point except for IO_REPARSE_TAG_SYMLINK and IO_REPARSE_TAG_MOUNT_POINT resolves to itself; at the very least we should assume that !IsReparseTagNameSurrogate() does not. And even with this amendment, I'm reluctant to make this change for the stable PHP versions (7.3 being particularly problematic). @weltling, thoughts?

Anyhow, could you please check your reparse points with fsutil reparsePoint query <filename>, and post the results (the "Reparse Tag Value" should be sufficient).

PS: relevant link for futher lookup: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/c8e77b37-3909-4fe6-a4ea-2b9d423b1ee4

@gilbertjgonzales
Copy link
Author

gilbertjgonzales commented Apr 15, 2020 via email

@wadetb
Copy link

wadetb commented Apr 15, 2020

Hi, our reparse tag is found in ntifs.h in the Windows 10 SDK here:

//
//  Tag allocated to Activision for HSM
//  GUID: 18CC35B3-5DFF-408E-B42E-9FA6731BC506
//

#define IO_REPARSE_TAG_ACTIVISION_HSM           (0x00000047L)

It seems to me that we must not assume that any reparse point except for IO_REPARSE_TAG_SYMLINK and IO_REPARSE_TAG_MOUNT_POINT resolves to itself

Actually, I think that you must- reparse points are used for many functions besides links, and if the application doesn't know specifically what the reparse tag means it must make no assumptions about the meaning and treat it as an ordinary file.

Other libraries with filesystem abstractions such as libuv take this approach (https://github.com/libuv/libuv/blob/master/src/win/fs.c#L1693).

at the very least we should assume that !IsReparseTagNameSurrogate() does not.

This seems like a reasonable compromise, but I don't know what you can do with a link that you cannot read- seems better to just let the filesystem handle it.

@cmb69
Copy link
Member

cmb69 commented Apr 16, 2020

Thanks for the further information!

This seems like a reasonable compromise, but I don't know what you can do with a link that you cannot read- seems better to just let the filesystem handle it.

This is about PHP's implementation of realpath(), and not properly resolving (or rejecting) reparse points could even lead to security issues. The documentation of IsReparseTagNameSurrogate states:

If the surrogacy bit is set, the file or directory represents another named entity in the system, such as a mounted folder that associates this directory with another volume.

So I think we should better reject "unknown" reparse tags which are IsReparseTagNameSurrogate().

I tend to agree that we should accept all other reparse tags (not only IO_REPARSE_TAG_ACTIVISION_HSM; there is also a bug report regarding VFS for Git), but I'm still somewhat reluctant to do this for the stable releases.

I would like to get more insights on this topic. :)

@wadetb
Copy link

wadetb commented Apr 20, 2020

Maybe the best course of action would be to extend the existing OneDrive codepath to cover the Git VFS and the Activision tags in the stable release, and then to put the more generic solution into development builds.

Since it's simply extending an existing check to cover our tag, and our tag isn't present in the wild, it feels like it would be a safe change to make in a stable release.

@cmb69
Copy link
Member

cmb69 commented Apr 21, 2020

Maybe the best course of action would be to extend the existing OneDrive codepath to cover the Git VFS and the Activision tags in the stable release, and then to put the more generic solution into development builds.

I agree that this looks like the best way forward. @weltling, @dalehhirt, what do you think?

@cmb69
Copy link
Member

cmb69 commented Apr 24, 2020

I have just submitted PR #5451, which just adds support for IO_REPARSE_TAG_ACTIVISION_HSM and IO_REPARSE_TAG_PROJFS. While I have tested the latter, I cannot test the former, so @gilbertjgonzales or @wadetb, could you please test that PR, please? If there are no issues, I'd like to merge the other PR on Monday, so the fix would make it into 7.3.18RC1.

@gilbertjgonzales
Copy link
Author

gilbertjgonzales commented Apr 26, 2020 via email

@cmb69
Copy link
Member

cmb69 commented Apr 27, 2020

Thanks for checking!

I have merged the other PR, but will keep this one open as a reminder that we might want to have a more general solution for master. I'll see to that ASAP.

@cmb69
Copy link
Member

cmb69 commented May 13, 2020

PR #5565 for general support is now available, so I'm closing this PR.

Thanks again for having worked on this!

@cmb69 cmb69 closed this May 13, 2020
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