Skip to content
This repository has been archived by the owner on Feb 7, 2021. It is now read-only.

Fix UB when T contains uninit bytes and when T is zero-sized #4

Closed
wants to merge 4 commits into from
Closed

Conversation

TyPR124
Copy link
Contributor

@TyPR124 TyPR124 commented Feb 7, 2020

Fixes #3 by always zero'ing the storage.

@TyPR124
Copy link
Contributor Author

TyPR124 commented Feb 7, 2020

Added fix for #5

@TyPR124 TyPR124 changed the title Fix UB when T contains uninit bytes Fix UB when T contains uninit bytes and when T is zero-sized Feb 7, 2020
@TyPR124
Copy link
Contributor Author

TyPR124 commented Feb 7, 2020

Note that I'm editing this via Github's edit feature as I am away from my normal computer for a few days. That said I was able to run the tests via playground, and only the doc-tests fail but that seems to be due to playground using a different crate name.

@Lucretiel
Copy link
Owner

I'm actually not convinced of the presence of UB in the ZST case; I linked a relevant passage to the nomicon in #5.

When you get to a computer, can you add a regression test case that checks the undefined bytes case? I have a test.sh that runs miri, so the test case that triggers this behavior should be sufficient.

@TyPR124
Copy link
Contributor Author

TyPR124 commented Feb 8, 2020

I can add a regression test (and properly verify it) in the next day or two. No later than Monday.

@Lucretiel
Copy link
Owner

Sounds good. I've added a different implementation of ZST handling if that's alright; I don't really trust just feeding an 8 into a pointer and dereferencing it, so I changed new and into_inner and drop to use MaybeUninit, and as_ref to use the same path as Packed. This means that ZSTs with an alignment larger than a machine word have to be boxed, which I have zero problems with.

@TyPR124
Copy link
Contributor Author

TyPR124 commented Feb 8, 2020

This means that ZSTs with an alignment larger than a machine word have to be boxed, which I have zero problems with.

Actually, this gives me an idea. You could actually just box all ZSTs. There is no allocation for boxing ZSTs, and Box will handle the pointer semantics for you. I'm pretty sure it will generate the same (optimized) assembly as maintaining the pointer yourself (I can verify this later if you want), except you only need to worry about Box's into_raw and from_raw safety considerations, can have all ZSTs follow the Boxed codepath, and potentially eliminate the SizeClass::Zero variant and paths.

There shouldn't be any issue with de-referencing a properly aligned pointer to a ZST, since the actual read/write is still a no-op. The non-null and alignment requirements are really just to allow optimizations (example, compiler can assume that any raw pointer converted to a reference is non-null, which may allow other parts of the code to be better optimized).

Also when I get to adding the test, I will close this PR and open a new one so that these changes don't interfere with whatever ZST changes you wish to make.

@Lucretiel
Copy link
Owner

Sounds good, thank you! And that's a very interesting point about the Zero Size Box; I'd be more than happy to go that way if you can confirm that that's what happens.

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

Successfully merging this pull request may close these issues.

Undefined Behavior when T contains uninit bytes
2 participants