Skip to content

std: further windows resource fix #15460

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

Merged
merged 1 commit into from
Apr 25, 2023
Merged

Conversation

xEgoist
Copy link
Contributor

@xEgoist xEgoist commented Apr 25, 2023

addition to #15450
createFileW does not account for when LockFile fails. This can result in a file handle not being closed on failure Which can be seen on test such as open file with exclusive nonblocking lock twice here:

zig/lib/std/fs/test.zig

Lines 1030 to 1050 in d8bdfd8

test "open file with exclusive nonblocking lock twice" {
if (builtin.os.tag == .wasi) return error.SkipZigTest;
const filename = "file_nonblocking_lock_test.txt";
var tmp = tmpDir(.{});
defer tmp.cleanup();
const file1 = try tmp.dir.createFile(filename, .{ .lock = .Exclusive, .lock_nonblocking = true });
defer file1.close();
const file2 = tmp.dir.createFile(filename, .{ .lock = .Exclusive, .lock_nonblocking = true });
try testing.expectError(error.WouldBlock, file2);
}
test "open file with shared and exclusive nonblocking lock" {
if (builtin.os.tag == .wasi) return error.SkipZigTest;
const filename = "file_nonblocking_lock_test.txt";
var tmp = tmpDir(.{});

this can also be observed by finding the file specified in the test in the zig-cache due to failure in cleanup.

addition to ziglang#15450
createFileW does not account for failure on `LockFile`. This can result
in a file handle not being closed on failure which can be seen on test
such as `fs.test.'open file with exclusive nonblocking lock twice'`.
Copy link
Member

@jacobly0 jacobly0 left a comment

Choose a reason for hiding this comment

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

Thanks, I didn't notice there was another one. It would still be nice to fix the infinite loop in deleteTree that this resource leak happens to trigger though.

@Vexu Vexu enabled auto-merge (rebase) April 25, 2023 17:10
@andrewrk andrewrk disabled auto-merge April 25, 2023 17:37
@andrewrk andrewrk merged commit f780a6b into ziglang:master Apr 25, 2023
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.

4 participants