Merge pull request #84 from turbofish-org/fixes

Misc fixes

docs/algorithms.md

@@ -29,7 +29,7 @@ node_hash = H(kv_hash, left_child_hash, right_child_hash)
 
 Note that the `left_child_hash` and/or `right_child_hash` values may be null since it is possible for the node to have no children or only one child.
 
-In our implementation, the hash function used is Blake2b (chosen for its performance characteristics) but this choice is trivially swappable.
+In our implementation, the hash function used is SHA512/256 (SHA512 with output truncated to 256 bits) but this choice is trivially swappable.
 
 #### Database Representation
 

src/merk/restore.rs

@@ -173,8 +173,11 @@ impl Restorer {
             0
         };
 
-        // FIXME: this one shouldn't be an assert because it comes from a peer
-        assert_eq!(self.stated_length, chunks_remaining + 1);
+        if self.stated_length != chunks_remaining + 1 {
+            return Err(Error::ChunkProcessing(
+                "Stated length does not match calculated number of chunks".into(),
+            ));
+        }
 
         // note that these writes don't happen atomically, which is fine here
         // because if anything fails during the restore process we will just

src/proofs/chunk.rs

@@ -197,17 +197,20 @@ pub(crate) fn verify_leaf<I: Iterator<Item = Result<Op>>>(
 #[cfg(feature = "full")]
 pub(crate) fn verify_trunk<I: Iterator<Item = Result<Op>>>(ops: I) -> Result<(ProofTree, usize)> {
     fn verify_height_proof(tree: &ProofTree) -> Result<usize> {
-        Ok(match tree.child(true) {
-            Some(child) => {
-                if let Node::Hash(_) = child.tree.node {
-                    return Err(Error::UnexpectedNode(
-                        "Expected height proof to only contain KV and KVHash nodes".into(),
-                    ));
-                }
-                verify_height_proof(&child.tree)? + 1
+        let mut height = 1;
+        let mut cursor = tree;
+        while let Some(child) = cursor.child(true) {
+            if let Node::Hash(_) = child.tree.node {
+                return Err(Error::UnexpectedNode(
+                    "Expected height proof to only contain KV and KVHash
+        nodes"
+                        .into(),
+                ));
             }
-            None => 1,
-        })
+            height += 1;
+            cursor = &child.tree;
+        }
+        Ok(height)
     }
 
     fn verify_completeness(tree: &ProofTree, remaining_depth: usize, leftmost: bool) -> Result<()> {
@@ -253,6 +256,11 @@ pub(crate) fn verify_trunk<I: Iterator<Item = Result<Op>>>(ops: I) -> Result<(Pr
     })?;
 
     let height = verify_height_proof(&tree)?;
+    if height > 64 {
+        // This is a sanity check to prevent stack overflows in `verify_completeness`,
+        // but any tree above 64 is probably an error (~3.7e19 nodes).
+        return Err(Error::Tree("Tree is too large".into()));
+    }
     let trunk_height = height / 2;
 
     if trunk_height < MIN_TRUNK_HEIGHT {
@@ -268,12 +276,11 @@ pub(crate) fn verify_trunk<I: Iterator<Item = Result<Op>>>(ops: I) -> Result<(Pr
 
 #[cfg(test)]
 mod tests {
-    use std::usize;
-
     use super::super::tree::Tree;
     use super::*;
     use crate::test_utils::*;
     use crate::tree::{NoopCommit, PanicSource, Tree as BaseTree};
+    use ed::Encode;
 
     #[derive(Default)]
     struct NodeCounts {
@@ -467,4 +474,21 @@ mod tests {
         assert_eq!(counts.hash, 0);
         assert_eq!(counts.kvhash, 0);
     }
+
+    #[test]
+    #[should_panic(expected = "Tree is too large")]
+    fn test_verify_height_stack_overflow() {
+        let height = 5_000u32;
+        let push_op = |i: u32| Op::Push(Node::KV(i.to_be_bytes().to_vec(), vec![]));
+        let mut ops = Vec::with_capacity((height * 2) as usize);
+        ops.push(push_op(0));
+        for i in 1..height {
+            ops.push(push_op(i));
+            ops.push(Op::Parent)
+        }
+        assert!(ops.encoding_length().unwrap() < 50_000);
+        println!("Len: {}", ops.encoding_length().unwrap());
+        let (_, result_height) = verify_trunk(ops.into_iter().map(Ok)).unwrap();
+        assert_eq!(height, result_height as u32);
+    }
 }

src/proofs/query/mod.rs

@@ -474,6 +474,7 @@ mod test {
     use super::*;
     use crate::test_utils::make_tree_seq;
     use crate::tree::{NoopCommit, PanicSource, RefWalker, Tree};
+    use ed::Encode;
 
     fn make_3_node_tree() -> Result<Tree> {
         let mut tree = Tree::new(vec![5], vec![5])?
@@ -1477,4 +1478,18 @@ mod test {
 
         let _result = verify_query(bytes.as_slice(), &query, [42; 32]).expect("verify failed");
     }
+
+    #[test]
+    #[should_panic(expected = "Tried to attach to Hash node")]
+    fn hash_attach() {
+        let mut target = make_3_node_tree().expect("tree construction failed");
+
+        let mut proof = Vec::new();
+        proof.push(Op::Push(Node::KV(vec![42], vec![42])));
+        proof.push(Op::Push(Node::Hash(target.hash())));
+        proof.push(Op::Parent);
+
+        let map = verify(&proof.encode().unwrap(), target.hash()).unwrap();
+        assert_eq!(map.get(&[42]).unwrap().unwrap(), &[42])
+    }
 }

src/proofs/tree.rs

@@ -116,15 +116,19 @@ impl Tree {
         }
     }
 
-    /// Attaches the child to the `Tree`'s given side. Panics if there is
-    /// already a child attached to this side.
+    /// Attaches the child to the `Tree`'s given side. Returns an error if
+    /// there is already a child attached to this side.
     pub(crate) fn attach(&mut self, left: bool, child: Tree) -> Result<()> {
         if self.child(left).is_some() {
             return Err(Error::Attach(
                 "Tried to attach to left child, but it is already Some".into(),
             ));
         }
 
+        if let Node::Hash(_) = self.node {
+            return Err(Error::Attach("Tried to attach to Hash node".into()));
+        }
+
         self.height = self.height.max(child.height + 1);
 
         let hash = child.hash()?;

src/tree/hash.rs

@@ -14,9 +14,6 @@ pub const NULL_HASH: Hash = [0; HASH_LENGTH];
 pub type Hash = [u8; HASH_LENGTH];
 
 /// Hashes a key/value pair.
-///
-/// **NOTE:** This will fail if the key is longer than 255 bytes, or the value
-/// is longer than 65,535 bytes.
 pub fn kv_hash<D: Digest>(key: &[u8], value: &[u8]) -> Result<Hash, TryFromIntError> {
     let mut hasher = D::new();
     hasher.update([0]);