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.

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

Writing to uninitialized buffer #90

Closed
Kixunil opened this issue Jun 4, 2017 · 10 comments
Closed

Writing to uninitialized buffer #90

Kixunil opened this issue Jun 4, 2017 · 10 comments
Labels

Comments

@Kixunil
Copy link

Kixunil commented Jun 4, 2017

write_* methods of ByteOrder trait accept a buffer and don't guarantee that they wouldn't read from it. This has a drawback that strictly speaking, the provided buffer shouldn't be uninitialized.

I suggest to provide some way of guaranteeing that the buffer won't be read from, so it's fine to pass uninitialized buffer.

@BurntSushi
Copy link
Owner

This seems reasonable to me. I can't imagine any circumstances under which the write methods would actually want to read from the buffer. Can you?

@Kixunil
Copy link
Author

Kixunil commented Jun 4, 2017

I can't. Would it be breaking change to make the trait unsafe? If the only implementors are LittleEndian and BigEndian, then it should be fine...

@Kixunil
Copy link
Author

Kixunil commented Jun 27, 2017

Ping?

@BurntSushi
Copy link
Owner

I don't think it would be a breaking change.

Why does the standard io::Read trait not make this guarantee?

@Kixunil
Copy link
Author

Kixunil commented Jul 11, 2017

There's a PR addressing this.

@BurntSushi
Copy link
Owner

I see. So it seems like the preferred solution is to make the trait unsafe, but we couldn't do that for std::io::Read. However, I believe we can here since the trait is sealed.

@Kixunil
Copy link
Author

Kixunil commented Jul 11, 2017

Exactly. I'd submit a PR, but I'm currently low on time. I expect to have more time after ~2 weeks.

@Kixunil
Copy link
Author

Kixunil commented Sep 8, 2017

Sorry, I forgot about this completely. I'll look at it today (not sooner than 9 hours from now).

@BurntSushi
Copy link
Owner

I think that if you're already dealing with unsafe code and uninitialized buffers, then it probably doesn't make sense to use byteorder's safe APIs, since they impose a cost.

@Kixunil
Copy link
Author

Kixunil commented Jan 7, 2021

This is weird, I completely forgot about this. I do remember trying to add unsafe but something stopped me and then I didn't continue for some reason. Can't remember anything more. I guess it's a good thing because MaybeUninit was introduced since then and it doesn't make sense to guarantee anything about uninitialized &mut [u8] as that's UB already.

No change is actually required because performant writing can be done safely already: https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=26babbb49afda7644f61f4e7ffe1ad2c

TL;DR is that writing to a safe, 0-init array first and then copying it into uninit slice gets optimized just fine.
It could be nice to guarantee that unsafe code can rely on values being written as expected but I personally wouldn't consider unsafe code being broken if it relies on it implicitly as that's a reasonable assumption.

Maybe a more appropriate label would be magically-fixed-itself instead of wontfix. 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants