diff --git a/.changeset/text-import-out-of-bounds.md b/.changeset/text-import-out-of-bounds.md new file mode 100644 index 000000000..94baa530c --- /dev/null +++ b/.changeset/text-import-out-of-bounds.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +Reject imported Text ops whose positions lie past the end of the text. An insert, delete or mark past the end used to panic inside the document (a `RuntimeError: unreachable` trap in WASM, leaving the doc unusable). `import`, `importBatch` and `importJsonUpdates` now throw a decode error and leave the document unchanged and usable, like out-of-bounds List and MovableList ops (#1160). diff --git a/context/movable-list-op-validation.md b/context/movable-list-op-validation.md index 575dd298e..9e5c454f5 100644 --- a/context/movable-list-op-validation.md +++ b/context/movable-list-op-validation.md @@ -1,6 +1,6 @@ # Movable List `Move`/`Set` Validation on Import -Verified against code 2026-09-28 (merged with main `6e294c87`). +Verified against code 2026-09-28 (merged with main `6e294c87`). Text bounds and the rollback-scope list verified 2026-09-30. Imported movable-list `Move { from, to, elem_id }` and `Set { elem_id, value }` ops come from other peers, so they are external input. Several shapes of them used @@ -75,12 +75,21 @@ Tests: - It checks the composed delta, so an out-of-bounds move that a later op in the same import cancels (e.g. `move to: 9` followed by `delete pos: 9`) is accepted. Every path gives the same result for it. + - Text has the same check: `RichtextState::validate_diff` bounds-checks the text + delta in entity-index space (style anchors count). An insert, delete or mark + past the end of the text used to panic in `insert_elem_at_entity_index` with + the locks held (loro-dev/loro#1160). ### Why every such import gets a rollback scope `ImportChangesPreflight` (`OpLog::preflight_import_changes`) and `PendingChanges::has_state_apply_rollback_ops` set `needs_state_apply_rollback` -for List, MovableList and Tree ops. +for List, MovableList, Text and Tree ops (`oplog::state_apply_can_reject`). An +import without that scope panics if `validate_diff` rejects its diff, so a +container type whose `validate_diff` can fail must be listed there. +- The scope costs a few small clones and journal entries per import: about + +0.3 µs per single-op Text import (loro-dev/loro#1160 measured 20k imports at + ~44 → ~50 ms). Large imports are not measurably slower. - The preflight inspects the ops of **every** new change, including ones whose deps are not in the DAG yet. Those deps may be earlier changes of the same import, which then unlock them during the import. diff --git a/crates/loro-internal/src/oplog.rs b/crates/loro-internal/src/oplog.rs index dacb7f605..d07beaa21 100644 --- a/crates/loro-internal/src/oplog.rs +++ b/crates/loro-internal/src/oplog.rs @@ -146,6 +146,19 @@ pub(crate) struct ReplayBase { pub concurrent_containers: Option>, } +/// Whether an imported op on a container of type `ty` can be rejected by +/// `ContainerState::validate_diff` (e.g. a sequence position past the end), so +/// the import needs a rollback scope to undo the op log on that error. +pub(crate) fn state_apply_can_reject(ty: ContainerType) -> bool { + matches!( + ty, + ContainerType::List + | ContainerType::MovableList + | ContainerType::Text + | ContainerType::Tree + ) +} + impl OpLog { #[inline] pub(crate) fn new(visible_op_count: Arc) -> Self { @@ -365,12 +378,11 @@ impl OpLog { // Inspect the ops even when the deps are not in the DAG yet: they may be // earlier changes of this same import, which then unlock this one. - if change.ops.iter().any(|op| { - matches!( - op.container.get_type(), - ContainerType::List | ContainerType::MovableList | ContainerType::Tree - ) - }) { + if change + .ops + .iter() + .any(|op| state_apply_can_reject(op.container.get_type())) + { ans.needs_state_apply_rollback = true; } @@ -385,7 +397,7 @@ impl OpLog { // Any newly applied change can unlock pending changes whose ops are not // visible in `changes`, so include pending in the rollback decision. - // Keep this narrow: text/map-only pending changes cannot return a + // Keep this narrow: map-only pending changes cannot return a // state-apply error, and forcing rollback there adds lock traffic to // small sync/import workloads. // diff --git a/crates/loro-internal/src/oplog/pending_changes.rs b/crates/loro-internal/src/oplog/pending_changes.rs index 35361ffab..78e65da0b 100644 --- a/crates/loro-internal/src/oplog/pending_changes.rs +++ b/crates/loro-internal/src/oplog/pending_changes.rs @@ -8,9 +8,7 @@ use crate::{ version::{ImVersionVector, VersionRange}, OpLog, VersionVector, }; -use loro_common::{ - ContainerType, Counter, CounterSpan, HasCounterSpan, HasIdSpan, IdSpan, PeerID, ID, -}; +use loro_common::{Counter, CounterSpan, HasCounterSpan, HasIdSpan, IdSpan, PeerID, ID}; use rustc_hash::FxHashMap; #[derive(Debug, Clone)] @@ -43,12 +41,10 @@ impl PendingChanges { self.changes.values().any(|tree| { tree.values().any(|changes| { changes.iter().any(|change| { - change.ops.iter().any(|op| { - matches!( - op.container.get_type(), - ContainerType::List | ContainerType::MovableList | ContainerType::Tree - ) - }) + change + .ops + .iter() + .any(|op| super::state_apply_can_reject(op.container.get_type())) }) }) }) diff --git a/crates/loro-internal/src/state/AGENTS.md b/crates/loro-internal/src/state/AGENTS.md index 9373e691e..d6c42ebbb 100644 --- a/crates/loro-internal/src/state/AGENTS.md +++ b/crates/loro-internal/src/state/AGENTS.md @@ -22,7 +22,7 @@ before changing mergeable child behavior. [../../../../context/shallow-snapshot-style-redaction.md](../../../../context/shallow-snapshot-style-redaction.md) before changing the richtext snapshot codec or that pass. `movable_list_state.rs` import validation and the Checkout-mode fallback for - moves of deleted elements: + moves of deleted elements, plus the List/Text `validate_diff` bounds checks: [../../../../context/movable-list-op-validation.md](../../../../context/movable-list-op-validation.md). - `mergeable.rs`: logical child edge resolution for mergeable containers. - `dead_containers_cache.rs`: dead/alive tracking. Map/list deletions are diff --git a/crates/loro-internal/src/state/richtext_state.rs b/crates/loro-internal/src/state/richtext_state.rs index d95415c20..e753cffc1 100644 --- a/crates/loro-internal/src/state/richtext_state.rs +++ b/crates/loro-internal/src/state/richtext_state.rs @@ -572,6 +572,46 @@ impl ContainerState for RichtextState { Diff::Text(ans) } + fn validate_diff(&self, diff: &InternalDiff) -> LoroResult<()> { + let InternalDiff::RichtextRaw(delta) = diff else { + unreachable!() + }; + + // Text items are addressed by entity index (style anchors included). + let mut cursor = 0usize; + let mut projected = match &self.state { + LazyLoad::Src(s) => s.entity_index, + LazyLoad::Dst(s) => s.len_entity(), + }; + for span in delta.iter() { + match span { + loro_delta::DeltaItem::Retain { len, .. } => { + cursor += len; + if cursor > projected { + return Err(LoroError::DecodeError( + format!( + "text diff retains {cursor} items but state only has {projected}" + ) + .into_boxed_str(), + )); + } + } + loro_delta::DeltaItem::Replace { value, delete, .. } => { + if cursor + delete > projected { + return Err(LoroError::DecodeError( + format!("text diff deletes {delete} at {cursor} but state only has {projected}") + .into_boxed_str(), + )); + } + projected = projected - delete + value.rle_len(); + cursor += value.rle_len(); + } + } + } + + Ok(()) + } + fn apply_diff(&mut self, diff: InternalDiff, _ctx: DiffApplyContext) -> LoroResult<()> { self.update_version(); let InternalDiff::RichtextRaw(richtext) = diff else { diff --git a/crates/loro-wasm/tests/text_invalid_ops.test.ts b/crates/loro-wasm/tests/text_invalid_ops.test.ts new file mode 100644 index 000000000..8d5e82cda --- /dev/null +++ b/crates/loro-wasm/tests/text_invalid_ops.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, it } from "vitest"; +import { LoroDoc } from "../bundler/index"; + +// A Text op past the end of the text used to panic inside the doc locks, which +// traps the WASM instance ("RuntimeError: unreachable") (loro-dev/loro#1160). It +// must throw a readable error and leave the doc unchanged and usable. + +/** Peer 1 writes "ab" (counters/lamports 0..=1). */ +function base(): LoroDoc { + const doc = new LoroDoc(); + doc.setPeerId(1); + doc.getText("t").insert(0, "ab"); + doc.commit(); + return doc; +} + +function forged(ops: unknown[]): string { + return JSON.stringify({ + schema_version: 1, + start_version: {}, + peers: ["1"], + changes: [ + { + id: "2@0", + timestamp: 0, + deps: ["1@0"], + lamport: 2, + msg: null, + ops: ops.map((content, i) => ({ + container: "cid:root-t:Text", + content, + counter: 2 + i, + })), + }, + ], + }); +} + +function expectRejected(doc: LoroDoc, json: string) { + const value = doc.toJSON(); + const delta = doc.getText("t").toDelta(); + const vv = doc.oplogVersion().toJSON(); + let error: unknown; + try { + doc.importJsonUpdates(json); + } catch (e) { + error = e; + } + expect(error).toBeDefined(); + expect(String(error)).toMatch(/Decode error/); + expect(String(error)).not.toMatch(/unreachable/); + expect(doc.toJSON()).toEqual(value); + expect(doc.getText("t").toDelta()).toEqual(delta); + expect(doc.oplogVersion().toJSON()).toEqual(vv); + + // Still usable. + const text = doc.getText("t"); + text.insert(text.length, "!"); + doc.commit(); + const remote = new LoroDoc(); + remote.import(doc.export({ mode: "snapshot" })); + remote.getText("t").insert(0, "r"); + remote.commit(); + doc.import(remote.export({ mode: "update", from: doc.oplogVersion() })); + expect(doc.toJSON()).toEqual(remote.toJSON()); +} + +describe("out-of-bounds text ops", () => { + it("rejects an insert past the end of the text", () => { + expectRejected(base(), forged([{ type: "insert", pos: 3, text: "x" }])); + }); + + it("rejects deletes and marks past the end of the text", () => { + expectRejected( + base(), + forged([{ type: "delete", pos: 1, len: 2, start_id: "1@0" }]), + ); + expectRejected( + base(), + forged([ + { + type: "mark", + start: 1, + end: 9, + style_key: "bold", + style_value: true, + info: 0, + }, + { type: "mark_end" }, + ]), + ); + }); + + it("still imports an insert at the end of the text", () => { + const doc = base(); + doc.importJsonUpdates(forged([{ type: "insert", pos: 2, text: "x" }])); + expect(doc.getText("t").toString()).toBe("abx"); + }); +}); diff --git a/crates/loro/tests/text_invalid_ops.rs b/crates/loro/tests/text_invalid_ops.rs new file mode 100644 index 000000000..6ef74f0be --- /dev/null +++ b/crates/loro/tests/text_invalid_ops.rs @@ -0,0 +1,281 @@ +//! Forged Text ops whose positions lie past the end of the text used to panic +//! inside the doc locks (loro-dev/loro#1160). They are now rejected with `Err` +//! and leave the document untouched and usable, like out-of-bounds List and +//! MovableList ops. + +use loro::{ExportMode, LoroDoc, LoroValue, VersionVector}; +use serde_json::{json, Value}; + +/// Peer 1 writes `"ab"` (counters/lamports 0..=1). +fn base() -> LoroDoc { + let doc = LoroDoc::new(); + doc.set_peer_id(1).unwrap(); + doc.get_text("t").insert(0, "ab").unwrap(); + doc.commit(); + doc +} + +/// `base()` plus a concurrent edit by peer 2, so the forged op is not a plain +/// fast-forward of the state. +fn base_with_concurrent_edit() -> LoroDoc { + let doc = LoroDoc::new(); + doc.set_peer_id(2).unwrap(); + doc.get_text("t").insert(0, "xy").unwrap(); + doc.commit(); + doc.import(&base().export(ExportMode::all_updates()).unwrap()) + .unwrap(); + doc +} + +/// A single change by peer 1, causally after `base()`, holding `ops`. +fn forged_ops(ops: &[Value]) -> String { + let mut counter = 2; + let ops: Vec = ops + .iter() + .map(|content| { + let op = + json!({"container": "cid:root-t:Text", "content": content, "counter": counter}); + counter += match content["type"].as_str().unwrap() { + "insert" => content["text"].as_str().unwrap().chars().count() as i64, + "delete" => content["len"].as_i64().unwrap().abs(), + _ => 1, + }; + op + }) + .collect(); + json!({ + "schema_version": 1, + "start_version": {}, + "peers": ["1"], + "changes": [{ + "id": "2@0", + "timestamp": 0, + "deps": ["1@0"], + "lamport": 2, + "msg": null, + "ops": ops + }] + }) + .to_string() +} + +fn forged(op: Value) -> String { + forged_ops(&[op]) +} + +fn mark(start: u32, end: u32) -> Vec { + vec![ + json!({"type": "mark", "start": start, "end": end, "style_key": "bold", "style_value": true, "info": 0}), + json!({"type": "mark_end"}), + ] +} + +struct Snapshot { + value: LoroValue, + richtext: LoroValue, + vv: VersionVector, +} + +impl Snapshot { + fn of(doc: &LoroDoc) -> Self { + Self { + value: doc.get_deep_value(), + richtext: doc.get_text("t").get_richtext_value(), + vv: doc.oplog_vv(), + } + } + + fn assert_unchanged(&self, doc: &LoroDoc) { + assert_eq!(doc.get_deep_value(), self.value); + assert_eq!(doc.get_text("t").get_richtext_value(), self.richtext); + assert_eq!(doc.oplog_vv(), self.vv); + } +} + +/// The doc still accepts local edits and valid remote updates after a rejected import. +fn assert_usable(doc: &LoroDoc) { + let text = doc.get_text("t"); + let len = text.len_unicode(); + text.insert(len, "!").unwrap(); + text.mark(0..1, "italic", true).unwrap(); + text.delete(0, 1).unwrap(); + doc.commit(); + assert_eq!(text.len_unicode(), len); + + let remote = LoroDoc::new(); + remote + .import(&doc.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + remote.get_text("t").insert(0, "r").unwrap(); + remote.commit(); + doc.import(&remote.export(ExportMode::updates(&doc.oplog_vv())).unwrap()) + .unwrap(); + assert_eq!(doc.get_deep_value(), remote.get_deep_value()); +} + +fn assert_rejected(doc: &LoroDoc, json: String) { + let before = Snapshot::of(doc); + let err = doc.import_json_updates(json).unwrap_err(); + assert!( + matches!(err, loro::LoroError::DecodeError(_)), + "unexpected error: {err:?}" + ); + before.assert_unchanged(doc); + assert_usable(doc); +} + +/// Detached imports only touch the oplog, which lets us export a forged op as binary. +fn forged_binary(json: String) -> Vec { + let carrier = base(); + carrier.detach(); + carrier.import_json_updates(json).unwrap(); + carrier + .export(ExportMode::updates(&base().oplog_vv())) + .unwrap() +} + +fn out_of_bounds_ops() -> Vec> { + vec![ + // loro-dev/loro#1160 + vec![json!({"type": "insert", "pos": 3, "text": "x"})], + vec![json!({"type": "insert", "pos": 100, "text": "x"})], + vec![json!({"type": "delete", "pos": 1, "len": 2, "start_id": "1@0"})], + vec![json!({"type": "delete", "pos": 2, "len": 1, "start_id": "1@0"})], + // A reversed delete removes `pos + len + 1..=pos`. + vec![json!({"type": "delete", "pos": 2, "len": -2, "start_id": "1@0"})], + vec![json!({"type": "delete", "pos": 5, "len": -2, "start_id": "1@0"})], + mark(0, 3), + mark(3, 4), + mark(1, 9), + // Out of bounds only after an earlier op of the same change. + vec![ + json!({"type": "delete", "pos": 0, "len": 2, "start_id": "0@0"}), + json!({"type": "insert", "pos": 1, "text": "x"}), + ], + ] +} + +#[test] +fn text_insert_past_end_is_rejected() { + // The exact repro from loro-dev/loro#1160. + assert_rejected( + &base(), + forged(json!({"type": "insert", "pos": 3, "text": "x"})), + ); +} + +#[test] +fn out_of_bounds_text_ops_are_rejected() { + for ops in out_of_bounds_ops() { + assert_rejected(&base(), forged_ops(&ops)); + assert_rejected(&base_with_concurrent_edit(), forged_ops(&ops)); + } +} + +#[test] +fn out_of_bounds_text_ops_are_rejected_in_binary_imports() { + for ops in out_of_bounds_ops() { + let update = forged_binary(forged_ops(&ops)); + + let doc = base(); + let before = Snapshot::of(&doc); + doc.import(&update).unwrap_err(); + before.assert_unchanged(&doc); + assert_usable(&doc); + + let doc = base(); + let before = Snapshot::of(&doc); + doc.import_batch(&[update.clone()]).unwrap_err(); + assert!(!doc.is_detached()); + before.assert_unchanged(&doc); + assert_usable(&doc); + + let doc = base_with_concurrent_edit(); + let before = Snapshot::of(&doc); + doc.import(&update).unwrap_err(); + before.assert_unchanged(&doc); + assert_usable(&doc); + } +} + +#[test] +fn in_bounds_text_ops_still_import() { + let cases = [ + ( + vec![json!({"type": "insert", "pos": 2, "text": "x"})], + "abx", + ), + ( + vec![json!({"type": "delete", "pos": 0, "len": 2, "start_id": "0@0"})], + "", + ), + ( + vec![json!({"type": "delete", "pos": 1, "len": -2, "start_id": "1@0"})], + "", + ), + (mark(0, 2), "ab"), + (mark(1, 2), "ab"), + ]; + for (ops, expected) in cases { + let doc = base(); + doc.import_json_updates(forged_ops(&ops)).unwrap(); + assert_eq!(doc.get_text("t").to_string(), expected); + assert_usable(&doc); + + let doc = base_with_concurrent_edit(); + doc.import_json_updates(forged_ops(&ops)).unwrap(); + assert_usable(&doc); + } +} + +#[test] +fn out_of_bounds_text_op_after_map_change_in_same_import_is_rejected() { + // The forged change depends on a map-only change of the same import, so its + // deps are not in the DAG when the import starts. + let json = json!({ + "schema_version": 1, "start_version": {}, "peers": ["1"], + "changes": [ + {"id": "2@0", "timestamp": 0, "deps": ["1@0"], "lamport": 2, "msg": null, + "ops": [{"container": "cid:root-meta:Map", + "content": {"type": "insert", "key": "k", "value": 1}, "counter": 2}]}, + {"id": "3@0", "timestamp": 0, "deps": ["2@0"], "lamport": 3, "msg": null, + "ops": [{"container": "cid:root-t:Text", + "content": {"type": "insert", "pos": 3, "text": "x"}, "counter": 3}]} + ] + }) + .to_string(); + assert_rejected(&base(), json); +} + +#[test] +fn map_only_import_that_unlocks_parked_out_of_bounds_text_op_is_rejected() { + let change = |id: &str, dep: &str, lamport: u32, op: Value| { + json!({ + "schema_version": 1, "start_version": {}, "peers": ["1"], + "changes": [{"id": id, "timestamp": 0, "deps": [dep], "lamport": lamport, + "msg": null, "ops": [op]}] + }) + .to_string() + }; + let map_change = change( + "2@0", + "1@0", + 2, + json!({"container": "cid:root-meta:Map", + "content": {"type": "insert", "key": "k", "value": 1}, "counter": 2}), + ); + let forged_text = change( + "3@0", + "2@0", + 3, + json!({"container": "cid:root-t:Text", + "content": {"type": "insert", "pos": 3, "text": "x"}, "counter": 3}), + ); + + let doc = base(); + // Its dep is missing, so the forged change is parked, not validated yet. + let status = doc.import_json_updates(forged_text).unwrap(); + assert!(status.pending.is_some()); + // The map-only import unlocks it, so it must still get a rollback scope. + assert_rejected(&doc, map_change); +}