Fix Fuzzing crash by using Result() instead of assert!

[baylesj]

This patch fixes a downstream fuzzing crash in Chrome, caused by asserting on input data.

Wherever possible, returning an Error Result is greatly preferred, since crashing on user data may cause unexpected application crashes or potentially other issues.

See related Chromium issue: https://g-issues.chromium.org/issues/433557303

[baylesj]

I think this fix is also part of a larger scoped discussion around how we want to handle issues with input data. In Chromium, we want the decoder to error whenever there is an issue with user data, instead of crash, which has a negative impact on the user. In this library, it would be ideal if, generally speaking, issues with user data ended up in an error Result, and asserts were limited to developer/logic errors.

[baylesj]

@pdeljanov mind taking a look?

[pdeljanov]

Nice find, let's just adjust the error string!

[baylesj]

Applied feedback. Thank you for the review!

[pdeljanov]

I think this fix is also part of a larger scoped discussion around how we want to handle issues with input data. In Chromium, we want the decoder to error whenever there is an issue with user data, instead of crash, which has a negative impact on the user. In this library, it would be ideal if, generally speaking, issues with user data ended up in an error Result, and asserts were limited to developer/logic errors.

I agree.

We should always return a decode error. Unwrap should only be used after we pre-validate that it, in-fact, can be unwrapped. In the newer areas of the codebase unwraps usually have a "Safety:" comment/justification associated with them to help auditing, but that's a more recent addition.