authorgravatar for squeek502@hotmail.comRyan Liptak <squeek502@hotmail.com> 2023-07-22 16:52:01-07:00
committergravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2023-07-23 19:09:20-07:00
log3f7166eb8bd714b412889d8ded71fd839111c624
treee3e2e41c75e70ecbce184888b43795c81d9d0ac0
parent7dcbabef512403382f4647e2c7cc0c900ecae913

child_process: Fix regression on Windows for FAT filesystems

This fixes a regression caused by https://github.com/ziglang/zig/pull/13993 As an optimization, the first call to `NtQueryDirectoryFile` would only ask for a single result and assume that if the result returned did not match the app_name exactly, then the unappended app_name did not exist. However, this relied on the assumption that the unappended app_name would always be returned first, but that only seems to be the case on NTFS. On FAT filesystems, the order of returned files can be different, which meant that it could assume the unappended file doesn't exist when it actually does. This commit fixes that by fully iterating the wildcard matches via `NtQueryDirectoryFile` and taking note of any unappended/PATHEXT-appended filenames it finds. In practice, this strategy does not introduce a speed regression compared to the previous (buggy) implementation. Benchmark 1 (10 runs): winpathbench-master.exe measurement mean ± σ min … max outliers delta wall_time 508ms ± 4.08ms 502ms … 517ms 1 (10%) 0% peak_rss 3.62MB ± 2.76KB 3.62MB … 3.63MB 0 ( 0%) 0% Benchmark 2 (10 runs): winpathbench-fat32-fix.exe measurement mean ± σ min … max outliers delta wall_time 500ms ± 21.4ms 480ms … 535ms 0 ( 0%) - 1.5% ± 2.8% peak_rss 3.62MB ± 2.76KB 3.62MB … 3.63MB 0 ( 0%) - 0.0% ± 0.1% --- Partially addresses #16374 (it fixes `zig build` on FAT32 when no `zig-cache` is present)

2 files changed, 97 insertions(+), 90 deletions(-)

lib/std/child_process.zig+77-90
...@@ -952,8 +952,9 @@ fn windowsCreateProcessPathExt(...@@ -952,8 +952,9 @@ fn windowsCreateProcessPathExt(
952 // for any directory that doesn't contain any possible matches, instead of having952 // for any directory that doesn't contain any possible matches, instead of having
953 // to use a separate look up for each individual filename combination (unappended +953 // to use a separate look up for each individual filename combination (unappended +
954 // each PATHEXT appended). For directories where the wildcard *does* match something,954 // each PATHEXT appended). For directories where the wildcard *does* match something,
955 // we only need to do a maximum of <number of supported PATHEXT extensions> more955 // we iterate the matches and take note of any that are either the unappended version,
956 // NtQueryDirectoryFile calls.956 // or a version with a supported PATHEXT appended. We then try calling CreateProcessW
957 // with the found versions in the appropriate order.
957958
958 var dir = dir: {959 var dir = dir: {
959 // needs to be null-terminated960 // needs to be null-terminated
...@@ -970,11 +971,26 @@ fn windowsCreateProcessPathExt(...@@ -970,11 +971,26 @@ fn windowsCreateProcessPathExt(
970 try app_buf.append(allocator, 0);971 try app_buf.append(allocator, 0);
971 const app_name_wildcard = app_buf.items[0 .. app_buf.items.len - 1 :0];972 const app_name_wildcard = app_buf.items[0 .. app_buf.items.len - 1 :0];
972973
973 // Enough for the FILE_DIRECTORY_INFORMATION + (NAME_MAX UTF-16 code units [2 bytes each]).974 // This 2048 is arbitrary, we just want it to be large enough to get multiple FILE_DIRECTORY_INFORMATION entries
974 const file_info_buf_size = @sizeOf(windows.FILE_DIRECTORY_INFORMATION) + (windows.NAME_MAX * 2);975 // returned per NtQueryDirectoryFile call.
975 var file_information_buf: [file_info_buf_size]u8 align(@alignOf(os.windows.FILE_DIRECTORY_INFORMATION)) = undefined;976 var file_information_buf: [2048]u8 align(@alignOf(os.windows.FILE_DIRECTORY_INFORMATION)) = undefined;
977 const file_info_maximum_single_entry_size = @sizeOf(windows.FILE_DIRECTORY_INFORMATION) + (windows.NAME_MAX * 2);
978 if (file_information_buf.len < file_info_maximum_single_entry_size) {
979 @compileError("file_information_buf must be large enough to contain at least one maximum size FILE_DIRECTORY_INFORMATION entry");
980 }
976 var io_status: windows.IO_STATUS_BLOCK = undefined;981 var io_status: windows.IO_STATUS_BLOCK = undefined;
977 const found_name: ?[]const u16 = found_name: {982
983 const num_supported_pathext = @typeInfo(CreateProcessSupportedExtension).Enum.fields.len;
984 var pathext_seen = [_]bool{false} ** num_supported_pathext;
985 var any_pathext_seen = false;
986 var unappended_exists = false;
987
988 // Fully iterate the wildcard matches via NtQueryDirectoryFile and take note of all versions
989 // of the app_name we should try to spawn.
990 // Note: This is necessary because the order of the files returned is filesystem-dependent:
991 // On NTFS, `blah.exe*` will always return `blah.exe` first if it exists.
992 // On FAT32, it's possible for something like `blah.exe.obj` to be returned first.
993 while (true) {
978 const app_name_len_bytes = math.cast(u16, app_name_wildcard.len * 2) orelse return error.NameTooLong;994 const app_name_len_bytes = math.cast(u16, app_name_wildcard.len * 2) orelse return error.NameTooLong;
979 var app_name_unicode_string = windows.UNICODE_STRING{995 var app_name_unicode_string = windows.UNICODE_STRING{
980 .Length = app_name_len_bytes,996 .Length = app_name_len_bytes,
...@@ -990,18 +1006,9 @@ fn windowsCreateProcessPathExt(...@@ -990,18 +1006,9 @@ fn windowsCreateProcessPathExt(
990 &file_information_buf,1006 &file_information_buf,
991 file_information_buf.len,1007 file_information_buf.len,
992 .FileDirectoryInformation,1008 .FileDirectoryInformation,
993 // TODO: It might be better to iterate over all wildcard matches and1009 windows.FALSE, // single result
994 // only pick the ones that match an appended PATHEXT instead of only
995 // using the wildcard as a lookup and then restarting iteration
996 // on future NtQueryDirectoryFile calls.
997 //
998 // However, note that this could lead to worse outcomes in the
999 // case of a very generic command name (e.g. "a"), so it might
1000 // be better to only use the wildcard to determine if it's worth
1001 // checking with PATHEXT (this is the current behavior).
1002 windows.TRUE, // single result
1003 &app_name_unicode_string,1010 &app_name_unicode_string,
1004 windows.TRUE, // restart iteration1011 windows.FALSE, // restart iteration
1005 );1012 );
10061013
1007 // If we get nothing with the wildcard, then we can just bail out1014 // If we get nothing with the wildcard, then we can just bail out
...@@ -1009,25 +1016,36 @@ fn windowsCreateProcessPathExt(...@@ -1009,25 +1016,36 @@ fn windowsCreateProcessPathExt(
1009 switch (rc) {1016 switch (rc) {
1010 .SUCCESS => {},1017 .SUCCESS => {},
1011 .NO_SUCH_FILE => return error.FileNotFound,1018 .NO_SUCH_FILE => return error.FileNotFound,
1012 .NO_MORE_FILES => return error.FileNotFound,1019 .NO_MORE_FILES => break,
1013 .ACCESS_DENIED => return error.AccessDenied,1020 .ACCESS_DENIED => return error.AccessDenied,
1014 else => return windows.unexpectedStatus(rc),1021 else => return windows.unexpectedStatus(rc),
1015 }1022 }
10161023
1017 const dir_info = @as(*windows.FILE_DIRECTORY_INFORMATION, @ptrCast(&file_information_buf));1024 // According to the docs, this can only happen if there is not enough room in the
1018 if (dir_info.FileAttributes & windows.FILE_ATTRIBUTE_DIRECTORY != 0) {1025 // buffer to write at least one complete FILE_DIRECTORY_INFORMATION entry.
1019 break :found_name null;1026 // Therefore, this condition should not be possible to hit with the buffer size we use.
1027 std.debug.assert(io_status.Information != 0);
1028
1029 var it = windows.FileInformationIterator(windows.FILE_DIRECTORY_INFORMATION){ .buf = &file_information_buf };
1030 while (it.next()) |info| {
1031 // Skip directories
1032 if (info.FileAttributes & windows.FILE_ATTRIBUTE_DIRECTORY != 0) continue;
1033 const filename = @as([*]u16, @ptrCast(&info.FileName))[0 .. info.FileNameLength / 2];
1034 // Because all results start with the app_name since we're using the wildcard `app_name*`,
1035 // if the length is equal to app_name then this is an exact match
1036 if (filename.len == app_name_len) {
1037 // Note: We can't break early here because it's possible that the unappended version
1038 // fails to spawn, in which case we still want to try the PATHEXT appended versions.
1039 unappended_exists = true;
1040 } else if (windowsCreateProcessSupportsExtension(filename[app_name_len..])) |pathext_ext| {
1041 pathext_seen[@intFromEnum(pathext_ext)] = true;
1042 any_pathext_seen = true;
1043 }
1020 }1044 }
1021 break :found_name @as([*]u16, @ptrCast(&dir_info.FileName))[0 .. dir_info.FileNameLength / 2];1045 }
1022 };
10231046
1024 const unappended_err = unappended: {1047 const unappended_err = unappended: {
1025 // NtQueryDirectoryFile returns results in order by filename, so the first result of1048 if (unappended_exists) {
1026 // the wildcard call will always be the unappended version if it exists. So, if found_name
1027 // is not the unappended version, we can skip straight to trying versions with PATHEXT appended.
1028 // TODO: This might depend on the filesystem, though; need to somehow verify that it always
1029 // works this way.
1030 if (found_name != null and windows.eqlIgnoreCaseWTF16(found_name.?, app_buf.items[0..app_name_len])) {
1031 if (dir_path_len != 0) switch (dir_buf.items[dir_buf.items.len - 1]) {1049 if (dir_path_len != 0) switch (dir_buf.items[dir_buf.items.len - 1]) {
1032 '/', '\\' => {},1050 '/', '\\' => {},
1033 else => try dir_buf.append(allocator, fs.path.sep),1051 else => try dir_buf.append(allocator, fs.path.sep),
...@@ -1060,52 +1078,13 @@ fn windowsCreateProcessPathExt(...@@ -1060,52 +1078,13 @@ fn windowsCreateProcessPathExt(
1060 break :unappended error.FileNotFound;1078 break :unappended error.FileNotFound;
1061 };1079 };
10621080
1063 // Now we know that at least *a* file matching the wildcard exists, we can loop1081 if (!any_pathext_seen) return unappended_err;
1064 // through PATHEXT in order and exec any that exist
10651082
1083 // Now try any PATHEXT appended versions that we've seen
1066 var ext_it = mem.tokenizeScalar(u16, pathext, ';');1084 var ext_it = mem.tokenizeScalar(u16, pathext, ';');
1067 while (ext_it.next()) |ext| {1085 while (ext_it.next()) |ext| {
1068 if (!windowsCreateProcessSupportsExtension(ext)) continue;1086 const ext_enum = windowsCreateProcessSupportsExtension(ext) orelse continue;
10691087 if (!pathext_seen[@intFromEnum(ext_enum)]) continue;
1070 app_buf.shrinkRetainingCapacity(app_name_len);
1071 try app_buf.appendSlice(allocator, ext);
1072 try app_buf.append(allocator, 0);
1073 const app_name_appended = app_buf.items[0 .. app_buf.items.len - 1 :0];
1074
1075 const app_name_len_bytes = math.cast(u16, app_name_appended.len * 2) orelse return error.NameTooLong;
1076 var app_name_unicode_string = windows.UNICODE_STRING{
1077 .Length = app_name_len_bytes,
1078 .MaximumLength = app_name_len_bytes,
1079 .Buffer = @constCast(app_name_appended.ptr),
1080 };
1081
1082 // Re-use the directory handle but this time we call with the appended app name
1083 // with no wildcard.
1084 const rc = windows.ntdll.NtQueryDirectoryFile(
1085 dir.fd,
1086 null,
1087 null,
1088 null,
1089 &io_status,
1090 &file_information_buf,
1091 file_information_buf.len,
1092 .FileDirectoryInformation,
1093 windows.TRUE, // single result
1094 &app_name_unicode_string,
1095 windows.TRUE, // restart iteration
1096 );
1097
1098 switch (rc) {
1099 .SUCCESS => {},
1100 .NO_SUCH_FILE => continue,
1101 .NO_MORE_FILES => continue,
1102 .ACCESS_DENIED => continue,
1103 else => return windows.unexpectedStatus(rc),
1104 }
1105
1106 const dir_info = @as(*windows.FILE_DIRECTORY_INFORMATION, @ptrCast(&file_information_buf));
1107 // Skip directories
1108 if (dir_info.FileAttributes & windows.FILE_ATTRIBUTE_DIRECTORY != 0) continue;
11091088
1110 dir_buf.shrinkRetainingCapacity(dir_path_len);1089 dir_buf.shrinkRetainingCapacity(dir_path_len);
1111 if (dir_path_len != 0) switch (dir_buf.items[dir_buf.items.len - 1]) {1090 if (dir_path_len != 0) switch (dir_buf.items[dir_buf.items.len - 1]) {
...@@ -1170,9 +1149,17 @@ fn windowsCreateProcess(app_name: [*:0]u16, cmd_line: [*:0]u16, envp_ptr: ?[*]u1...@@ -1170,9 +1149,17 @@ fn windowsCreateProcess(app_name: [*:0]u16, cmd_line: [*:0]u16, envp_ptr: ?[*]u1
1170 );1149 );
1171}1150}
11721151
1152// Should be kept in sync with `windowsCreateProcessSupportsExtension`
1153const CreateProcessSupportedExtension = enum {
1154 bat,
1155 cmd,
1156 com,
1157 exe,
1158};
1159
1173/// Case-insensitive UTF-16 lookup1160/// Case-insensitive UTF-16 lookup
1174fn windowsCreateProcessSupportsExtension(ext: []const u16) bool {1161fn windowsCreateProcessSupportsExtension(ext: []const u16) ?CreateProcessSupportedExtension {
1175 if (ext.len != 4) return false;1162 if (ext.len != 4) return null;
1176 const State = enum {1163 const State = enum {
1177 start,1164 start,
1178 dot,1165 dot,
...@@ -1188,50 +1175,50 @@ fn windowsCreateProcessSupportsExtension(ext: []const u16) bool {...@@ -1188,50 +1175,50 @@ fn windowsCreateProcessSupportsExtension(ext: []const u16) bool {
1188 for (ext) |c| switch (state) {1175 for (ext) |c| switch (state) {
1189 .start => switch (c) {1176 .start => switch (c) {
1190 '.' => state = .dot,1177 '.' => state = .dot,
1191 else => return false,1178 else => return null,
1192 },1179 },
1193 .dot => switch (c) {1180 .dot => switch (c) {
1194 'b', 'B' => state = .b,1181 'b', 'B' => state = .b,
1195 'c', 'C' => state = .c,1182 'c', 'C' => state = .c,
1196 'e', 'E' => state = .e,1183 'e', 'E' => state = .e,
1197 else => return false,1184 else => return null,
1198 },1185 },
1199 .b => switch (c) {1186 .b => switch (c) {
1200 'a', 'A' => state = .ba,1187 'a', 'A' => state = .ba,
1201 else => return false,1188 else => return null,
1202 },1189 },
1203 .c => switch (c) {1190 .c => switch (c) {
1204 'm', 'M' => state = .cm,1191 'm', 'M' => state = .cm,
1205 'o', 'O' => state = .co,1192 'o', 'O' => state = .co,
1206 else => return false,1193 else => return null,
1207 },1194 },
1208 .e => switch (c) {1195 .e => switch (c) {
1209 'x', 'X' => state = .ex,1196 'x', 'X' => state = .ex,
1210 else => return false,1197 else => return null,
1211 },1198 },
1212 .ba => switch (c) {1199 .ba => switch (c) {
1213 't', 'T' => return true, // .BAT1200 't', 'T' => return .bat,
1214 else => return false,1201 else => return null,
1215 },1202 },
1216 .cm => switch (c) {1203 .cm => switch (c) {
1217 'd', 'D' => return true, // .CMD1204 'd', 'D' => return .cmd,
1218 else => return false,1205 else => return null,
1219 },1206 },
1220 .co => switch (c) {1207 .co => switch (c) {
1221 'm', 'M' => return true, // .COM1208 'm', 'M' => return .com,
1222 else => return false,1209 else => return null,
1223 },1210 },
1224 .ex => switch (c) {1211 .ex => switch (c) {
1225 'e', 'E' => return true, // .EXE1212 'e', 'E' => return .exe,
1226 else => return false,1213 else => return null,
1227 },1214 },
1228 };1215 };
1229 return false;1216 return null;
1230}1217}
12311218
1232test "windowsCreateProcessSupportsExtension" {1219test "windowsCreateProcessSupportsExtension" {
1233 try std.testing.expect(windowsCreateProcessSupportsExtension(&[_]u16{ '.', 'e', 'X', 'e' }));1220 try std.testing.expectEqual(CreateProcessSupportedExtension.exe, windowsCreateProcessSupportsExtension(&[_]u16{ '.', 'e', 'X', 'e' }).?);
1234 try std.testing.expect(!windowsCreateProcessSupportsExtension(&[_]u16{ '.', 'e', 'X', 'e', 'c' }));1221 try std.testing.expect(windowsCreateProcessSupportsExtension(&[_]u16{ '.', 'e', 'X', 'e', 'c' }) == null);
1235}1222}
12361223
1237/// Caller must dealloc.1224/// Caller must dealloc.
lib/std/os/windows.zig+20
...@@ -4306,6 +4306,26 @@ pub const FILE_BOTH_DIR_INFORMATION = extern struct {...@@ -4306,6 +4306,26 @@ pub const FILE_BOTH_DIR_INFORMATION = extern struct {
4306};4306};
4307pub const FILE_BOTH_DIRECTORY_INFORMATION = FILE_BOTH_DIR_INFORMATION;4307pub const FILE_BOTH_DIRECTORY_INFORMATION = FILE_BOTH_DIR_INFORMATION;
43084308
4309/// Helper for iterating a byte buffer of FILE_*_INFORMATION structures (from
4310/// things like NtQueryDirectoryFile calls).
4311pub fn FileInformationIterator(comptime FileInformationType: type) type {
4312 return struct {
4313 byte_offset: usize = 0,
4314 buf: []u8 align(@alignOf(FileInformationType)),
4315
4316 pub fn next(self: *@This()) ?*FileInformationType {
4317 if (self.byte_offset >= self.buf.len) return null;
4318 const cur: *FileInformationType = @ptrCast(@alignCast(&self.buf[self.byte_offset]));
4319 if (cur.NextEntryOffset == 0) {
4320 self.byte_offset = self.buf.len;
4321 } else {
4322 self.byte_offset += cur.NextEntryOffset;
4323 }
4324 return cur;
4325 }
4326 };
4327}
4328
4309pub const IO_APC_ROUTINE = *const fn (PVOID, *IO_STATUS_BLOCK, ULONG) callconv(.C) void;4329pub const IO_APC_ROUTINE = *const fn (PVOID, *IO_STATUS_BLOCK, ULONG) callconv(.C) void;
43104330
4311pub const CURDIR = extern struct {4331pub const CURDIR = extern struct {