Validate dimensionality #43
Conversation
- crash-1.nii generated through fuzz testing
- add error variant InconsistentHeader - add public methods to NiftiHeader `dim` and `dimensionality` - raise an error if dimensionality is inconsistent
nilgoyette
left a comment
nilgoyette
left a comment
There was a problem hiding this comment.
A git grep "+ 1" tells me that we have this same pattern in impl NiftiVolume for InMemNiftiVolume, in dim and dimensionality. I understand that it didn't crash the application when you tested, but, I wonder, should the same pattern have the same error management?
src/header.rs
Outdated
| /// | ||
| /// `NiftiError::` if `dim[0]` does not represent a valid dimensionality. | ||
| pub fn dimensionality(&self) -> Result <usize> { | ||
| if self.dim[0] > 7 { |
There was a problem hiding this comment.
Is 0 a valid dimensionality?
There was a problem hiding this comment.
Great question, I don't know. Might be worth looking into this.
| //! | ||
| #![deny(missing_debug_implementations)] | ||
| #![warn(missing_docs, unused_extern_crates, trivial_casts, unused_results)] | ||
| #![recursion_limit = "128"] |
There was a problem hiding this comment.
Can you please tell me why we need a recursion limit? I don't remember any use of recursion in this lib.
There was a problem hiding this comment.
This is the compiler's recursive operation limit. Recursion is used internally by the quick_error! macro. Once a significant number of variants are declared, which is the case here, we need to push this value a bit higher.
|
Thanks for reviewing, @nilgoyette. I can double-check those patterns and replace occurrences where reasonable the next few days. But from what I noticed, the circumstances in the volume structs are different, since More comments inline. |
- replace InconsistentHeader with InconsistentDim in NiftiHeader - dim[0] == 0 is invalid - check all dimensions in NiftiHeader::dim() for no zeroes
|
LGTM. |
I found a problem while fuzzing the crate. If
dim[0]contains an inappropriate value, attempting to read a volume would panic. This PR makes some changes to accommodate the fix.dim[0]must not be higher than 7).dim[0]is incorrect.NiftiHeader, which validate the values in the process.