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.

Sign up for GitHub

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, fs: add fallback for fs__stat_handle #3268

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

win, fs: add fallback for fs__stat_handle #3268

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 79 additions & 3 deletions src/win/fs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1678,6 +1678,75 @@ void fs__closedir(uv_fs_t* req) {
SET_REQ_RESULT(req, 0);
}

/* This function is called when fs__stat_impl_from_path
* CreateFileW fails with the code ERROR_ACCESS_DENIED (0x5)
* It's a fallback when the user doesn't have the permission
* To open a handle to the desired file, this queries less
* Information, but still have the most important ones.
*
* These are the values we can access in this method:
* - File Attributes
* - Creation Time
* - Last Access Time
* - Last Write Time
* - File Size
*/
INLINE static int fs__stat_nohandle(WCHAR* path, uv_stat_t* statbuf) {
WIN32_FIND_DATAW find_data;
HANDLE handle;

handle = FindFirstFileW(path, &find_data);

if (handle == INVALID_HANDLE_VALUE)
return -1;

memset(statbuf, 0, sizeof(*statbuf));

uv__filetime_to_timespec(&statbuf->st_atim,
(uint64_t)find_data.ftLastAccessTime.dwHighDateTime << 32 |
find_data.ftLastAccessTime.dwLowDateTime
);

uv__filetime_to_timespec(&statbuf->st_ctim,
(uint64_t)find_data.ftCreationTime.dwHighDateTime << 32 |
find_data.ftCreationTime.dwLowDateTime
);

uv__filetime_to_timespec(&statbuf->st_mtim,
(uint64_t)find_data.ftLastWriteTime.dwHighDateTime << 32 |
find_data.ftLastWriteTime.dwLowDateTime
);

uv__filetime_to_timespec(&statbuf->st_birthtim,
(uint64_t)find_data.ftCreationTime.dwHighDateTime << 32 |
find_data.ftCreationTime.dwLowDateTime
);

if (find_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
statbuf->st_size = 0;
statbuf->st_mode |= _S_IFDIR;
} else {
statbuf->st_mode |= _S_IFREG;
statbuf->st_size =
find_data.nFileSizeLow | ((uint64_t)find_data.nFileSizeHigh << 32);
}

if (find_data.dwFileAttributes & FILE_ATTRIBUTE_READONLY) {
statbuf->st_mode |=
_S_IREAD | (_S_IREAD >> 3) | (_S_IREAD >> 6);
} else {
statbuf->st_mode |= (_S_IREAD | _S_IWRITE) |
((_S_IREAD | _S_IWRITE) >> 3) |
((_S_IREAD | _S_IWRITE) >> 6);
}

oluan marked this conversation as resolved.
Show resolved Hide resolved
statbuf->st_blksize = 4096;
oluan marked this conversation as resolved.
Show resolved Hide resolved

FindClose(handle);

return 0;
}

INLINE static int fs__stat_handle(HANDLE handle, uv_stat_t* statbuf,
int do_lstat) {
FILE_ALL_INFORMATION file_info;
Expand Down Expand Up @@ -1853,10 +1922,17 @@ INLINE static DWORD fs__stat_impl_from_path(WCHAR* path,
flags,
NULL);

if (handle == INVALID_HANDLE_VALUE)
return GetLastError();
if (handle == INVALID_HANDLE_VALUE) {
ret = GetLastError();

if (fs__stat_handle(handle, statbuf, do_lstat) != 0)
if (ret == ERROR_ACCESS_DENIED) {
if (fs__stat_nohandle(path, statbuf) != 0)
Copy link
Member

Choose a reason for hiding this comment

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

This introduces a race condition and we try very hard to avoid those in libuv. It's a TOCTOU variant: the file can change between the CreateFile and FindFirstFile calls.

It's probably benign (I can't think of a plausible scenario where it'd matter) but still, if CreateFile can be substituted with FindFirstFile with no loss of functionality, then why not use FindFirstFile always?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, FindFirstFile queries less information.

Currently, libuv calls CreateFile to open a handle to the file, and then call another two other functions to gather information:

NtQueryInformationFile
NtQueryVolumeInformationFile

and in some cases DeviceIoControl.

See: https://github.com/libuv/libuv/blob/v1.x/src/win/fs.c#L1696

As we can't open handles to files that we don't have the necessary permission for, the FindFirstFile is a fallback that won't require a handle and only get the most critical information: creation date, last write, last access, size, and file attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnoordhuis bump, I guess?

Copy link
Member

Choose a reason for hiding this comment

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

@bnoordhuis I don't think this is a TOCTOU, since we don't try to merge the information. It is a bit unfortunate that there is a narrow window where a few items of information won't be available, which makes the filesystem appear less atomic than it would otherwise have been.

Alternatively, we could repeat the CreateFileW above but without FILE_READ_ATTRIBUTES, which I assume is what FindFirstFile is doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CreateFileW will fail no matter what if the user doesn't have the necessary permissions to a file.

Copy link
Member

Choose a reason for hiding this comment

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

We already have the code to handle the result of NtOpenFile though, whereas this requires a new code path to handle the results.

Copy link
Contributor Author

@oluan oluan Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

POC with NtOpenFile (kinda messy, just a poc 🔨): oluan@fd6b562

I guess this just adds even more new paths VS FindFirstFileW, even if we deal only with NtOpenFile

Copy link
Member

Choose a reason for hiding this comment

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

Turns out FindFirstFile is (un)documented here to calling NtQueryDirectoryFile

Copy link
Member

@vtjnash vtjnash Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Does the POC work? I expected you need to open a handle to the containing directory, rather than the file itself. I guess I need to do a bit of testing myself, but I think I understand this issue better now, to the point of being able to review and merge something. I was wrong about needing NtOpenFile for in case, since it just needs CreateFile for the parent directory. It appears that thhe advantage of NtQueryDirectoryFile is a substantial performance boost over FindFirstFile (according to git and chromium, who tested this and switched) and the ability to still query the drive information.

Copy link
Member

Choose a reason for hiding this comment

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

I also realized this PR seems like it may implement lstat, so to implement stat, it needs to check if dwFileAttributes member includes FILE_ATTRIBUTE_REPARSE_POINT, then figure out how to handle that if so

ret = GetLastError();
else
ret = 0;
}
}
else if (fs__stat_handle(handle, statbuf, do_lstat) != 0)
ret = GetLastError();
else
ret = 0;
Expand Down