Storage panic from oss-fuzz#385
Conversation
kaczmarczyck
force-pushed
the
storage-fuzz-test
branch
from
51ab3c4 to
e4152af
Compare
|
|
||
| impl UpgradeStorage for SyscallUpgradeStorage { | ||
| fn read_partition(&self, offset: usize, length: usize) -> StorageResult<&[u8]> { | ||
| if length == 0 { |
There was a problem hiding this comment.
Shouldn't we check that the offset is valid (essentially offset <= partition.len())? And then wouldn't the rest of the code just work by itself?
There was a problem hiding this comment.
I was wondering if I should ignore empty slices (my current choice) or treat them as being present at offset. The rest of the code just works after that initial check in both cases, right?
To be honest, both choices should do the job. I don't see why a client would read or write empty slices. The fail outside of range could be a hacky partition check, but that's harder than just asking directly.
Any advantages of one over the other?
There was a problem hiding this comment.
The rest of the code just works after that initial check in both cases, right?
I don't know. In the current case it's not executed so it doesn't matter. But if it works it makes it possible to just check the offset and still run the code.
I don't see why a client would read or write empty slices.
Then why not return an error instead of an empty slice?
The fail outside of range could be a hacky partition check, but that's harder than just asking directly.
I don't understand what you want to say.
Any advantages of one over the other?
From the 3 options:
- Early exit with error
- Early exit with empty slice (no validity check)
- Check offset validity
I think "early exit with empty slice" should not be an option because it can hide bad behaviors (invalid offset returning correct result). Between the 2 other options it's a matter of whether it's valid to read an empty slice or not.
There was a problem hiding this comment.
Okay, then I'll just disallow empty slices in general, for read and write.
| } | ||
|
|
||
| fn write_partition(&mut self, offset: usize, data: &[u8]) -> StorageResult<()> { | ||
| if data.is_empty() { |
There was a problem hiding this comment.
Changed to error.
3 participants
oss-fuzz found a panic (bug report is private until fixed).
Fixed and added tests for this issue, on more coverage.