NotificationsYou must be signed in to change notification settings
Fork3.8k
Star26.3k

win,fs: get fstat of AppExecLink reparse points#4663

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

huseyinacacak-janea wants to merge5 commits intolibuv:v1.x

base:v1.x

Choose a base branch

fromJaneaSystems:huseyin-9465-appexeclink-reparse-point

Open

win,fs: get fstat of AppExecLink reparse points#4663

huseyinacacak-janea wants to merge5 commits intolibuv:v1.xfromJaneaSystems:huseyin-9465-appexeclink-reparse-point

Conversation

Copy link

Contributor

huseyinacacak-janea commentedDec 27, 2024

This fixes the problem of getting the fstat of AppExecLink reparse points (e.g. Windows Apps).

Refs:nodejs/node#36790

win,fs: get fstat of AppExecLink reparse points

68000aa

vtjnash reviewed

Dec 27, 2024

View reviewed changes

Copy link

Member

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, you actually need to replace the firstCreateFileW on this code path with this fallback, otherwise (here) you introduced a TOCTOU bug into the code. And also replace almost allCreateFileW calls in this fs.c file with this retry strategy, like this pseudo code:

if (!do_lstat) {    for (i = 0; i < 255; i++)        /* Try to open the file using kernel filters */        if (CreateFileW())            break        /* The kernel doesn't implement all filters, as some must be emulated in userspace by every application: see if this is a tag that libuv knows about */        if (! CreateFileW(FILE_FLAG_OPEN_REPARSE_POINT))            return error        if (! fs__readlink_handle())            return error        CloseHandle()}else    CreateFileW(FILE_FLAG_OPEN_REPARSE_POINT)if (error)    return errorreturn handle

huseyinacacak-janea force-pushed thehuseyin-9465-appexeclink-reparse-point branch frome08364d tobe96774Compare

January 3, 2025 13:24

fixup! win,fs: get fstat of AppExecLink reparse points

be96774

Copy link

ContributorAuthor

huseyinacacak-janea commentedJan 3, 2025

Thanks for the review. I've applied your suggestion to the respective place.
I've added a utility function (fs__stat_get_handle) to add the logic to the other places. I would prefer opening a new PR and calling this function from other places. However, if you want me to include those fixes in this PR, I can add another commit to this PR.

vtjnash reviewed

Jan 3, 2025

View reviewed changes

src/win/fs.c OutdatedShow resolvedHide resolved

vtjnash reviewed

Jan 3, 2025

View reviewed changes

src/win/fs.c OutdatedShow resolvedHide resolved

vtjnash reviewed

Jan 3, 2025

View reviewed changes

src/win/fs.c

		stat_info.CreationTime.QuadPart=dir_info.CreationTime.QuadPart;
		stat_info.LastAccessTime.QuadPart=dir_info.LastAccessTime.QuadPart;
		stat_info.LastWriteTime.QuadPart=dir_info.LastWriteTime.QuadPart;
		if (stat_info.FileAttributes&FILE_ATTRIBUTE_REPARSE_POINT) {

Copy link

Member

vtjnashJan 3, 2025

Choose a reason for hiding this comment

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

There should not need to be any changes tofs__stat_directory function, as you're introducing a TOCTOU bug by doing so

Copy link

ContributorAuthor

huseyinacacak-janeaJan 9, 2025

Choose a reason for hiding this comment

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

I tried to fix it. Hope there is no TOCTOU bug.

huseyinacacak-janeaand others added3 commits

January 6, 2025 09:16

Update src/win/fs.c

85b488e

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

fixup! win,fs: get fstat of AppExecLink reparse points

8dc553a

fixup! win,fs: get fstat of AppExecLink reparse points

4ee3bd5

Copy link

ContributorAuthor

huseyinacacak-janea commentedJan 24, 2025

Is there anything I can do to help this PR move forward?

vtjnash reviewed

Jan 24, 2025

View reviewed changes

Copy link

Member

vtjnash left a comment

Choose a reason for hiding this comment

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

I need you to rework the changes such that the only code being changed in stat is that any existing call to CreateFile are replaced by your new helperfs__create_file (fs__stat_get_handle), instead of changing thestat functions themselves. That should avoid any time-of-check issues and then give us a drop-in replacement function to fix all of the other CreateFile calls later also. I have been trying to find time to do it myself to better explain what I mean, but other concerns have kept me too busy lately.

Copy link

ContributorAuthor

huseyinacacak-janea commentedJan 31, 2025

Sorry, I couldn't quite understand what you meant. Let me explain the options I have in mind.

Add thisCreateFileW (calling for directory handle) to the new helper function and replace thisCreateFileW call with the helper function call. However, this will require checking if the handle returned is a directory handle or file handle in this stat function to call the respective API (eitherpNtQueryDirectoryFile orpNtQueryInformationFile).
Just call the new helper function in caseif (!do_lstat), which is what I'm currently doing in this PR. If I delete the lines below this helper function call to keep this stat function unchanged, the information returned by this stat function will not be correct.

Copy link

ContributorAuthor

huseyinacacak-janea commentedFeb 7, 2025

I'll be stepping down from the project in 2 weeks. If there is anything I can do to help this PR move forward, feel free to reach out to me until then.