Reject out-of-range enum variant tags during value serialization

aptos-move/e2e-move-tests/src/tests/enum_variant_tag_serialization.rs

@@ -0,0 +1,221 @@
+// Copyright © Aptos Foundation
+// SPDX-License-Identifier: Apache-2.0
+
+//! End-to-end tests that exercise serialization of enum (variant) values whose
+//! layout changes across a package upgrade.
+//!
+//! A `RuntimeVariants` value carries a `u16` variant tag. Serialization must
+//! agree with strict deserialization about which tags are valid: if a value
+//! could be serialized against a layout that does not describe its tag (for
+//! example a variant added by an upgrade), the resulting bytes would be
+//! rejected on read, creating a serialize/deserialize asymmetry that can wedge
+//! state or fail a block.
+//!
+//! These tests upgrade a module to add an enum variant and then store, read
+//! back, and mutate a resource holding that enum at the newly added (higher)
+//! variant tag, driving the value through the full serialize-on-write /
+//! deserialize-on-read path in the VM. (The out-of-range rejection itself is
+//! unit-tested directly in `move-vm-types`, since the bytecode verifier
+//! prevents Move source from ever packing an invalid variant.)
+
+use crate::{assert_success, MoveHarness};
+use aptos_framework::BuildOptions;
+use aptos_language_e2e_tests::account::Account;
+use aptos_package_builder::PackageBuilder;
+use aptos_types::{
+    account_address::AccountAddress,
+    transaction::{SignedTransaction, TransactionStatus},
+};
+use move_core_types::{
+    identifier::Identifier,
+    language_storage::StructTag,
+};
+use serde::Deserialize;
+
+/// Rust mirror of `0x815::m::Data`. Variant order must match the Move
+/// declaration so BCS variant indices line up (`V1` == tag 0, `V2` == tag 1).
+#[derive(Deserialize, Debug, PartialEq)]
+enum Data {
+    V1 { x: u64 },
+    V2 { x: u64, y: u8 },
+}
+
+/// Rust mirror of `0x815::m::Box`.
+#[derive(Deserialize, Debug, PartialEq)]
+struct Box {
+    data: Data,
+}
+
+// Module before the upgrade: a single enum variant `V1`.
+const MODULE_V1: &str = r#"
+    module 0x815::m {
+        enum Data has drop, store {
+            V1 { x: u64 },
+        }
+        struct Box has key {
+            data: Data,
+        }
+        public entry fun init(s: &signer, x: u64) {
+            move_to(s, Box { data: Data::V1 { x } });
+        }
+        public entry fun set_v1(addr: address, x: u64) acquires Box {
+            borrow_global_mut<Box>(addr).data = Data::V1 { x };
+        }
+    }
+"#;
+
+// Module after a compatible upgrade: adds a second enum variant `V2` and a
+// setter that stores a value at the newly introduced (higher) tag.
+const MODULE_V2: &str = r#"
+    module 0x815::m {
+        enum Data has drop, store {
+            V1 { x: u64 },
+            V2 { x: u64, y: u8 },
+        }
+        struct Box has key {
+            data: Data,
+        }
+        public entry fun init(s: &signer, x: u64) {
+            move_to(s, Box { data: Data::V1 { x } });
+        }
+        public entry fun set_v1(addr: address, x: u64) acquires Box {
+            borrow_global_mut<Box>(addr).data = Data::V1 { x };
+        }
+        public entry fun set_v2(addr: address, x: u64, y: u8) acquires Box {
+            borrow_global_mut<Box>(addr).data = Data::V2 { x, y };
+        }
+    }
+"#;
+
+fn box_struct_tag(addr: AccountAddress) -> StructTag {
+    StructTag {
+        address: addr,
+        module: Identifier::new("m").unwrap(),
+        name: Identifier::new("Box").unwrap(),
+        type_args: vec![],
+    }
+}
+
+#[test]
+fn enum_variant_tag_serialization() {
+    let mut h = MoveHarness::new();
+    let addr = AccountAddress::from_hex_literal("0x815").unwrap();
+    let acc = h.new_account_at(addr);
+
+    // Publish V1 and store the resource at the only variant (tag 0). This
+    // serializes the enum value on write.
+    assert_success!(publish(&mut h, &acc, MODULE_V1));
+    assert_success!(h.run_entry_function(
+        &acc,
+        str::parse("0x815::m::init").unwrap(),
+        vec![],
+        vec![bcs::to_bytes(&7u64).unwrap()],
+    ));
+    assert_eq!(
+        h.read_resource::<Box>(&addr, box_struct_tag(addr)).unwrap(),
+        Box {
+            data: Data::V1 { x: 7 }
+        },
+        "V1 value round-trips through serialize-on-write / deserialize-on-read",
+    );
+
+    // Compatible upgrade that adds variant `V2`.
+    assert_success!(publish(&mut h, &acc, MODULE_V2));
+
+    // Overwrite the resource with the newly added variant (tag 1). Serializing
+    // this value drives the `RuntimeVariants` path with a tag that did not exist
+    // in the pre-upgrade layout.
+    assert_success!(h.run_entry_function(
+        &acc,
+        str::parse("0x815::m::set_v2").unwrap(),
+        vec![],
+        vec![
+            bcs::to_bytes(&addr).unwrap(),
+            bcs::to_bytes(&9u64).unwrap(),
+            bcs::to_bytes(&3u8).unwrap(),
+        ],
+    ));
+    assert_eq!(
+        h.read_resource::<Box>(&addr, box_struct_tag(addr)).unwrap(),
+        Box {
+            data: Data::V2 { x: 9, y: 3 }
+        },
+        "newly added variant round-trips after upgrade",
+    );
+
+    // The original variant must still serialize/deserialize after the upgrade.
+    assert_success!(h.run_entry_function(
+        &acc,
+        str::parse("0x815::m::set_v1").unwrap(),
+        vec![],
+        vec![bcs::to_bytes(&addr).unwrap(), bcs::to_bytes(&11u64).unwrap()],
+    ));
+    assert_eq!(
+        h.read_resource::<Box>(&addr, box_struct_tag(addr)).unwrap(),
+        Box {
+            data: Data::V1 { x: 11 }
+        },
+        "pre-existing variant still round-trips after upgrade",
+    );
+}
+
+#[test]
+fn enum_variant_tag_serialization_same_block_as_upgrade() {
+    // The riskiest window is a value carrying a just-added variant tag being
+    // serialized in the same block that introduced the variant, where a stale
+    // cached layout could be observed. Publish V1 and initialize first, then run
+    // the upgrade and a store at the new variant together in one block.
+    let mut h = MoveHarness::new();
+    let addr = AccountAddress::from_hex_literal("0x815").unwrap();
+    let acc = h.new_account_at(addr);
+
+    assert_success!(publish(&mut h, &acc, MODULE_V1));
+    assert_success!(h.run_entry_function(
+        &acc,
+        str::parse("0x815::m::init").unwrap(),
+        vec![],
+        vec![bcs::to_bytes(&1u64).unwrap()],
+    ));
+
+    let upgrade_txn = create_publish_txn(&mut h, &acc, MODULE_V2);
+    let set_v2_txn = h.create_entry_function(
+        &acc,
+        str::parse("0x815::m::set_v2").unwrap(),
+        vec![],
+        vec![
+            bcs::to_bytes(&addr).unwrap(),
+            bcs::to_bytes(&42u64).unwrap(),
+            bcs::to_bytes(&7u8).unwrap(),
+        ],
+    );
+    for status in h.run_block(vec![upgrade_txn, set_v2_txn]) {
+        assert_success!(status);
+    }
+
+    assert_eq!(
+        h.read_resource::<Box>(&addr, box_struct_tag(addr)).unwrap(),
+        Box {
+            data: Data::V2 { x: 42, y: 7 }
+        },
+        "new variant stored in the same block as the upgrade round-trips",
+    );
+}
+
+fn publish(h: &mut MoveHarness, account: &Account, source: &str) -> TransactionStatus {
+    let mut builder = PackageBuilder::new("Package");
+    builder.add_source("m.move", source);
+    let path = builder.write_to_temp().unwrap();
+    h.publish_package_with_options(account, path.path(), BuildOptions::move_2())
+}
+
+fn create_publish_txn(h: &mut MoveHarness, account: &Account, source: &str) -> SignedTransaction {
+    let mut builder = PackageBuilder::new("Package");
+    builder.add_source("m.move", source);
+    let path = builder.write_to_temp().unwrap();
+    h.create_publish_package(
+        account,
+        path.path(),
+        Some(BuildOptions::move_2()),
+        |_| {},
+    )
+}

aptos-move/e2e-move-tests/src/tests/mod.rs

@@ -19,6 +19,7 @@ mod cryptoalgebra;
 mod dependencies;
 mod enum_upgrade;
 mod enum_variant_count;
+mod enum_variant_tag_serialization;
 mod error_map;
 mod events;
 mod fee_payer;

third_party/move/move-vm/types/src/values/serialization_tests.rs

@@ -129,6 +129,55 @@ mod tests {
         );
     }
 
+    #[test]
+    fn enum_out_of_range_variant_tag_is_not_serializable() {
+        // Layout has variants 0, 1, 2. `enum_round_trip_vm_value` already covers
+        // an out-of-range tag whose payload is non-empty — that is rejected by
+        // the field-count check. The dangerous case is an out-of-range tag with
+        // *zero* fields: it matches the (empty) fallback field list and would
+        // otherwise serialize into a unit variant that strict deserialization
+        // rejects, creating a serialize/deserialize asymmetry.
+        let layout = enum_layout();
+
+        // A valid unit variant (tag 1 genuinely has zero fields) must still
+        // serialize and round-trip — the fix must not over-reject.
+        let good_unit_variant = Value::struct_(Struct::pack_variant(1, iter::empty()));
+        let blob = ValueSerDeContext::new(None)
+            .serialize(&good_unit_variant, &layout)
+            .unwrap()
+            .expect("valid unit variant serializes");
+        let de_value = ValueSerDeContext::new(None)
+            .deserialize(&blob, &layout)
+            .expect("valid unit variant deserializes");
+        assert!(
+            good_unit_variant.equals(&de_value).unwrap(),
+            "valid unit variant round-trips"
+        );
+
+        // Zero-field out-of-range tags (just past the end, and far past it) must
+        // fail to serialize rather than emit bytes deserialization would reject.
+        for bad_tag in [3u16, 4, 100, u16::MAX] {
+            let bad_value = Value::struct_(Struct::pack_variant(bad_tag, iter::empty()));
+
+            assert!(
+                ValueSerDeContext::new(None)
+                    .serialize(&bad_value, &layout)
+                    .unwrap()
+                    .is_none(),
+                "zero-field out-of-range tag {} must not serialize",
+                bad_tag
+            );
+
+            // `serialized_size` must agree with `serialize`: it must not report a
+            // size for a value that cannot be serialized.
+            assert_err!(
+                ValueSerDeContext::new(None).serialized_size(&bad_value, &layout),
+                "serialized_size must fail for out-of-range tag {}",
+                bad_tag
+            );
+        }
+    }
+
     // ---------------------------------------------------------------------------
     // Rust cross-serialization tests
 

third_party/move/move-vm/types/src/values/values_impl.rs

@@ -4006,6 +4006,20 @@ impl serde::Serialize for SerializationReadyValue<'_, '_, '_, MoveStructLayout,
     fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
         let mut values = self.value.as_slice();
         if let Some((tag, variant_layouts)) = try_get_variant_field_layouts(self.layout, values) {
+            // Reject a value whose variant tag is out of range for the layout.
+            // Serializing it would emit valid bytes for a variant that strict
+            // deserialization rejects, opening a serialize/deserialize asymmetry
+            // (e.g. after an enum upgrade adds a variant that a stale layout does
+            // not know about).
+            let variant_layouts = match variant_layouts {
+                Some(variant_layouts) => variant_layouts,
+                None => {
+                    return Err(invariant_violation::<S>(format!(
+                        "cannot serialize value {:?} as {:?} -- variant tag {} is out of range",
+                        self.value, self.layout, tag
+                    )));
+                },
+            };
             let tag_idx = tag as usize;
             let variant_tag = tag_idx as u32;
             let variant_names = value::variant_name_placeholder((tag + 1) as usize)
@@ -4882,6 +4896,12 @@ impl ValueImpl {
                 if let Some((tag, variant_layouts)) =
                     try_get_variant_field_layouts(struct_layout, values)
                 {
+                    // This conversion is infallible and only feeds best-effort
+                    // consumers such as `debug::print`. An out-of-range tag
+                    // cannot occur for a well-formed value; if one somehow does,
+                    // fall back to no field layouts rather than panicking. The
+                    // enforced check lives on the BCS serialization path above.
+                    let variant_layouts = variant_layouts.unwrap_or(&[]);
                     MoveValue::Struct(MoveStruct::new_variant(
                         tag,
                         values
@@ -4945,13 +4965,24 @@ impl Value {
     }
 }
 
+/// If `layout` is an enum (variant) layout and `values` begins with a variant
+/// tag, returns that tag together with the field layouts of the selected
+/// variant.
+///
+/// The inner `Option` is `None` precisely when the tag is out of range for the
+/// layout — a case that is otherwise indistinguishable from a valid zero-field
+/// variant. Callers MUST treat an out-of-range tag as an error and must not
+/// fall back to an empty variant: doing so would let a value serialize against
+/// a layout that does not describe it (e.g. a layout cached from before an enum
+/// upgrade introduced the variant), producing bytes that strict deserialization
+/// rejects.
 fn try_get_variant_field_layouts<'a>(
     layout: &'a MoveStructLayout,
     values: &[ValueImpl],
-) -> Option<(u16, &'a [MoveTypeLayout])> {
-    if matches!(layout, MoveStructLayout::RuntimeVariants(..)) {
+) -> Option<(u16, Option<&'a [MoveTypeLayout]>)> {
+    if let MoveStructLayout::RuntimeVariants(variants) = layout {
         if let Some(ValueImpl::U16(tag)) = values.first() {
-            return Some((*tag, layout.fields(Some(*tag as usize))));
+            return Some((*tag, variants.get(*tag as usize).map(Vec::as_slice)));
         }
     }
     None