-
Notifications
You must be signed in to change notification settings - Fork 980
fix: bound MerkleProof path length on deserialize #3902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -595,6 +595,10 @@ impl<'a> Reader for StreamingReader<'a> { | |
|
|
||
| /// Read a fixed number of bytes. | ||
| fn read_fixed_bytes(&mut self, len: usize) -> Result<Vec<u8>, Error> { | ||
| // Match BinReader / BufReader: refuse huge single reads that would OOM or panic. | ||
| if len > 100_000 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now the third copy of the 100k reader limit. Could we use one shared constant and add a direct StreamingReader boundary test here? |
||
| return Err(Error::TooLargeReadErr); | ||
| } | ||
| let mut buf = vec![0u8; len]; | ||
| self.stream.read_exact(&mut buf)?; | ||
| self.total_bytes_read += len as u64; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,19 +14,54 @@ | |
|
|
||
| mod common; | ||
|
|
||
| use self::core::core::merkle_proof::MerkleProof; | ||
| use self::core::core::merkle_proof::{MerkleProof, MAX_MERKLE_PROOF_PATH}; | ||
| use self::core::core::pmmr::{ReadablePMMR, VecBackend, PMMR}; | ||
| use self::core::ser::{self, PMMRIndexHashable}; | ||
| use self::core::ser::{self, DeserializationMode, PMMRIndexHashable, ProtocolVersion}; | ||
| use crate::common::TestElem; | ||
| use grin_core as core; | ||
|
|
||
| fn push_u64(buf: &mut Vec<u8>, v: u64) { | ||
| buf.extend_from_slice(&v.to_be_bytes()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn empty_merkle_proof() { | ||
| let proof = MerkleProof::empty(); | ||
| assert_eq!(proof.path, vec![]); | ||
| assert_eq!(proof.mmr_size, 0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn merkle_proof_read_rejects_huge_path_len() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we also cover the accepted side of the boundary: 64 hashes accepted and 65 rejected? The current tests only exercise rejection. |
||
| // mmr_size=0, path_len just over the allowed maximum → TooLargeReadErr | ||
| // before any allocation of path entries. | ||
| let mut bytes = vec![]; | ||
| push_u64(&mut bytes, 0); | ||
| push_u64(&mut bytes, MAX_MERKLE_PROOF_PATH + 1); | ||
|
|
||
| let res: Result<MerkleProof, _> = ser::deserialize( | ||
| &mut &bytes[..], | ||
| ProtocolVersion(1), | ||
| DeserializationMode::default(), | ||
| ); | ||
| assert_eq!(res.err(), Some(ser::Error::TooLargeReadErr)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn merkle_proof_read_rejects_capacity_overflow_path_len() { | ||
| // Historical bug: path_len near u64::MAX caused Vec::with_capacity to panic. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we trim these tests a little? The names already describe the cases, and the historical context is covered by the PR description. A compact bounds test covering 64, 65, and u64::MAX would avoid the duplicated setup. |
||
| let mut bytes = vec![]; | ||
| push_u64(&mut bytes, 1); | ||
| push_u64(&mut bytes, u64::MAX); | ||
|
|
||
| let res: Result<MerkleProof, _> = ser::deserialize( | ||
| &mut &bytes[..], | ||
| ProtocolVersion(1), | ||
| DeserializationMode::default(), | ||
| ); | ||
| assert_eq!(res.err(), Some(ser::Error::TooLargeReadErr)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn merkle_proof_ser_deser() { | ||
| let mut ba = VecBackend::new(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With path_len capped at 64, the full allocation is at most 2 KiB. Could we allocate path_len directly and drop the second limit and the min import?