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

Speculative fix for alignment crash in directory iterator on Windows #10628

Merged
merged 3 commits into from
Apr 29, 2024
Merged

Speculative fix for alignment crash in directory iterator on Windows #10628

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d6baf2
Speculative fix for alignment crash in directory iterator on Windows
Jarred-Sumner Apr 29, 2024
01b3d32
Update dir_iterator.zig
Jarred-Sumner Apr 29, 2024
07c2d23
Add comment
Jarred-Sumner Apr 29, 2024
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
18 changes: 16 additions & 2 deletions src/bun.js/node/dir_iterator.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,18 @@ pub fn NewIterator(comptime use_windows_ospath: bool) type {
}
},
.windows => struct {
const FILE_DIRECTORY_INFORMATION = std.os.windows.FILE_DIRECTORY_INFORMATION;
// While the official api docs guarantee FILE_BOTH_DIR_INFORMATION to be aligned properly
// this may not always be the case (e.g. due to faulty VM/Sandboxing tools)
const FILE_DIRECTORY_INFORMATION_PTR = *align(2) FILE_DIRECTORY_INFORMATION;
Jarred-Sumner marked this conversation as resolved.
Show resolved Hide resolved
dir: Dir,
buf: [8192]u8 align(@alignOf(os.windows.FILE_DIRECTORY_INFORMATION)),

// This structure must be aligned on a LONGLONG (8-byte) boundary.
// If a buffer contains two or more of these structures, the
// NextEntryOffset value in each entry, except the last, falls on an
// 8-byte boundary.
// https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_directory_information
buf: [8192]u8 align(8),
index: usize,
end_index: usize,
first: bool,
Expand All @@ -208,6 +218,10 @@ pub fn NewIterator(comptime use_windows_ospath: bool) type {
const w = os.windows;
if (self.index >= self.end_index) {
var io: w.IO_STATUS_BLOCK = undefined;
if (self.first) {
// > Any bytes inserted for alignment SHOULD be set to zero, and the receiver MUST ignore them
@memset(&self.buf, 0);
}

const rc = w.ntdll.NtQueryDirectoryFile(
self.dir.fd,
Expand Down Expand Up @@ -270,7 +284,7 @@ pub fn NewIterator(comptime use_windows_ospath: bool) type {
bun.sys.syslog("NtQueryDirectoryFile({}) = {d}", .{ bun.toFD(self.dir.fd), self.end_index });
}

const dir_info: *w.FILE_DIRECTORY_INFORMATION = @ptrCast(@alignCast(&self.buf[self.index]));
const dir_info: FILE_DIRECTORY_INFORMATION_PTR = @ptrCast(@alignCast(&self.buf[self.index]));
if (dir_info.NextEntryOffset != 0) {
Comment on lines +287 to 288
Copy link
Contributor

Choose a reason for hiding this comment

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

If the struct is unaligned, even if we cast alignment, do we have to worry about unaligned reads? Technically that is undefined behavior right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if the expected alignment is 2 but the alignment we're seeing is 8, then it is also 2 so that should be okay

self.index += dir_info.NextEntryOffset;
} else {
Expand Down
Loading