Check for lengths in ULE and revise safety docs #1121
Conversation
utils/zerovec/src/ule/mod.rs
Outdated
| Self::validate_byte_slice(bytes).map_err(|e| UleError::InvalidBytes(e))?; | ||
| if bytes.len() % mem::size_of::<Self>() != 0 { | ||
| return Err(UleError::InvalidLength); | ||
| } |
There was a problem hiding this comment.
question (n-b): In my patches I put this test in validate_byte_slice. This addition means that you don't want the check of length being divisible by size be in validate_byte_slice?
There was a problem hiding this comment.
Yeah, I would imagine that this is the job of validate_byte_slice. We could debug_assert here, but I think it's better if the validation is in one place.
There was a problem hiding this comment.
It's easy to check the invariant in the default impl, and relying on validate_byte_slice is prone to error.
There was a problem hiding this comment.
I made a separate error type with the check in parse_byte_slice because:
- It's not possible to verify that
validate_byte_sliceperforms this check (debug_assert only works if there is a code path) - We can perform the check automatically, so it seems like we should
- It's easy to miss the check in code review
There was a problem hiding this comment.
Right, but "it's not possible to verify X" is the standard state of all unsafe code. My experience with Rust unsafe is that it's far better to consolidate the unsafe requirements in one well-documented place. This is basically splitting out the responsibilities and making it trickier to reason about: now validate_byte_slice is in charge of validating the slice only in some ways.
"We can perform this check so we should" is why I'm suggesting debug asserts, we should absolutely be sprinkling the code with more of these.
I think it's table stakes for Rust unsafe code review for folks to be verifying that the function is defined according to the checklist, if reviewers aren't doing that that's a much bigger problem.
Perhaps we should more strongly require comments that ensure the checklists are satisfied. We require safety explainer comments, but in the case of unsafe traits, we could require checklists and require that these checklists be proven for each impl.
There was a problem hiding this comment.
Okay, I'm convinced. I changed it to a bullet point in the checklist.
There was a problem hiding this comment.
It turns out that there is another problem here. What error type should PlainOldULE and and CharULE return when the lengths are not correct? In my previous draft, I had them returning UleError (as the default), but now that I've gotten rid of that, I don't know what error they should return. In an effort to not block this otherwise productive PR with documentation changes, I filed a follow-up issue for this: #1144
utils/zerovec/src/ule/mod.rs
Outdated
| Self::validate_byte_slice(bytes).map_err(|e| UleError::InvalidBytes(e))?; | ||
| if bytes.len() % mem::size_of::<Self>() != 0 { | ||
| return Err(UleError::InvalidLength); | ||
| } |
There was a problem hiding this comment.
Yeah, I would imagine that this is the job of validate_byte_slice. We could debug_assert here, but I think it's better if the validation is in one place.
Check for lengths in ULE and revise safety docs
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
|
I believe I addressed everyone's feedback. I changed back to a single error type. PTAL and feel free to merge. |
I realized that with the new byte equality (#1103), we can no longer ignore extra bytes at the end of a byte slice. I also revised the safety docs to make them more actionable by adding a checklist.