From 22406c8c4517af69d30d9bc4c6d758fa8f1129b9 Mon Sep 17 00:00:00 2001 From: Sean Young Date: Thu, 6 Aug 2026 14:33:51 +0100 Subject: [PATCH] Reject out-of-range enum variant tags during value serialization --- .../tests/enum_variant_tag_serialization.rs | 221 ++++++++++++++++++ aptos-move/e2e-move-tests/src/tests/mod.rs | 1 + .../types/src/values/serialization_tests.rs | 49 ++++ .../move-vm/types/src/values/values_impl.rs | 37 ++- 4 files changed, 305 insertions(+), 3 deletions(-) create mode 100644 aptos-move/e2e-move-tests/src/tests/enum_variant_tag_serialization.rs diff --git a/aptos-move/e2e-move-tests/src/tests/enum_variant_tag_serialization.rs b/aptos-move/e2e-move-tests/src/tests/enum_variant_tag_serialization.rs new file mode 100644 index 00000000000..ce315772ab3 --- /dev/null +++ b/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(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(addr).data = Data::V1 { x }; + } + public entry fun set_v2(addr: address, x: u64, y: u8) acquires Box { + borrow_global_mut(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::(&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::(&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::(&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::(&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()), + |_| {}, + ) +} diff --git a/aptos-move/e2e-move-tests/src/tests/mod.rs b/aptos-move/e2e-move-tests/src/tests/mod.rs index c432ce73def..9ea95e79242 100644 --- a/aptos-move/e2e-move-tests/src/tests/mod.rs +++ b/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; diff --git a/third_party/move/move-vm/types/src/values/serialization_tests.rs b/third_party/move/move-vm/types/src/values/serialization_tests.rs index f6a04e963ad..21bf65bdc55 100644 --- a/third_party/move/move-vm/types/src/values/serialization_tests.rs +++ b/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 diff --git a/third_party/move/move-vm/types/src/values/values_impl.rs b/third_party/move/move-vm/types/src/values/values_impl.rs index 1859ba704f4..c1ecf88ceaf 100644 --- a/third_party/move/move-vm/types/src/values/values_impl.rs +++ b/third_party/move/move-vm/types/src/values/values_impl.rs @@ -4006,6 +4006,20 @@ impl serde::Serialize for SerializationReadyValue<'_, '_, '_, MoveStructLayout, fn serialize(&self, serializer: S) -> Result { 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::(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