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

Fix/crashing when draining empty minivec small #19

Merged
merged 1 commit into from
Feb 18, 2021
Merged

Fix/crashing when draining empty minivec small #19

merged 1 commit into from
Feb 18, 2021

Conversation

hbina
Copy link
Contributor

@hbina hbina commented Feb 12, 2021

This is a much shorter that prevents the crash.
However, I think there's still an underlying problem with using NonNull on a NULL data.

@cmazakas
Copy link
Owner

cmazakas commented Feb 12, 2021

Does this one obsolete #18?

Do you think we might need to use NonNull::dangling() here?
https://github.com/LeonineKing1199/minivec/blob/29caf18f28cb09cd2083103f283a9666a700b2b4/src/impl/drain.rs#L23-L24

@hbina
Copy link
Contributor Author

hbina commented Feb 13, 2021

This is just a fix that prevents the crash, but still invoking UB because it violates NonNull usage.

I'll finish reading https://doc.rust-lang.org/nomicon/subtyping.html first but I think dangling might be the proper use. I don't understand why that other PR changes the covariance thing :/

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 13, 2021
…-vec, r=dtolnay

Added tests to drain an empty vec

Discovered this kind of issue in an unrelated library.
The author copied the tests from here and AFAIK, there are no tests for this particular case.

cmazakas/minivec#19

Signed-off-by: Hanif Bin Ariffin <[email protected]>
@cmazakas
Copy link
Owner

This is just a fix that prevents the crash, but still invoking UB because it violates NonNull usage.

To be fair, it's actually fine for NonNull to point to junk.

The type system uses this information to optimize where appropriate and it also has effects with regards to covariance (which I'm honestly still figuring out as a Rust noob).

@cmazakas
Copy link
Owner

Alright, this looks good and works on my end.

Merging.

Thank you for the contribution. I appreciate it.

@cmazakas cmazakas merged commit 09fd54a into cmazakas:develop Feb 18, 2021
@hbina hbina deleted the fix/crashing-when-draining-empty-minivec-small branch February 18, 2021 23:18
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.

2 participants