Skip to content

Add 0-length buffer checks to os.read & os.write #13885

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
Dec 12, 2022

Conversation

moosichu
Copy link
Contributor

This helps prevent errors related to undefined pointers being passed through to some OS apis when slices have 0 length.

Tests have also been added to catch these cases.

@moosichu
Copy link
Contributor Author

This helps with some cases from #11604 - although I only checked the common cases of the read & write procedures, and haven't exhaustively checked all the procedures instd.os. So whilst I think merging is worthwhile as an improvement to stdlib - it might be worth someone conclusively checking all the std.os calls before closing that issue.

@moosichu moosichu force-pushed the fix/empty-file-read branch from 301d91c to b241665 Compare December 11, 2022 19:11
@wooster0
Copy link
Contributor

wooster0 commented Dec 11, 2022

So if buffers of length zero are not supposed to be passed, I wonder if assert(buf.len != 0) would do the trick too? That way in release mode under correct circumstances there won't be an additional if check.

Not sure though; maybe it should be fine to pass empty buffers. With an assert it'd be on the user to make sure they don't pass empty buffers.
Practically though it doesn't make much sense to write or read nothing?

These are lower level functions though so maybe an assert is better.

@moosichu
Copy link
Contributor Author

So if buffers of length zero are not supposed to be passed

It's a valid use-case - the "0-case" working in zig is baked into the ethos of the language from 0-bit types upwards - it makes generalising code a lot simpler than it would be otherwise.

It's worth pointing out that underlying OS apis do allow for buffers of 0 length, just in the case of linux the memory pointer must be valid, even if the buffer length is 0 - so this is a problem with the way 0-length zig slices interface with that api specifically (as far as I understand). So needs to be manually accounted for in this case.

There is no assert available in zig for "is this a valid buffer pointer" - which is what you would want if you wanted to assert.

Also this might not be consistent across different operating systems - so ensure it is from within zig's abstraction layer helps ensure consistency and prevent unexpected bugs when dealing with different targets as well.

That way in release mode under correct circumstances there won't be an additional if check.

When performing an api read call - the order of magnitude of overhead absolute dwarfs the cost of a single if check that will most-probably be correctly be handled by the cpu's branch predictor anyway.

Practically though it doesn't make much sense to write or read nothing?

In theory - but there might be cases where you are generalising over buffer sizes that included empty buffers - and having the os api calls no-op instead of resulting in undefined behaviour greatly simplifies that kind of code.

This helps prevent errors related to undefined pointers being passed
through to some OS apis when slices have 0 length.

Tests have also been added to catch these cases.
@moosichu moosichu force-pushed the fix/empty-file-read branch from b241665 to c49afdb Compare December 11, 2022 20:45
@moosichu
Copy link
Contributor Author

All checks seem to have passed now after I fixed-up the tests. :)

Keen to have this one merged as it's blocking development of zig-ar.

@andrewrk andrewrk merged commit d0eef26 into ziglang:master Dec 12, 2022
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.

3 participants