Error instead of corrupt large op counters ()

Since b78211ca6, OpIds have been silently truncated to 2**32. This
causes corruption in the case the op id overflows.

This change converts the silent error to a panic, and guards against the
panic on the codepath found by the fuzzer.
This commit is contained in:
Conrad Irwin 2023-03-07 09:49:04 -07:00 committed by GitHub
parent 2c1970f664
commit 7b747b8341
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 29 additions and 8 deletions

View file

@ -104,11 +104,11 @@ impl<'a> OpIdIter<'a> {
.transpose()
.map_err(|e| DecodeColumnError::decode_raw("counter", e))?;
match (actor, counter) {
(Some(Some(a)), Some(Some(c))) => match c.try_into() {
Ok(c) => Ok(Some(OpId::new(c, a as usize))),
(Some(Some(a)), Some(Some(c))) => match u32::try_from(c) {
Ok(c) => Ok(Some(OpId::new(c as u64, a as usize))),
Err(_) => Err(DecodeColumnError::invalid_value(
"counter",
"negative value encountered",
"negative or large value encountered",
)),
},
(Some(None), _) => Err(DecodeColumnError::unexpected_null("actor")),

View file

@ -139,7 +139,7 @@ pub(crate) fn option_splice_scenario<
}
pub(crate) fn opid() -> impl Strategy<Value = OpId> + Clone {
(0..(i64::MAX as usize), 0..(i64::MAX as u64)).prop_map(|(actor, ctr)| OpId::new(ctr, actor))
(0..(u32::MAX as usize), 0..(u32::MAX as u64)).prop_map(|(actor, ctr)| OpId::new(ctr, actor))
}
pub(crate) fn elemid() -> impl Strategy<Value = ElemId> + Clone {

View file

@ -177,6 +177,9 @@ impl<'a> Change<'a, Unverified> {
for op in self.iter_ops() {
f(op?);
}
if u32::try_from(u64::from(self.start_op)).is_err() {
return Err(ReadChangeOpError::CounterTooLarge);
}
Ok(Change {
bytes: self.bytes,
header: self.header,

View file

@ -283,6 +283,8 @@ pub enum ReadChangeOpError {
DecodeError(#[from] DecodeColumnError),
#[error(transparent)]
InvalidOpType(#[from] InvalidOpType),
#[error("counter too large")]
CounterTooLarge,
}
#[derive(Clone)]

View file

@ -439,17 +439,17 @@ pub(crate) struct OpId(u32, u32);
impl OpId {
pub(crate) fn new(counter: u64, actor: usize) -> Self {
Self(counter as u32, actor as u32)
Self(counter.try_into().unwrap(), actor.try_into().unwrap())
}
#[inline]
pub(crate) fn counter(&self) -> u64 {
self.0 as u64
self.0.into()
}
#[inline]
pub(crate) fn actor(&self) -> usize {
self.1 as usize
self.1.try_into().unwrap()
}
#[inline]

View file

@ -129,7 +129,7 @@ mod tests {
fn gen_opid(actors: Vec<ActorId>) -> impl Strategy<Value = OpId> {
(0..actors.len()).prop_flat_map(|actor_idx| {
(Just(actor_idx), 0..u64::MAX)
(Just(actor_idx), 0..(u32::MAX as u64))
.prop_map(|(actor_idx, counter)| OpId::new(counter, actor_idx))
})
}

Binary file not shown.

Binary file not shown.

View file

@ -1445,6 +1445,22 @@ fn negative_64() {
assert!(doc.transact(|d| { d.put(ROOT, "a", -64_i64) }).is_ok())
}
#[test]
fn obj_id_64bits() {
// this change has an opId of 2**42, which when cast to a 32-bit int gives 0.
// The file should either fail to load (a limit of ~4 billion ops per doc seems reasonable), or be handled correctly.
if let Ok(doc) = Automerge::load(&fixture("64bit_obj_id_change.automerge")) {
let map_id = doc.get(ROOT, "a").unwrap().unwrap().1;
assert!(map_id != ROOT)
}
// this fixture is the same as the above, but as a document chunk.
if let Ok(doc) = Automerge::load(&fixture("64bit_obj_id_doc.automerge")) {
let map_id = doc.get(ROOT, "a").unwrap().unwrap().1;
assert!(map_id != ROOT)
}
}
#[test]
fn bad_change_on_optree_node_boundary() {
let mut doc = Automerge::new();