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

win: fix fs.stat for AppExecLink #3874

Closed

Conversation

StefanStojanovic
Copy link
Contributor

Calling uv_fs_stat for AppExecLink reparse points (eg. LOCALAPPDATA\Microsoft\\WindowsApps\MicrosoftEdge.exe or any other application from that directory) was failing with EACCES code. Under the hood, what was failing was the CreateFileW function because FILE_FLAG_OPEN_REPARSE_POINT was not used in that case, unlike in the uv_fs_lstat.

Based on documentation using the FILE_FLAG_OPEN_REPARSE_POINT flag is safe because "If the file is not a reparse point, then this flag is ignored". As a result, this change will not have a negative (nor any other) effect on all other cases when the file is not an AppExecLink reparse point.

This PR also adds a test covering this behavior.

Refs: nodejs/node#36790
Refs: nodejs/node#33024
Refs: #2812

Calling uv_fs_stat for AppExecLink reparse points (eg.
LOCALAPPDATA\Microsoft\\WindowsApps\MicrosoftEdge.exe) was failing with
EACCES code. This change enables making that call without failure.

Also added a test covering this behavior.

Refs: nodejs/node#36790
Refs: nodejs/node#33024
Refs: libuv#2812
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this would change stat to do lstat instead, which would be very broken.

@vtjnash
Copy link
Member

vtjnash commented Jan 9, 2023

Please see issue comment that implemented the fix and closed this issue originally: nodejs/node#33024 (comment)

@vtjnash vtjnash closed this Jan 18, 2023
@StefanStojanovic
Copy link
Contributor Author

Apologies for responding only now. TL;DR Closing this PR was the right thing to do.

IIUC, this would change stat to do lstat instead, which would be very broken.

Not completely, but overall yes. This would change the way a handle to a file is created. Afterward, in fs__stat_handle stat and lstat would still work differently in the case of a symlink when setting size and mode, but ultimately statbuf would be the same except for those two fields.

Unfortunately, all tests passed, so this issue was caught during the review. Adding a test for comparing stat and lstat results could help prevent this in the future.

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.

2 participants