diff --git a/grovedb-commitment-tree/src/client/sqlite_store/sql_helpers.rs b/grovedb-commitment-tree/src/client/sqlite_store/sql_helpers.rs index cb1a429a2..8f3782bee 100644 --- a/grovedb-commitment-tree/src/client/sqlite_store/sql_helpers.rs +++ b/grovedb-commitment-tree/src/client/sqlite_store/sql_helpers.rs @@ -18,10 +18,26 @@ use super::{ SqliteShardStoreError, SHARD_HEIGHT, }; +fn non_negative_i64_to_u64(value_name: &str, value: i64) -> Result { + u64::try_from(value).map_err(|_| { + SqliteShardStoreError::Serialization(format!( + "invalid negative {value_name} in sqlite row: {value}" + )) + }) +} + +fn u64_to_i64(value_name: &str, value: u64) -> Result { + i64::try_from(value).map_err(|_| { + SqliteShardStoreError::Serialization(format!( + "{value_name} value {value} exceeds sqlite i64 range" + )) + }) +} + pub(crate) fn create_tables(conn: &Connection) -> Result<(), SqliteShardStoreError> { conn.execute_batch( "CREATE TABLE IF NOT EXISTS commitment_tree_shards ( - shard_index INTEGER PRIMARY KEY, + shard_index INTEGER PRIMARY KEY CHECK (shard_index >= 0), shard_data BLOB NOT NULL ); CREATE TABLE IF NOT EXISTS commitment_tree_cap ( @@ -30,11 +46,11 @@ pub(crate) fn create_tables(conn: &Connection) -> Result<(), SqliteShardStoreErr ); CREATE TABLE IF NOT EXISTS commitment_tree_checkpoints ( checkpoint_id INTEGER PRIMARY KEY, - position INTEGER + position INTEGER CHECK (position IS NULL OR position >= 0) ); CREATE TABLE IF NOT EXISTS commitment_tree_checkpoint_marks_removed ( checkpoint_id INTEGER NOT NULL, - position INTEGER NOT NULL, + position INTEGER NOT NULL CHECK (position >= 0), PRIMARY KEY (checkpoint_id, position), FOREIGN KEY (checkpoint_id) REFERENCES commitment_tree_checkpoints(checkpoint_id) );", @@ -46,7 +62,7 @@ pub(crate) fn sql_get_shard( conn: &Connection, shard_root: Address, ) -> Result>, SqliteShardStoreError> { - let index = shard_root.index() as i64; + let index = u64_to_i64("shard_index", shard_root.index())?; let row: Option> = conn .query_row( "SELECT shard_data FROM commitment_tree_shards WHERE shard_index = ?1", @@ -85,7 +101,8 @@ pub(crate) fn sql_last_shard( match row { None => Ok(None), Some((index, data)) => { - let addr = Address::from_parts(Level::from(SHARD_HEIGHT), index as u64); + let index = non_negative_i64_to_u64("shard_index", index)?; + let addr = Address::from_parts(Level::from(SHARD_HEIGHT), index); let mut pos = 0; let tree = deserialize_tree(&data, &mut pos)?; let located = LocatedTree::from_parts(addr, tree).map_err(|addr| { @@ -102,7 +119,7 @@ pub(crate) fn sql_put_shard( conn: &Connection, subtree: &LocatedPrunableTree, ) -> Result<(), SqliteShardStoreError> { - let index = subtree.root_addr().index() as i64; + let index = u64_to_i64("shard_index", subtree.root_addr().index())?; let data = serialize_tree(subtree.root()); conn.execute( "INSERT OR REPLACE INTO commitment_tree_shards (shard_index, shard_data) VALUES (?1, ?2)", @@ -116,13 +133,14 @@ pub(crate) fn sql_get_shard_roots( ) -> Result, SqliteShardStoreError> { let mut stmt = conn.prepare("SELECT shard_index FROM commitment_tree_shards ORDER BY shard_index")?; - let rows = stmt.query_map([], |row| { - let index: i64 = row.get(0)?; - Ok(Address::from_parts(Level::from(SHARD_HEIGHT), index as u64)) - })?; + let rows = stmt.query_map([], |row| row.get::<_, i64>(0))?; let mut result = Vec::new(); - for addr in rows { - result.push(addr?); + for index in rows { + let index = index?; + result.push(Address::from_parts( + Level::from(SHARD_HEIGHT), + non_negative_i64_to_u64("shard_index", index)?, + )); } Ok(result) } @@ -131,9 +149,10 @@ pub(crate) fn sql_truncate_shards( conn: &Connection, shard_index: u64, ) -> Result<(), SqliteShardStoreError> { + let shard_index = u64_to_i64("shard_index", shard_index)?; conn.execute( "DELETE FROM commitment_tree_shards WHERE shard_index >= ?1", - params![shard_index as i64], + params![shard_index], )?; Ok(()) } @@ -197,21 +216,27 @@ pub(crate) fn sql_add_checkpoint( checkpoint_id: u32, checkpoint: &Checkpoint, ) -> Result<(), SqliteShardStoreError> { - let tx = conn.unchecked_transaction()?; let position: Option = match checkpoint.tree_state() { TreeState::Empty => None, - TreeState::AtPosition(pos) => Some(u64::from(pos) as i64), + TreeState::AtPosition(pos) => Some(u64_to_i64("position", u64::from(pos))?), }; + let mark_positions: Vec = checkpoint + .marks_removed() + .iter() + .map(|mark_pos| u64_to_i64("mark position", u64::from(*mark_pos))) + .collect::>()?; + + let tx = conn.unchecked_transaction()?; tx.execute( "INSERT INTO commitment_tree_checkpoints (checkpoint_id, position) VALUES (?1, ?2)", params![checkpoint_id, position], )?; - for mark_pos in checkpoint.marks_removed() { + for mark_pos in mark_positions { tx.execute( "INSERT INTO commitment_tree_checkpoint_marks_removed (checkpoint_id, position) \ VALUES (?1, ?2)", - params![checkpoint_id, u64::from(*mark_pos) as i64], + params![checkpoint_id, mark_pos], )?; } tx.commit()?; @@ -303,24 +328,30 @@ where None => Ok(false), Some(mut cp) => { update(&mut cp)?; + let position: Option = match cp.tree_state() { + TreeState::Empty => None, + TreeState::AtPosition(pos) => Some(u64_to_i64("position", u64::from(pos))?), + }; + let mark_positions: Vec = cp + .marks_removed() + .iter() + .map(|mark_pos| u64_to_i64("mark position", u64::from(*mark_pos))) + .collect::>()?; + let tx = conn.unchecked_transaction()?; tx.execute( "DELETE FROM commitment_tree_checkpoint_marks_removed WHERE checkpoint_id = ?1", params![checkpoint_id], )?; - let position: Option = match cp.tree_state() { - TreeState::Empty => None, - TreeState::AtPosition(pos) => Some(u64::from(pos) as i64), - }; tx.execute( "UPDATE commitment_tree_checkpoints SET position = ?1 WHERE checkpoint_id = ?2", params![position, checkpoint_id], )?; - for mark_pos in cp.marks_removed() { + for mark_pos in mark_positions { tx.execute( "INSERT INTO commitment_tree_checkpoint_marks_removed (checkpoint_id, \ position) VALUES (?1, ?2)", - params![checkpoint_id, u64::from(*mark_pos) as i64], + params![checkpoint_id, mark_pos], )?; } tx.commit()?; @@ -375,18 +406,20 @@ fn sql_load_checkpoint( ) -> Result { let tree_state = match position { None => TreeState::Empty, - Some(p) => TreeState::AtPosition(Position::from(p as u64)), + Some(p) => TreeState::AtPosition(Position::from(non_negative_i64_to_u64("position", p)?)), }; let mut stmt = conn.prepare( "SELECT position FROM commitment_tree_checkpoint_marks_removed WHERE checkpoint_id = ?1", )?; - let marks: BTreeSet = stmt - .query_map(params![checkpoint_id], |row| { - let p: i64 = row.get(0)?; - Ok(Position::from(p as u64)) - })? - .collect::, _>>()?; + let rows = stmt.query_map(params![checkpoint_id], |row| row.get::<_, i64>(0))?; + let mut marks = BTreeSet::new(); + for position in rows { + let position = position?; + marks.insert(Position::from(non_negative_i64_to_u64( + "position", position, + )?)); + } Ok(Checkpoint::from_parts(tree_state, marks)) } diff --git a/grovedb-commitment-tree/src/client/sqlite_store_tests.rs b/grovedb-commitment-tree/src/client/sqlite_store_tests.rs index 864d006a5..a846595d3 100644 --- a/grovedb-commitment-tree/src/client/sqlite_store_tests.rs +++ b/grovedb-commitment-tree/src/client/sqlite_store_tests.rs @@ -15,7 +15,7 @@ mod tests { use crate::client::sqlite_store::{ tree_serialization::{deserialize_tree, serialize_tree}, - SqliteShardStore, SHARD_HEIGHT, + SqliteShardStore, SqliteShardStoreError, SHARD_HEIGHT, }; fn test_store() -> SqliteShardStore { @@ -23,11 +23,47 @@ mod tests { SqliteShardStore::new(conn).expect("create store") } + fn legacy_schema_conn() -> Connection { + let conn = Connection::open_in_memory().expect("open legacy in-memory sqlite"); + conn.execute_batch( + "CREATE TABLE commitment_tree_shards ( + shard_index INTEGER PRIMARY KEY, + shard_data BLOB NOT NULL + ); + CREATE TABLE commitment_tree_cap ( + id INTEGER PRIMARY KEY CHECK (id = 0), + cap_data BLOB NOT NULL + ); + CREATE TABLE commitment_tree_checkpoints ( + checkpoint_id INTEGER PRIMARY KEY, + position INTEGER + ); + CREATE TABLE commitment_tree_checkpoint_marks_removed ( + checkpoint_id INTEGER NOT NULL, + position INTEGER NOT NULL, + PRIMARY KEY (checkpoint_id, position), + FOREIGN KEY (checkpoint_id) REFERENCES commitment_tree_checkpoints(checkpoint_id) + );", + ) + .expect("create legacy schema"); + conn + } + fn test_hash(i: u8) -> MerkleHashOrchard { let empty = MerkleHashOrchard::empty_leaf(); MerkleHashOrchard::combine(Level::from(i % 31 + 1), &empty, &empty) } + /// Assert that an error from a checked u64 -> i64 conversion mentions the + /// given value name and the standard "exceeds sqlite i64 range" fragment. + fn assert_overflow_error(err: &SqliteShardStoreError, value_name: &str) { + let msg = err.to_string(); + assert!( + msg.contains(value_name) && msg.contains("exceeds sqlite i64 range"), + "expected overflow error mentioning {value_name:?}, got: {msg}" + ); + } + // -- Schema tests -- #[test] @@ -80,6 +116,68 @@ mod tests { assert_eq!(count, 1); } + #[test] + fn test_schema_rejects_negative_shard_index() { + let store = test_store(); + let data = serialize_tree(&Tree::leaf((test_hash(1), RetentionFlags::MARKED))); + + let err = store + .with_conn(|conn| { + conn.execute( + "INSERT INTO commitment_tree_shards (shard_index, shard_data) VALUES (?1, ?2)", + rusqlite::params![-1i64, data], + ) + }) + .expect_err("negative shard index should be rejected"); + + assert!( + err.to_string().contains("CHECK constraint failed"), + "unexpected sqlite error: {err}" + ); + } + + #[test] + fn test_schema_rejects_negative_checkpoint_positions() { + let store = test_store(); + + let err = store + .with_conn(|conn| { + conn.execute( + "INSERT INTO commitment_tree_checkpoints (checkpoint_id, position) VALUES (?1, ?2)", + rusqlite::params![1u32, -1i64], + ) + }) + .expect_err("negative checkpoint position should be rejected"); + + assert!( + err.to_string().contains("CHECK constraint failed"), + "unexpected sqlite error: {err}" + ); + + store + .with_conn(|conn| { + conn.execute( + "INSERT INTO commitment_tree_checkpoints (checkpoint_id, position) VALUES (?1, NULL)", + rusqlite::params![2u32], + ) + }) + .expect("insert empty checkpoint"); + + let err = store + .with_conn(|conn| { + conn.execute( + "INSERT INTO commitment_tree_checkpoint_marks_removed (checkpoint_id, position) VALUES (?1, ?2)", + rusqlite::params![2u32, -1i64], + ) + }) + .expect_err("negative mark position should be rejected"); + + assert!( + err.to_string().contains("CHECK constraint failed"), + "unexpected sqlite error: {err}" + ); + } + // -- Serialization round-trip tests -- #[test] @@ -264,6 +362,48 @@ mod tests { assert_eq!(roots.last().expect("last root").index(), 2); } + #[test] + fn test_last_shard_rejects_negative_shard_index_from_legacy_schema() { + let conn = legacy_schema_conn(); + let data = serialize_tree(&Tree::leaf((test_hash(7), RetentionFlags::MARKED))); + conn.execute( + "INSERT INTO commitment_tree_shards (shard_index, shard_data) VALUES (?1, ?2)", + rusqlite::params![-1i64, data], + ) + .expect("insert corrupt shard"); + + let store = SqliteShardStore::new(conn).expect("open legacy store"); + let err = store + .last_shard() + .expect_err("negative shard index should fail"); + assert!( + err.to_string() + .contains("invalid negative shard_index in sqlite row: -1"), + "unexpected store error: {err}" + ); + } + + #[test] + fn test_get_shard_roots_rejects_negative_shard_index_from_legacy_schema() { + let conn = legacy_schema_conn(); + let data = serialize_tree(&Tree::leaf((test_hash(8), RetentionFlags::MARKED))); + conn.execute( + "INSERT INTO commitment_tree_shards (shard_index, shard_data) VALUES (?1, ?2)", + rusqlite::params![-1i64, data], + ) + .expect("insert corrupt shard"); + + let store = SqliteShardStore::new(conn).expect("open legacy store"); + let err = store + .get_shard_roots() + .expect_err("negative shard index should fail"); + assert!( + err.to_string() + .contains("invalid negative shard_index in sqlite row: -1"), + "unexpected store error: {err}" + ); + } + // -- Cap tests -- #[test] @@ -370,6 +510,51 @@ mod tests { assert_eq!(retrieved.tree_state(), TreeState::Empty); } + #[test] + fn test_get_checkpoint_rejects_negative_position_from_legacy_schema() { + let conn = legacy_schema_conn(); + conn.execute( + "INSERT INTO commitment_tree_checkpoints (checkpoint_id, position) VALUES (?1, ?2)", + rusqlite::params![1u32, -1i64], + ) + .expect("insert corrupt checkpoint"); + + let store = SqliteShardStore::new(conn).expect("open legacy store"); + let err = store + .get_checkpoint(&1) + .expect_err("negative checkpoint position should fail"); + assert!( + err.to_string() + .contains("invalid negative position in sqlite row: -1"), + "unexpected store error: {err}" + ); + } + + #[test] + fn test_get_checkpoint_rejects_negative_mark_position_from_legacy_schema() { + let conn = legacy_schema_conn(); + conn.execute( + "INSERT INTO commitment_tree_checkpoints (checkpoint_id, position) VALUES (?1, ?2)", + rusqlite::params![1u32, 10i64], + ) + .expect("insert checkpoint"); + conn.execute( + "INSERT INTO commitment_tree_checkpoint_marks_removed (checkpoint_id, position) VALUES (?1, ?2)", + rusqlite::params![1u32, -1i64], + ) + .expect("insert corrupt mark"); + + let store = SqliteShardStore::new(conn).expect("open legacy store"); + let err = store + .get_checkpoint(&1) + .expect_err("negative mark position should fail"); + assert!( + err.to_string() + .contains("invalid negative position in sqlite row: -1"), + "unexpected store error: {err}" + ); + } + #[test] fn test_remove_checkpoint() { let mut store = test_store(); @@ -464,6 +649,123 @@ mod tests { assert_eq!(ids, vec![5, 4]); } + #[test] + fn test_truncate_shards_rejects_u64_overflow() { + let mut store = test_store(); + let err = store + .truncate_shards(u64::MAX) + .expect_err("u64::MAX shard index should overflow i64"); + assert_overflow_error(&err, "shard_index"); + } + + #[test] + fn test_put_shard_rejects_u64_overflow_shard_index() { + let mut store = test_store(); + let overflow_index = (i64::MAX as u64) + 1; + let addr = Address::from_parts(Level::from(SHARD_HEIGHT), overflow_index); + let tree = Tree::leaf((test_hash(1), RetentionFlags::MARKED)); + let located = LocatedTree::from_parts(addr, tree).expect("create located"); + let err = store + .put_shard(located) + .expect_err("u64 shard index above i64::MAX should overflow"); + assert_overflow_error(&err, "shard_index"); + } + + #[test] + fn test_get_shard_rejects_u64_overflow_shard_index() { + let store = test_store(); + let overflow_index = (i64::MAX as u64) + 1; + let addr = Address::from_parts(Level::from(SHARD_HEIGHT), overflow_index); + let err = store + .get_shard(addr) + .expect_err("u64 shard index above i64::MAX should overflow"); + assert_overflow_error(&err, "shard_index"); + } + + #[test] + fn test_add_checkpoint_rejects_position_overflow() { + let mut store = test_store(); + let overflow_pos = (i64::MAX as u64) + 1; + let cp = Checkpoint::at_position(Position::from(overflow_pos)); + let err = store + .add_checkpoint(1, cp) + .expect_err("position above i64::MAX should overflow"); + assert_overflow_error(&err, "position"); + + // The failed binding must not have persisted the row. + assert_eq!(store.checkpoint_count().expect("count"), 0); + } + + #[test] + fn test_add_checkpoint_rejects_mark_position_overflow() { + let mut store = test_store(); + let mut marks = BTreeSet::new(); + marks.insert(Position::from((i64::MAX as u64) + 1)); + let cp = Checkpoint::from_parts(TreeState::AtPosition(Position::from(0)), marks); + let err = store + .add_checkpoint(1, cp) + .expect_err("mark position above i64::MAX should overflow"); + assert_overflow_error(&err, "mark position"); + + // The failure must happen before any rows are written. + assert_eq!(store.checkpoint_count().expect("count"), 0); + } + + #[test] + fn test_update_checkpoint_with_rejects_position_overflow() { + let mut store = test_store(); + store + .add_checkpoint(1, Checkpoint::at_position(Position::from(10))) + .expect("add"); + + let overflow_pos = (i64::MAX as u64) + 1; + let err = store + .update_checkpoint_with(&1, |cp| { + *cp = Checkpoint::at_position(Position::from(overflow_pos)); + Ok(()) + }) + .expect_err("position above i64::MAX should overflow"); + assert_overflow_error(&err, "position"); + + // The original checkpoint must be unchanged because the failure happens + // before the rewrite transaction starts. + let loaded = store.get_checkpoint(&1).expect("get").expect("exists"); + assert_eq!( + loaded.tree_state(), + TreeState::AtPosition(Position::from(10)) + ); + } + + #[test] + fn test_update_checkpoint_with_rejects_mark_position_overflow() { + let mut store = test_store(); + let mut original_marks = BTreeSet::new(); + original_marks.insert(Position::from(5)); + let cp = Checkpoint::from_parts(TreeState::AtPosition(Position::from(10)), original_marks); + store.add_checkpoint(1, cp).expect("add"); + + let overflow_pos = (i64::MAX as u64) + 1; + let err = store + .update_checkpoint_with(&1, |cp| { + let mut marks = BTreeSet::new(); + marks.insert(Position::from(overflow_pos)); + *cp = Checkpoint::from_parts(TreeState::AtPosition(Position::from(20)), marks); + Ok(()) + }) + .expect_err("mark position above i64::MAX should overflow"); + assert_overflow_error(&err, "mark position"); + + // The original checkpoint must be unchanged because the failure happens + // before the rewrite transaction starts. + let loaded = store.get_checkpoint(&1).expect("get").expect("exists"); + assert_eq!( + loaded.tree_state(), + TreeState::AtPosition(Position::from(10)) + ); + assert_eq!(loaded.marks_removed().len(), 1); + assert!(loaded.marks_removed().contains(&Position::from(5))); + } + #[test] fn test_update_checkpoint_with_replacement() { let mut store = test_store();