OpenSK icon indicating copy to clipboard operation
OpenSK copied to clipboard

Ordering invariant does not hold between `cbor_map!` and `destructure_cbor_map!` on the develop branch

Open gendx opened this issue 4 years ago • 0 comments

Expected Behavior

In the develop branch, the cbor_map! macro's documentation indicates that keys do not need to be sorted: https://github.com/google/OpenSK/blob/develop/libraries/cbor/src/macros.rs#L143.

It is therefore expected that a map built like:

let map = cbor_map! {
    1 => 10,
    2 => 20,
};

can be correctly used with destructure_cbor_map! as follows:

    #[test]
    fn test_destructure_unsorted_cbor_map() {
        let map = cbor_map! {
            2 => 20,
            1 => 10,
        };

        destructure_cbor_map! {
            let {
                1 => x1,
                2 => x2,
            } = extract_map(map);
        }

        assert_eq!(x1, Some(cbor_unsigned!(10)));
        assert_eq!(x2, Some(cbor_unsigned!(20)));
    }

Actual Behavior

This test fails with:

failures:

---- macros::test::test_destructure_unsorted_cbor_map stdout ----
thread 'macros::test::test_destructure_unsorted_cbor_map' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(Unsigned(10))`', src/macros.rs:667:9

This is because destructure_cbor_map! expects that both the destructured keys (left-hand-side of the arrow) and the keys in the map itself are sorted. The former constraint is well documented by destructure_cbor_map! (and checked via unit tests for lack of static assertions), but the latter does not hold if inputs are not sorted in cbor_map! (which just copies the key-value pairs into a vectors without doing any sorting).

Affected code

  1. #303 did change a bunch of cbor_map! expressions to actually have sorted keys in their inputs, so I don't think the OpenSK usage of sk-cbor is affected.
  2. The stable branch shouldn't be affected, because it's still using the BTreeMap representation of map values (https://github.com/google/OpenSK/pull/303 wasn't applied).
  3. A quick look at the dependents of the sk-cbor crate (https://crates.io/crates/sk-cbor/reverse_dependencies), as well as a global GitHub search (https://github.com/search?q=destructure_cbor_map&type=Code) shows that destructure_cbor_map! does not seem to be used outside of OpenSK.

So there doesn't seem to be code affected by this bug at the moment.

Steps to Reproduce the Problem

  1. Apply the following patch to the develop branch:
$ git diff
diff --git a/libraries/cbor/src/macros.rs b/libraries/cbor/src/macros.rs
index d7e1185..ca9ae02 100644
--- a/libraries/cbor/src/macros.rs
+++ b/libraries/cbor/src/macros.rs
@@ -650,6 +650,24 @@ mod test {
         assert_eq!(x2, Some(cbor_unsigned!(20)));
     }
 
+    #[test]
+    fn test_destructure_unsorted_cbor_map() {
+        let map = cbor_map! {
+            2 => 20,
+            1 => 10,
+        };
+
+        destructure_cbor_map! {
+            let {
+                1 => x1,
+                2 => x2,
+            } = extract_map(map);
+        }
+
+        assert_eq!(x1, Some(cbor_unsigned!(10)));
+        assert_eq!(x2, Some(cbor_unsigned!(20)));
+    }
+
     #[test]
     #[should_panic]
     fn test_destructure_cbor_map_unsorted() {
  1. cd libraries/cbor && cargo test.

Specifications

  • Version: develop branch
  • Platform: N/A

gendx avatar Dec 15 '21 09:12 gendx