Skip to content

std.os.read() can fault on Linux with empty 0-length buffer provided #13776

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

Closed
moosichu opened this issue Dec 5, 2022 · 3 comments
Closed

std.os.read() can fault on Linux with empty 0-length buffer provided #13776

moosichu opened this issue Dec 5, 2022 · 3 comments
Labels
bug Observed behavior contradicts documented or intended behavior

Comments

@moosichu
Copy link
Contributor

moosichu commented Dec 5, 2022

Zig Version

0.11.0-dev.514+4be1bb4aa

Steps to Reproduce and Observed Behavior

test "read with empty buffer" {
    var bytes = try std.testing.allocator.alloc(u8, 0);
    defer std.testing.allocator.free(bytes);
    var file = try std.fs.cwd().openFile("build.zig",  .{ .mode = .read_only });
    defer file.close();
    _ = try std.os.read(file.handle, bytes);
}

This panics:

panic: reached unreachable code
lib/zig/std/os.zig:705:23: 0x20cfb7 in read (test)
            .FAULT => unreachable,

Expected Behavior

Based on how zig otherwise deals with 0 values, I would expect this case to be gracefully handled (.i.e be a no-op). Otherwise, it should be clearly documented that 0 is an invalid argument in std.os.read().

@moosichu moosichu added the bug Observed behavior contradicts documented or intended behavior label Dec 5, 2022
@moosichu moosichu changed the title std.os.read() can fault on Linux with empty buffer provided std.os.read() can fault on Linux with empty 0-length buffer provided Dec 5, 2022
@moosichu
Copy link
Contributor Author

moosichu commented Dec 5, 2022

I'm putting together a PR that resolves this now (along with test).

@moosichu
Copy link
Contributor Author

moosichu commented Dec 5, 2022

Reading through the documentation on this, things are a little confusing, looking at https://linux.die.net/man/3/read :

Before any action described below is taken, and if nbyte is zero, the read() function may detect and return errors as described below. In the absence of errors, or if error detection is not performed, the read() function shall return zero and have no other results.

This volume of IEEE Std 1003.1-2001 requires that no action be taken for read() or write() when nbyte is zero. This is not intended to take precedence over detection of errors (such as invalid buffer pointers or file descriptors). This is consistent with the rest of this volume of IEEE Std 1003.1-2001, but the phrasing here could be misread to require detection of the zero case before any other errors. A value of zero is to be considered a correct value, for which the semantics are a no-op.

Which to me implies that we are passing through an invalid pointer which is being checked, but having read through the code, as far as I'm aware std.testing.allocator seems to be returning a valid pointer? So this is a little confusing.

As general_purpose_allocator.zig has this in allocInner():

const new_aligned_size = @max(len, @as(usize, 1) << @intCast(Allocator.Log2Align, log2_ptr_align));
...
const new_size_class = math.ceilPowerOfTwoAssert(usize, new_aligned_size);
const ptr = try self.allocSlot(new_size_class, ret_addr);
if (config.verbose_log) {
    log.info("small alloc {d} bytes at {*}", .{ len, ptr });
}
return ptr;

Which to me implies a valid slot is always being returned. So unless something else is invalidating the pointer I'm not aware of, this shouldn't be erroring. So maybe a fix isn't needed in std.os specifically?

@Vexu
Copy link
Member

Vexu commented Dec 5, 2022

Duplicate of #11604

@Vexu Vexu marked this as a duplicate of #11604 Dec 5, 2022
@Vexu Vexu closed this as completed Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior
Projects
None yet
Development

No branches or pull requests

2 participants