Safe access to buffer via .get(index)?
#138
Conversation
consts/src/lib.rs
Outdated
|
|
||
| fn get(self, index: usize) -> Result<u8, AccessError> { | ||
| if index >= self.len { | ||
| return Err(AccessError::OutOfBounds); |
There was a problem hiding this comment.
Normally I prefer to halt and early-return upon errors, but I think in this case the function is small enough to use an else. Updated.
| impl From<AccessError> for EDHOCError { | ||
| fn from(error: AccessError) -> Self { | ||
| match error { | ||
| AccessError::OutOfBounds => EDHOCError::ParsingError, |
There was a problem hiding this comment.
So we are basically mapping all OutOfBounds errors to EDHOCError:ParsingError. This may result in ParsingError for non-parsing, i.e. encoding routines when accidentally trying to access the buffer content, right?
In the case of parsing routines when unsanitized input is received, this will properly return ParsingError.
There was a problem hiding this comment.
Yes, will add a TODO to think of a way to handle it in a more general case. For now it is fine since encoding routines are expected to always work (they return the value itself, and not Ok/Err).
There was a problem hiding this comment.
But note that you then won't be able to use the ? syntax and propagate the error.
geonnave
force-pushed
the
safe-buffer-access-via-get
branch
from
3aeb5c4 to
4016e8e
Compare
| @@ -172,6 +187,8 @@ impl Default for EdhocMessageBuffer { | |||
|
|
|||
| pub trait MessageBufferTrait { | |||
| fn new() -> Self; | |||
| fn get(self, index: usize) -> Result<u8, AccessError>; | |||
There was a problem hiding this comment.
The common pattern in the standard library is that fallible access just returns an Option (eg. the slice get function). Why deviate here?
There was a problem hiding this comment.
By returning AccessError, I can automatically cast it to EDHOCError and then use the ? operator in a clean way in any parse_* function.
On the other hand, I now realize that all parse_* functions only ever return 1 type of error (ParsingError). If I make them return Option then the standard way could work.
There was a problem hiding this comment.
That is indeed an inconvenience. The means of making ? just turn into a Result<..., MyError> are not stable yet (if they even work that way). If you still want to take up the suggestion, there is a pattern that allows using foo.get(10).or_parse_error()? in the parsing code -- although it's really just a very small enhancement over simply writing foo.get(10).ok_or(Error::ParseError)? every time.
There was a problem hiding this comment.
I am working on an alternative parsing mechanism, inspired by minicbor, which should solve this in the parsing functions.
| @@ -172,6 +187,8 @@ impl Default for EdhocMessageBuffer { | |||
|
|
|||
| pub trait MessageBufferTrait { | |||
| fn new() -> Self; | |||
| fn get(self, index: usize) -> Result<u8, AccessError>; | |||
| fn get_slice<'a>(&'a self, start: usize, len: usize) -> Result<&'a [u8], AccessError>; | |||
There was a problem hiding this comment.
The standard library uses a single get function that utilizes the SliceIndex trait. Could we do this here as well?
(Those two items combined might pave the way toward plugging in more widespread buffer implementations for production builds, and/or working with anything that is DerefMut<[u8]>).
@malishav as discussed, this is a proposal for safely accessing the received buffer. Added a partial implementation to enable a quick first-pass review.