Avoid certain panics in decoding. #83
Conversation
There was a problem hiding this comment.
Looks good, @partim, but I'd suggest taking a slightly different approach to failing silently when unexpected conditions occur. I'd at least suggest sprinkling debug_assert!() all over the place -- it's the same tool one would use when writing unsafe code, which can fail worse than silently.
| if self.source.request(remaining)? < remaining { | ||
| Err(self.source.content_err("unexpected end of data")) | ||
| } | ||
| else { | ||
| Ok(&self.source.slice()[..remaining]) | ||
| return Err(self.source.content_err("unexpected end of data")) | ||
| } | ||
| self.source.slice().get(..remaining).ok_or_else(|| { | ||
| self.source.content_err("unexpected end of data") | ||
| }) |
There was a problem hiding this comment.
At a cursory glance, the checked get makes the initial check redundant, right? You should be able to simply omit it -- if not enough data is retrieved, the get will fail.
| self.source.limit().unwrap() | ||
| // The source is guaranteed to have a limit by the code creating | ||
| // the primitive, so we can just return 0 here if it isn’t. This | ||
| // isn’t ideal and we would rater guarantee things through types, |
| pos: usize, | ||
|
|
||
| /// The offset for the reported position. | ||
| /// | ||
| /// This is the value reported by `Source::pos` when `self.pos` is zero. | ||
| /// This is 0, unles the value was created with `Self::with_offset`. |
| data[i] = source.slice().get(i).copied().ok_or_else(|| { | ||
| source.content_err("source returned short slice") | ||
| })?; |
There was a problem hiding this comment.
Given how much you need to check that the Source is working correctly, perhaps it would be worthwhile to change its API to be less fallible.
There was a problem hiding this comment.
I have a plan to rewrite both Tag and Length in a follow-up PR to be more robust. Should have mentioned that in the PR comment but forgot.
| // Unwrapping here is okay. The only error that can happen is that | ||
| // the tag is longer that we support. However, we already checked that | ||
| // there’s only OctetString or End of Value tags which we _do_ | ||
| // support. | ||
| // Taking the tag and length shouldn’t fail since we checked already, | ||
| // so just returning `None` should be fine. |
There was a problem hiding this comment.
Does this mean that a bug here would lead to a silent/hidden failure rather than a panic? Maybe it would be good to leave a debug_assert!() or similar here, so that you have some avenue of detecting these bugs. That might apply to the whole PR.
There was a problem hiding this comment.
I am quite unhappy with all the string type handling, so I’ll probably do a rewrite here as well.
| assert_eq!(make([0]).as_slice(), b"\0"); | ||
| assert_eq!(make([0, 0]).as_slice(), b"\0"); | ||
|
|
||
| assert_eq!(make([0x10]).as_slice(), b"\x10"); | ||
| assert_eq!(make([0, 0x10]).as_slice(), b"\x10"); | ||
| assert_eq!(make([0, 0, 0x10]).as_slice(), b"\x10"); | ||
|
|
||
| assert_eq!(make([0x10, 0xF0]).as_slice(), b"\x10\xF0"); | ||
| assert_eq!(make([0, 0x10, 0xF0]).as_slice(), b"\x10\xF0"); | ||
| assert_eq!(make([0, 0, 0x10, 0xF0]).as_slice(), b"\x10\xF0"); | ||
|
|
||
| assert_eq!(make([0xF0]).as_slice(), b"\0\xF0"); | ||
| assert_eq!(make([0, 0xF0]).as_slice(), b"\0\xF0"); | ||
| assert_eq!(make([0, 0, 0xF0]).as_slice(), b"\0\xF0"); | ||
|
|
||
| assert_eq!(make([0xF0, 0xF0]).as_slice(), b"\0\xF0\xF0"); | ||
| assert_eq!(make([0, 0xF0, 0xF0]).as_slice(), b"\0\xF0\xF0"); | ||
| assert_eq!(make([0, 0, 0xF0, 0xF0]).as_slice(), b"\0\xF0\xF0"); |
There was a problem hiding this comment.
It's probably a lot of work to support, but proptest would be a great way to fuzz the whole library.
There was a problem hiding this comment.
There are a bunch of fuzzing tests already, currently implemented manually with cargo fuzz. I’ll have a look at proptest and see if that makes it easier to be more complete.
|
I am now considering changing the whole thing more drastically. I want to get rid of the built-in support for But it would avoid the need for debug assertions. |
|
The change to |
This PR changes most of the decoding code to be mostly panic free.
Specifically, it removes the use of slice indexing as well as
unwrapandexpecton options and results.It does not do this for encoding – which requires some redesign in the traits – and for the string types – which will be done in separate PRs to make reviewing of this PR easier.
The PR introduces a breaking change in the
decode::Sourcetrait by changing how invalid indexes are treated. Thebytesmethod now returns an optionalBytes, returningNoneif the indexes are invalid. If theadvancemethod is called with an invalid length, the source is expected to go into some error state. Finally, the error type oftake_opt_u8was changed to allow an explicit error.This PR raises the minimum supported Rust version to 1.74.