From 86492762c2c8af6cb132f994146ec6137ce56dbd Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Thu, 2 Jul 2026 21:35:17 +0200 Subject: [PATCH 1/2] Validate DynamicMessage values before set --- .../unreleased/fixed-20260702-212901.yaml | 3 + buffa-descriptor/src/reflect/dynamic.rs | 265 ++++++++++++++++-- buffa-descriptor/src/reflect/message.rs | 53 +++- buffa-descriptor/tests/dynamic_e2e.rs | 135 +++++++++ 4 files changed, 432 insertions(+), 24 deletions(-) create mode 100644 .changes/unreleased/fixed-20260702-212901.yaml diff --git a/.changes/unreleased/fixed-20260702-212901.yaml b/.changes/unreleased/fixed-20260702-212901.yaml new file mode 100644 index 0000000..2976545 --- /dev/null +++ b/.changes/unreleased/fixed-20260702-212901.yaml @@ -0,0 +1,3 @@ +kind: Fixed +body: '`DynamicMessage::try_set` now rejects `Value`s whose runtime shape does not match the target field descriptor, including repeated elements, map keys and values, and nested message descriptors. Invalid values left through direct mutable field access are skipped during encode instead of producing tag-only corrupt wire output.' +time: 2026-07-02T19:29:06.000000000Z diff --git a/buffa-descriptor/src/reflect/dynamic.rs b/buffa-descriptor/src/reflect/dynamic.rs index 7dd8e03..4833180 100644 --- a/buffa-descriptor/src/reflect/dynamic.rs +++ b/buffa-descriptor/src/reflect/dynamic.rs @@ -13,6 +13,7 @@ use alloc::borrow::ToOwned; use alloc::collections::BTreeMap; +use alloc::format; use alloc::string::String; use alloc::sync::Arc; use alloc::vec::Vec; @@ -193,6 +194,21 @@ impl DynamicMessage { } } + fn validate_field_value( + &self, + field: &FieldDescriptor, + value: &Value, + ) -> Result<(), ReflectError> { + validate_value_shape(field.kind, value, &self.pool).map_err(|err| { + ReflectError::wrong_value_kind( + self.message_descriptor(), + field, + err.expected, + err.actual, + ) + }) + } + /// Decode wire bytes against the descriptor. /// /// # Errors @@ -556,7 +572,7 @@ impl DynamicMessage { if should_skip_on_encode(fd, value) { continue; } - encode_field(fd, value, buf); + encode_field(fd, value, &self.pool, buf); } } self.unknown.write_to(buf); @@ -579,7 +595,7 @@ impl DynamicMessage { if should_skip_on_encode(fd, value) { continue; } - len += encoded_field_len(fd, value); + len += encoded_field_len(fd, value, &self.pool); } } len @@ -661,8 +677,9 @@ impl DynamicMessage { /// ``` /// /// Replacing the value with one whose shape doesn't match the field's - /// kind is permitted (the same latitude as `set`); the encoder defensively - /// skips a shape-mismatched value. + /// kind bypasses [`ReflectMessageMut::try_set`]'s validation. The encoder + /// defensively skips invalid stored values, but callers that replace a + /// whole value should prefer `try_set`. #[must_use] pub fn field_by_number_mut(&mut self, number: u32) -> Option<&mut Value> { self.fields.get_mut(&number) @@ -963,6 +980,7 @@ impl ReflectMessage for DynamicMessage { impl ReflectMessageMut for DynamicMessage { fn try_set(&mut self, field: &FieldDescriptor, value: Value) -> Result<(), ReflectError> { self.validate_field_descriptor(field)?; + self.validate_field_value(field, &value)?; // Setting a oneof member must clear its siblings, otherwise a // subsequent encode writes multiple oneof members onto the wire, // which violates the proto spec. The wire decoder and the JSON @@ -1022,6 +1040,199 @@ fn should_skip_on_encode(fd: &FieldDescriptor, value: &Value) -> bool { } } +#[derive(Debug)] +struct ValueShapeMismatch { + expected: String, + actual: String, +} + +fn validate_value_shape( + kind: FieldKind, + value: &Value, + pool: &Arc, +) -> Result<(), ValueShapeMismatch> { + match kind { + FieldKind::Singular(sk) => validate_singular_shape(sk, value, pool), + FieldKind::List(sk) => { + let Value::List(items) = value else { + return Err(field_shape_mismatch(kind, pool, value_shape(value))); + }; + for item in items { + validate_singular_shape(sk, item, pool).map_err(|err| { + field_shape_mismatch(kind, pool, format!("list element {}", err.actual)) + })?; + } + Ok(()) + } + FieldKind::Map { key, value: vk } => { + let Value::Map(entries) = value else { + return Err(field_shape_mismatch(kind, pool, value_shape(value))); + }; + for (map_key, map_value) in entries { + if !map_key_matches_scalar(key, map_key) { + return Err(field_shape_mismatch( + kind, + pool, + format!("map key {}", map_key_shape(map_key)), + )); + } + validate_singular_shape(vk, map_value, pool).map_err(|err| { + field_shape_mismatch(kind, pool, format!("map value {}", err.actual)) + })?; + } + Ok(()) + } + } +} + +fn validate_singular_shape( + kind: SingularKind, + value: &Value, + pool: &Arc, +) -> Result<(), ValueShapeMismatch> { + let matches = match kind { + SingularKind::Scalar(s) => value_matches_scalar(s, value), + SingularKind::Enum(_) => matches!(value, Value::EnumNumber(_)), + SingularKind::Message(midx) => match value { + Value::Message(msg) => Arc::ptr_eq(&msg.pool, pool) && msg.msg_idx == midx, + _ => false, + }, + }; + if matches { + Ok(()) + } else { + Err(ValueShapeMismatch { + expected: singular_kind_name(kind, pool), + actual: value_shape(value), + }) + } +} + +fn field_shape_mismatch( + kind: FieldKind, + pool: &DescriptorPool, + actual: String, +) -> ValueShapeMismatch { + ValueShapeMismatch { + expected: field_kind_name(kind, pool), + actual, + } +} + +fn field_kind_name(kind: FieldKind, pool: &DescriptorPool) -> String { + match kind { + FieldKind::Singular(sk) => singular_kind_name(sk, pool), + FieldKind::List(sk) => format!("list<{}>", singular_kind_name(sk, pool)), + FieldKind::Map { key, value } => { + format!( + "map<{}, {}>", + scalar_type_name(key), + singular_kind_name(value, pool) + ) + } + } +} + +fn value_matches_field_shape(kind: FieldKind, value: &Value, pool: &Arc) -> bool { + validate_value_shape(kind, value, pool).is_ok() +} + +fn value_matches_scalar(s: ScalarType, value: &Value) -> bool { + matches!( + (s, value), + (ScalarType::Double, Value::F64(_)) + | (ScalarType::Float, Value::F32(_)) + | ( + ScalarType::Int64 | ScalarType::Sfixed64 | ScalarType::Sint64, + Value::I64(_) + ) + | (ScalarType::Uint64 | ScalarType::Fixed64, Value::U64(_)) + | ( + ScalarType::Int32 | ScalarType::Sfixed32 | ScalarType::Sint32, + Value::I32(_) + ) + | (ScalarType::Uint32 | ScalarType::Fixed32, Value::U32(_)) + | (ScalarType::Bool, Value::Bool(_)) + | (ScalarType::String, Value::String(_)) + | (ScalarType::Bytes, Value::Bytes(_)) + ) +} + +fn map_key_matches_scalar(s: ScalarType, key: &MapKey) -> bool { + matches!( + (s, key), + (ScalarType::Bool, MapKey::Bool(_)) + | ( + ScalarType::Int32 | ScalarType::Sint32 | ScalarType::Sfixed32, + MapKey::I32(_) + ) + | ( + ScalarType::Int64 | ScalarType::Sint64 | ScalarType::Sfixed64, + MapKey::I64(_) + ) + | (ScalarType::Uint32 | ScalarType::Fixed32, MapKey::U32(_)) + | (ScalarType::Uint64 | ScalarType::Fixed64, MapKey::U64(_)) + | (ScalarType::String, MapKey::String(_)) + ) +} + +fn singular_kind_name(kind: SingularKind, pool: &DescriptorPool) -> String { + match kind { + SingularKind::Scalar(s) => scalar_type_name(s).into(), + SingularKind::Enum(eidx) => format!("enum {}", pool.enumeration(eidx).full_name()), + SingularKind::Message(midx) => format!("message {}", pool.message(midx).full_name()), + } +} + +fn scalar_type_name(s: ScalarType) -> &'static str { + match s { + ScalarType::Double => "double", + ScalarType::Float => "float", + ScalarType::Int64 => "int64", + ScalarType::Uint64 => "uint64", + ScalarType::Int32 => "int32", + ScalarType::Fixed64 => "fixed64", + ScalarType::Fixed32 => "fixed32", + ScalarType::Bool => "bool", + ScalarType::String => "string", + ScalarType::Bytes => "bytes", + ScalarType::Uint32 => "uint32", + ScalarType::Sfixed32 => "sfixed32", + ScalarType::Sfixed64 => "sfixed64", + ScalarType::Sint32 => "sint32", + ScalarType::Sint64 => "sint64", + } +} + +fn value_shape(value: &Value) -> String { + match value { + Value::Bool(_) => "bool".into(), + Value::I32(_) => "i32".into(), + Value::I64(_) => "i64".into(), + Value::U32(_) => "u32".into(), + Value::U64(_) => "u64".into(), + Value::F32(_) => "f32".into(), + Value::F64(_) => "f64".into(), + Value::String(_) => "string".into(), + Value::Bytes(_) => "bytes".into(), + Value::EnumNumber(_) => "enum number".into(), + Value::Message(msg) => format!("message {}", msg.message_descriptor().full_name()), + Value::List(_) => "list".into(), + Value::Map(_) => "map".into(), + } +} + +fn map_key_shape(key: &MapKey) -> &'static str { + match key { + MapKey::Bool(_) => "bool", + MapKey::I32(_) => "i32", + MapKey::I64(_) => "i64", + MapKey::U32(_) => "u32", + MapKey::U64(_) => "u64", + MapKey::String(_) => "string", + } +} + /// Whether a `Value` is its proto type's default. Used for implicit-presence /// encode skipping. Floats use `to_bits()` so -0.0 is treated as non-default. fn is_default_scalar(v: &Value) -> bool { @@ -1190,7 +1401,15 @@ fn default_scalar_ref(s: ScalarType) -> ValueRef<'static> { // ── Encode helpers ────────────────────────────────────────────────────────── -fn encode_field(fd: &FieldDescriptor, value: &Value, buf: &mut impl BufMut) { +fn encode_field( + fd: &FieldDescriptor, + value: &Value, + pool: &Arc, + buf: &mut impl BufMut, +) { + if !value_matches_field_shape(fd.kind, value, pool) { + return; + } match (&fd.kind, value) { (FieldKind::Singular(sk), v) => { encode_singular_with_tag(fd.number, *sk, fd.delimited, v, buf); @@ -1228,13 +1447,15 @@ fn encode_field(fd: &FieldDescriptor, value: &Value, buf: &mut impl BufMut) { } } _ => { - // Stored value's shape doesn't match the descriptor's kind — - // can happen if a consumer set() a mismatched Value. Skip. + // `value_matches_field_shape` checked this above. } } } -fn encoded_field_len(fd: &FieldDescriptor, value: &Value) -> usize { +fn encoded_field_len(fd: &FieldDescriptor, value: &Value, pool: &Arc) -> usize { + if !value_matches_field_shape(fd.kind, value, pool) { + return 0; + } match (&fd.kind, value) { (FieldKind::Singular(sk), v) => singular_len_with_tag(fd.number, *sk, fd.delimited, v), (FieldKind::List(sk), Value::List(items)) => { @@ -1323,11 +1544,17 @@ fn encode_singular_with_tag( value: &Value, buf: &mut impl BufMut, ) { - Tag::new(number, singular_wire_type(kind, delimited)).encode(buf); match (kind, value) { - (SingularKind::Scalar(s), v) => encode_scalar(s, v, buf), - (SingularKind::Enum(_), Value::EnumNumber(n)) => encode_int32(*n, buf), + (SingularKind::Scalar(s), v) if value_matches_scalar(s, v) => { + Tag::new(number, singular_wire_type(kind, delimited)).encode(buf); + encode_scalar(s, v, buf); + } + (SingularKind::Enum(_), Value::EnumNumber(n)) => { + Tag::new(number, singular_wire_type(kind, delimited)).encode(buf); + encode_int32(*n, buf); + } (SingularKind::Message(_), Value::Message(m)) => { + Tag::new(number, singular_wire_type(kind, delimited)).encode(buf); if delimited { m.encode(buf); Tag::new(number, WireType::EndGroup).encode(buf); @@ -1337,14 +1564,14 @@ fn encode_singular_with_tag( m.encode(buf); } } - _ => {} // shape mismatch — already wrote a tag, but no payload follows; corrupt output is the consumer's problem + _ => {} } } fn singular_len_with_tag(number: u32, kind: SingularKind, delimited: bool, value: &Value) -> usize { let tag_bytes = tag_len(number, singular_wire_type(kind, delimited)); let payload = match (kind, value) { - (SingularKind::Scalar(s), v) => scalar_len(s, v), + (SingularKind::Scalar(s), v) if value_matches_scalar(s, v) => scalar_len(s, v), (SingularKind::Enum(_), Value::EnumNumber(n)) => int32_encoded_len(*n), (SingularKind::Message(_), Value::Message(m)) => { if delimited { @@ -1355,7 +1582,7 @@ fn singular_len_with_tag(number: u32, kind: SingularKind, delimited: bool, value uint64_encoded_len(inner as u64) + inner } } - _ => 0, + _ => return 0, }; tag_bytes + payload } @@ -1383,8 +1610,10 @@ fn encode_scalar(s: ScalarType, v: &Value, buf: &mut impl BufMut) { fn scalar_len(s: ScalarType, v: &Value) -> usize { match (s, v) { - (ScalarType::Double | ScalarType::Fixed64 | ScalarType::Sfixed64, _) => 8, - (ScalarType::Float | ScalarType::Fixed32 | ScalarType::Sfixed32, _) => 4, + (ScalarType::Double, Value::F64(_)) => 8, + (ScalarType::Fixed64, Value::U64(_)) | (ScalarType::Sfixed64, Value::I64(_)) => 8, + (ScalarType::Float, Value::F32(_)) => 4, + (ScalarType::Fixed32, Value::U32(_)) | (ScalarType::Sfixed32, Value::I32(_)) => 4, (ScalarType::Bool, Value::Bool(_)) => buffa::types::BOOL_ENCODED_LEN, (ScalarType::Int64, Value::I64(x)) => int64_encoded_len(*x), (ScalarType::Uint64, Value::U64(x)) => uint64_encoded_len(*x), @@ -1457,8 +1686,8 @@ fn encode_map_key(s: ScalarType, k: &MapKey, buf: &mut impl BufMut) { fn map_key_len(s: ScalarType, k: &MapKey) -> usize { match (s, k) { - (ScalarType::Fixed32 | ScalarType::Sfixed32, _) => 4, - (ScalarType::Fixed64 | ScalarType::Sfixed64, _) => 8, + (ScalarType::Fixed32, MapKey::U32(_)) | (ScalarType::Sfixed32, MapKey::I32(_)) => 4, + (ScalarType::Fixed64, MapKey::U64(_)) | (ScalarType::Sfixed64, MapKey::I64(_)) => 8, (ScalarType::Bool, MapKey::Bool(_)) => buffa::types::BOOL_ENCODED_LEN, (ScalarType::Int32, MapKey::I32(x)) => int32_encoded_len(*x), (ScalarType::Sint32, MapKey::I32(x)) => sint32_encoded_len(*x), diff --git a/buffa-descriptor/src/reflect/message.rs b/buffa-descriptor/src/reflect/message.rs index 74122f7..df02fa6 100644 --- a/buffa-descriptor/src/reflect/message.rs +++ b/buffa-descriptor/src/reflect/message.rs @@ -48,6 +48,20 @@ pub enum ReflectError { /// The foreign descriptor's field number. number: u32, }, + /// The supplied value's runtime shape does not match the target field's + /// descriptor. + WrongValueKind { + /// The message being mutated. + message: String, + /// The target field's simple field name. + field_name: String, + /// The target field's field number. + number: u32, + /// Human-readable descriptor shape expected by the field. + expected: String, + /// Human-readable runtime shape supplied by the caller. + actual: String, + }, } impl ReflectError { @@ -58,6 +72,21 @@ impl ReflectError { number: field.number(), } } + + pub(crate) fn wrong_value_kind( + message: &MessageDescriptor, + field: &FieldDescriptor, + expected: String, + actual: String, + ) -> Self { + Self::WrongValueKind { + message: message.full_name().to_string(), + field_name: field.name().to_string(), + number: field.number(), + expected, + actual, + } + } } impl core::fmt::Display for ReflectError { @@ -71,6 +100,16 @@ impl core::fmt::Display for ReflectError { f, "field descriptor {field_name:?} (#{number}) is not a member of {message}" ), + Self::WrongValueKind { + message, + field_name, + number, + expected, + actual, + } => write!( + f, + "field {field_name:?} (#{number}) on {message} expects {expected}, got {actual}" + ), } } } @@ -200,10 +239,11 @@ pub trait ReflectMessageMut: ReflectMessage { /// The default implementation performs **no validation** — it forwards /// to `set` and returns `Ok(())`, so on an implementation that has not /// overridden it this can panic exactly where `set` would. - /// Implementations that can validate field-descriptor membership should - /// override it and return [`ReflectError::FieldNotMember`] rather than - /// mutating a colliding field number by accident ([`DynamicMessage`] - /// does). + /// Implementations that can validate field-descriptor membership or + /// runtime value shape should override it and return + /// [`ReflectError::FieldNotMember`] or + /// [`ReflectError::WrongValueKind`] rather than mutating invalid state + /// ([`DynamicMessage`] does both). fn try_set( &mut self, field: &FieldDescriptor, @@ -220,8 +260,9 @@ pub trait ReflectMessageMut: ReflectMessage { /// /// # Panics /// - /// May panic if `field` is not a member of this message's descriptor. - /// Use [`try_set`](Self::try_set) when membership is not already proven. + /// May panic if `field` is not a member of this message's descriptor or + /// `value` does not match the field kind. Use [`try_set`](Self::try_set) + /// when membership or value shape is not already proven. fn set(&mut self, field: &FieldDescriptor, value: super::Value); /// Checked variant of [`clear`](Self::clear). diff --git a/buffa-descriptor/tests/dynamic_e2e.rs b/buffa-descriptor/tests/dynamic_e2e.rs index 0ef3d71..a3bf8df 100644 --- a/buffa-descriptor/tests/dynamic_e2e.rs +++ b/buffa-descriptor/tests/dynamic_e2e.rs @@ -111,6 +111,141 @@ fn dynamic_message_containers_round_trip() { assert_eq!(msg.encoded_len(), bytes.len()); } +#[test] +fn try_set_rejects_values_that_do_not_match_field_kind() { + let p = pool(); + let scalars_idx = p.message_index("reflect.test.Scalars").unwrap(); + let scalars = p.message_by_name("reflect.test.Scalars").unwrap(); + let mut msg = DynamicMessage::new(Arc::clone(&p), scalars_idx); + + msg.set(scalars.field(3).unwrap(), Value::I32(7)); + let err = msg + .try_set(scalars.field(3).unwrap(), Value::String("bad".into())) + .unwrap_err(); + assert!(matches!( + err, + ReflectError::WrongValueKind { + ref message, + ref field_name, + number, + ref expected, + ref actual, + } if message == "reflect.test.Scalars" + && field_name == "f_int32" + && number == 3 + && expected == "int32" + && actual == "string" + )); + assert_eq!(msg.field_by_number(3), Some(&Value::I32(7))); +} + +#[test] +fn try_set_rejects_values_with_mismatched_container_contents() { + let p = pool(); + let containers_idx = p.message_index("reflect.test.Containers").unwrap(); + let md = p.message_by_name("reflect.test.Containers").unwrap(); + let mut msg = DynamicMessage::new(Arc::clone(&p), containers_idx); + + let err = msg + .try_set( + md.field(1).unwrap(), + Value::List(vec![Value::I32(1), Value::String("bad".into())]), + ) + .unwrap_err(); + assert!(matches!( + err, + ReflectError::WrongValueKind { + ref field_name, + ref expected, + ref actual, + .. + } if field_name == "packed_ints" && expected == "list" && actual == "list element string" + )); + + let mut wrong_key = MapValue::new(); + wrong_key.insert(MapKey::I32(1), Value::I32(7)); + let err = msg + .try_set(md.field(3).unwrap(), Value::Map(wrong_key)) + .unwrap_err(); + assert!(matches!( + err, + ReflectError::WrongValueKind { + ref field_name, + ref expected, + ref actual, + .. + } if field_name == "tags" && expected == "map" && actual == "map key i32" + )); + + let mut wrong_value = MapValue::new(); + wrong_value.insert(MapKey::String("k".into()), Value::String("bad".into())); + let err = msg + .try_set(md.field(3).unwrap(), Value::Map(wrong_value)) + .unwrap_err(); + assert!(matches!( + err, + ReflectError::WrongValueKind { + ref field_name, + ref expected, + ref actual, + .. + } if field_name == "tags" + && expected == "map" + && actual == "map value string" + )); +} + +#[test] +fn try_set_rejects_message_values_with_wrong_descriptor() { + let p = pool(); + let containers_idx = p.message_index("reflect.test.Containers").unwrap(); + let scalars_idx = p.message_index("reflect.test.Scalars").unwrap(); + let md = p.message_by_name("reflect.test.Containers").unwrap(); + + let mut msg = DynamicMessage::new(Arc::clone(&p), containers_idx); + let wrong_message = DynamicMessage::new(Arc::clone(&p), scalars_idx); + let err = msg + .try_set(md.field(5).unwrap(), Value::Message(wrong_message)) + .unwrap_err(); + assert!(matches!( + err, + ReflectError::WrongValueKind { + ref field_name, + ref expected, + ref actual, + .. + } if field_name == "nested" + && expected == "message reflect.test.Inner" + && actual == "message reflect.test.Scalars" + )); +} + +#[test] +#[should_panic(expected = "expects int32, got string")] +fn set_panics_if_value_does_not_match_field_kind() { + let p = pool(); + let scalars_idx = p.message_index("reflect.test.Scalars").unwrap(); + let scalars = p.message_by_name("reflect.test.Scalars").unwrap(); + let mut msg = DynamicMessage::new(Arc::clone(&p), scalars_idx); + + msg.set(scalars.field(3).unwrap(), Value::String("bad".into())); +} + +#[test] +fn encode_skips_invalid_values_left_by_mutable_field_access() { + let p = pool(); + let idx = p.message_index("reflect.test.Scalars").unwrap(); + let md = p.message_by_name("reflect.test.Scalars").unwrap(); + let mut msg = DynamicMessage::new(Arc::clone(&p), idx); + + msg.set(md.field(3).unwrap(), Value::I32(7)); + *msg.field_by_number_mut(3).unwrap() = Value::String("bad".into()); + + let bytes = msg.encode_to_vec(); + assert!(bytes.is_empty()); + assert_eq!(msg.encoded_len(), bytes.len()); +} + #[test] fn dynamic_message_unknown_fields_preserved() { let p = pool(); From 81afac8048e5d53e5e3ecf252aacf7096502ed08 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Tue, 7 Jul 2026 21:01:05 -0700 Subject: [PATCH 2/2] Review fixups: document the encode-skip contract where consumers look The skip semantics lived only on field_by_number_mut; encode/encoded_len now state the whole-field omission (one invalid element suppresses its repeated/map field), the pool pointer-identity rule for nested messages, and the JSON serializer's differing null representation. field_mut mirrors the bypass warning. Comment the deliberate enum-by-variant-only validation asymmetry, fix a dangling merge.rs comment reference, and note in the changelog that set now panics where it previously produced corrupt wire output. No-Verification-Needed: doc/comment/changelog-only fixups --- .../unreleased/fixed-20260702-212901.yaml | 4 +-- buffa-descriptor/src/reflect/dynamic.rs | 32 +++++++++++++++++++ buffa-descriptor/src/reflect/message.rs | 3 +- 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/.changes/unreleased/fixed-20260702-212901.yaml b/.changes/unreleased/fixed-20260702-212901.yaml index 2976545..3dcb8ac 100644 --- a/.changes/unreleased/fixed-20260702-212901.yaml +++ b/.changes/unreleased/fixed-20260702-212901.yaml @@ -1,3 +1,3 @@ kind: Fixed -body: '`DynamicMessage::try_set` now rejects `Value`s whose runtime shape does not match the target field descriptor, including repeated elements, map keys and values, and nested message descriptors. Invalid values left through direct mutable field access are skipped during encode instead of producing tag-only corrupt wire output.' -time: 2026-07-02T19:29:06.000000000Z +body: '`DynamicMessage::try_set` now rejects `Value`s whose runtime shape does not match the target field descriptor — repeated elements, map keys and values, and nested message descriptor identity included — and `set` consequently panics on shapes it previously accepted and encoded as corrupt tag-only wire output. Invalid values planted through direct mutable field access are omitted during encode (whole field, with `encoded_len` in agreement) instead of corrupting the wire. (#272)' +time: 2026-07-02T21:29:01.000000000Z diff --git a/buffa-descriptor/src/reflect/dynamic.rs b/buffa-descriptor/src/reflect/dynamic.rs index 4833180..7858ed1 100644 --- a/buffa-descriptor/src/reflect/dynamic.rs +++ b/buffa-descriptor/src/reflect/dynamic.rs @@ -563,6 +563,20 @@ impl DynamicMessage { // ── Encode ────────────────────────────────────────────────────────────── /// Encode this message into `buf`. + /// + /// A stored `Value` whose shape does not match its field descriptor — + /// only reachable by bypassing [`try_set`]'s validation via + /// [`field_by_number_mut`](Self::field_by_number_mut) / + /// [`field_mut`](Self::field_mut) — is silently omitted, as the whole + /// field: one invalid element suppresses its entire repeated/map field. + /// A nested `Value::Message` counts as invalid unless it was built from + /// the *same* [`DescriptorPool`] instance (pointer identity, not + /// full-name equality). [`encoded_len`](Self::encoded_len) applies the + /// same rule, so length and bytes always agree. The JSON serializer + /// represents the same invalid values as `null` rather than omitting + /// the field. + /// + /// [`try_set`]: crate::reflect::ReflectMessageMut::try_set pub fn encode(&self, buf: &mut impl BufMut) { for (&number, value) in &self.fields { // Skip if neither the descriptor nor the extension index @@ -587,6 +601,10 @@ impl DynamicMessage { } /// Compute the encoded length. + /// + /// Applies the same invalid-value omission rule as + /// [`encode`](Self::encode), so the result always matches the bytes + /// `encode` writes. #[must_use] pub fn encoded_len(&self) -> usize { let mut len = self.unknown.encoded_len(); @@ -692,6 +710,11 @@ impl DynamicMessage { /// `field` may be a declared field or a registered extension of this /// message; both resolve by number. /// + /// Replacing the value with one whose shape doesn't match the field's + /// kind bypasses [`ReflectMessageMut::try_set`]'s validation; the + /// encoder defensively skips invalid stored values (see + /// [`encode`](Self::encode)). + /// /// # Panics /// /// Debug builds assert that `field` is a member of this message's @@ -1092,7 +1115,16 @@ fn validate_singular_shape( ) -> Result<(), ValueShapeMismatch> { let matches = match kind { SingularKind::Scalar(s) => value_matches_scalar(s, value), + // Enums are validated by variant only — no enum-descriptor identity + // or known-value check, deliberately asymmetric with the message arm + // below: enums are plain i32 on the wire (proto3 keeps unknown + // numbers), and JSON serialization looks the number up against the + // *field's* enum descriptor, so a foreign or unknown number cannot + // corrupt output. SingularKind::Enum(_) => matches!(value, Value::EnumNumber(_)), + // Pointer identity, not full-name equality: a structurally-identical + // message built from a different pool instance is foreign, matching + // `FieldNotMember`'s identity philosophy. SingularKind::Message(midx) => match value { Value::Message(msg) => Arc::ptr_eq(&msg.pool, pool) && msg.msg_idx == midx, _ => false, diff --git a/buffa-descriptor/src/reflect/message.rs b/buffa-descriptor/src/reflect/message.rs index df02fa6..1acc530 100644 --- a/buffa-descriptor/src/reflect/message.rs +++ b/buffa-descriptor/src/reflect/message.rs @@ -400,5 +400,6 @@ pub trait Reflectable { fn reflect(&self) -> ReflectCow<'_>; // `reflect_mut(&mut self) -> ReflectCowMut<'_>` is part of the design but - // deferred to the MergeSink work in this prototype — see merge.rs. + // deferred to the MergeSink work sketched in + // docs/investigations/reflection.md. }