diff --git a/.changes/unreleased/breaking-changes-20261007-232231.yaml b/.changes/unreleased/breaking-changes-20261007-232231.yaml new file mode 100644 index 00000000..d9fded6f --- /dev/null +++ b/.changes/unreleased/breaking-changes-20261007-232231.yaml @@ -0,0 +1,5 @@ +kind: Breaking changes +body: |- + **`Extension::new` and `Extension::with_default` panic on a field number that is 0 or greater than `i32::MAX`** (#674). Both are `const fn`, so in a `const` item the panic is a compile error. Before, they accepted any `u32`, and the documentation said that 0 was accepted and inert ("a descriptor constructed with `0` will never match valid wire data"). A hand-written `const` extension numbered 0 therefore compiled and now fails to compile; so does one numbered above `i32::MAX`. `set_extension` with an extension numbered 0 stored a field that encodes with a tag decoders reject. + The upper bound is `i32::MAX`, not `MAX_FIELD_NUMBER`, so that a hand-written constant for an extension of a message with `option message_set_wire_format = true`, whose number can pass 2^29 - 1, still compiles. Codegen rejects an extension number above 2^29 - 1 before it emits the constant, so generated code is unaffected. A number in the implementation-reserved band, 19000 to 19999, is accepted: that is a schema rule, which `DescriptorPool` applies when it loads a descriptor set. +time: 2026-10-07T23:22:31.588977+02:00 diff --git a/buffa/src/extension.rs b/buffa/src/extension.rs index 93cfaad1..91d398f1 100644 --- a/buffa/src/extension.rs +++ b/buffa/src/extension.rs @@ -33,9 +33,13 @@ //! //! # Panics //! -//! [`extension`], [`set_extension`], and [`clear_extension`] panic if the -//! descriptor's [`extendee`](Extension::extendee) does not match the extendee -//! message's [`PROTO_FQN`](ExtensionSet::PROTO_FQN). This catches bugs like +//! [`Extension::new`] and [`Extension::with_default`] panic if the extension +//! field number is 0 or greater than `i32::MAX`. +//! +//! [`extension`], [`extension_or_default`], [`set_extension`], +//! [`try_set_extension`], and [`clear_extension`] panic if the descriptor's +//! [`extendee`](Extension::extendee) does not match the extendee message's +//! [`PROTO_FQN`](ExtensionSet::PROTO_FQN). This catches bugs like //! `field_options.extension(&MESSAGE_LEVEL_OPTION)` at the first call site //! (matches protobuf-go, which panics, and protobuf-es, which throws). //! [`has_extension`] returns `false` gracefully on mismatch — "is this @@ -66,6 +70,7 @@ //! //! [`extension`]: ExtensionSet::extension //! [`set_extension`]: ExtensionSet::set_extension +//! [`try_set_extension`]: ExtensionSet::try_set_extension //! [`clear_extension`]: ExtensionSet::clear_extension //! [`has_extension`]: ExtensionSet::has_extension //! [`extension_or_default`]: ExtensionSet::extension_or_default @@ -91,8 +96,8 @@ pub struct Extension { /// Fully-qualified proto name of the extendee (e.g. /// `"google.protobuf.FieldOptions"`), no leading dot. /// - /// Checked against [`ExtensionSet::PROTO_FQN`] on `extension()`, - /// `set_extension()`, and `clear_extension()` — a mismatch panics. + /// Checked against [`ExtensionSet::PROTO_FQN`] by the accessors that the + /// module-level `# Panics` section lists — a mismatch panics. /// `has_extension()` stays graceful (returns `false`), matching /// protobuf-go and protobuf-es precedent. extendee: &'static str, @@ -108,12 +113,44 @@ impl Extension { /// /// `extendee` is the fully-qualified proto type name (no leading dot) of /// the message this extension extends — e.g. `"google.protobuf.FieldOptions"`. - /// Passing an extension with a mismatched extendee to `extension()` / - /// `set_extension()` / `clear_extension()` will panic. + /// Passing an extension with a mismatched extendee to an accessor that + /// reads, writes or clears it panics; the + /// [module documentation](crate::extension#panics) lists those accessors. + /// + /// # Panics + /// + /// Panics if `number` is 0 or greater than `i32::MAX`. In a `const` item + /// the panic is a compile error. + /// + /// The upper bound is `i32::MAX` because an extension of a message with + /// `option message_set_wire_format = true` can have a number up to that. + /// For any other extendee the largest valid number is + /// [`MAX_FIELD_NUMBER`](crate::encoding::MAX_FIELD_NUMBER): with a larger + /// one, `set_extension()` stores a field that encodes with a tag decoders + /// reject. + /// + /// # Examples /// - /// Field number `0` is invalid in protobuf. Codegen never emits it; - /// a descriptor constructed with `0` will never match valid wire data. + /// ``` + /// use buffa::extension::codecs::Int32; + /// use buffa::Extension; + /// + /// const WEIGHT: Extension = Extension::new(50001, "my.pkg.Options"); + /// assert_eq!(WEIGHT.number(), 50001); + /// ``` + /// + /// A `const` with the number 0 does not compile: + /// + /// ```compile_fail,E0080 + /// use buffa::extension::codecs::Int32; + /// use buffa::Extension; + /// + /// const ZERO: Extension = Extension::new(0, "my.pkg.Options"); + /// assert_eq!(ZERO.number(), 0); + /// ``` + #[track_caller] pub const fn new(number: u32, extendee: &'static str) -> Self { + assert_valid_extension_field_number(number); Self { number, extendee, @@ -132,11 +169,18 @@ impl Extension { /// call. For `Copy` scalars codegen emits a `const fn`; for `String` and /// `bytes` a regular `fn` (allocates on each call — same cost as a /// hand-written `.unwrap_or_else(|| "x".into())`). + /// + /// # Panics + /// + /// Panics if `number` is 0 or greater than `i32::MAX`, as + /// [`new`](Self::new) does. + #[track_caller] pub const fn with_default( number: u32, extendee: &'static str, default: fn() -> C::Value, ) -> Self { + assert_valid_extension_field_number(number); Self { number, extendee, @@ -156,10 +200,27 @@ impl Extension { } } +/// Asserts the range that [`Extension::new`] documents, where the reason for +/// the upper bound is given. +/// +/// The implementation-reserved band (19000 to 19999) is not checked. It is a +/// schema rule, which `DescriptorPool` applies when it loads a descriptor +/// set; a number in the band encodes and decodes like any other. +#[track_caller] +const fn assert_valid_extension_field_number(number: u32) { + assert!(number != 0, "extension field number must not be 0"); + assert!( + number <= crate::message_set::MAX_TYPE_ID, + "extension field number must be at most i32::MAX" + ); +} + /// Asserts that `ext` actually extends `Self`. Called from `extension()`, -/// `set_extension()`, and `clear_extension()` (but NOT `has_extension()` — -/// asking "is this extension set here" has a legitimate answer of `false` -/// when it can't extend here, matching protobuf-go and protobuf-es). +/// `try_set_extension()`, and `clear_extension()`; the other accessors in the +/// module-level `# Panics` list reach it through those. `has_extension()` +/// does NOT call it — asking "is this extension set here" has a legitimate +/// answer of `false` when it can't extend here, matching protobuf-go and +/// protobuf-es. /// /// `#[track_caller]` so the panic points at the user's call site, not here. #[track_caller] @@ -244,9 +305,11 @@ pub trait ExtensionSet { /// Fully-qualified proto type name of this message (no leading dot), /// e.g. `"google.protobuf.FieldOptions"`. /// - /// Checked against [`Extension::extendee`] on every `extension()`, - /// `set_extension()`, and `clear_extension()` call. A mismatch panics: - /// passing an extension for the wrong message is a bug in the caller. + /// Checked against [`Extension::extendee`] on every call of an accessor + /// that reads, writes or clears an extension; the + /// [module documentation](crate::extension#panics) lists them. A mismatch + /// panics: passing an extension for the wrong message is a bug in the + /// caller. /// /// Equal to [`MessageName::FULL_NAME`](crate::MessageName::FULL_NAME) /// for generated messages — both come from the same `proto_fqn` in @@ -1196,6 +1259,70 @@ mod tests { assert_eq!(copy.number(), 1); } + #[test] + fn extension_constructors_validate_field_numbers() { + let valid_numbers = [ + 1, + crate::encoding::FIRST_RESERVED_FIELD_NUMBER, + crate::encoding::LAST_RESERVED_FIELD_NUMBER, + crate::encoding::MAX_FIELD_NUMBER, + crate::encoding::MAX_FIELD_NUMBER + 1, + i32::MAX as u32, + ]; + for number in valid_numbers { + assert_eq!(Extension::::new(number, CARRIER).number(), number); + assert_eq!( + Extension::::with_default(number, CARRIER, || 0).number(), + number + ); + } + + let invalid_numbers = [0, i32::MAX as u32 + 1, u32::MAX]; + for number in invalid_numbers { + assert!( + std::panic::catch_unwind(|| Extension::::new(number, CARRIER)).is_err(), + "new accepted field number {number}" + ); + assert!( + std::panic::catch_unwind(|| { + Extension::::with_default(number, CARRIER, || 0) + }) + .is_err(), + "with_default accepted field number {number}" + ); + } + } + + #[test] + #[should_panic(expected = "extension field number must not be 0")] + fn extension_number_zero_panic_names_the_rule() { + let _ = Extension::::new(0, CARRIER); + } + + #[test] + #[should_panic(expected = "extension field number must be at most i32::MAX")] + fn extension_number_above_i32_max_panic_names_the_rule() { + let _ = Extension::::with_default(i32::MAX as u32 + 1, CARRIER, || 0); + } + + /// A MessageSet item is stored as a length-delimited unknown field whose + /// number is the item's type ID, and a type ID can pass + /// `MAX_FIELD_NUMBER`. A `const` extension with such a number compiles + /// and reads the item. + #[test] + fn extension_number_above_max_field_number_reads_message_set_item() { + const TYPE_ID: u32 = i32::MAX as u32; + const E: Extension = Extension::new(TYPE_ID, CARRIER); + const fn empty() -> Vec { + Vec::new() + } + const D: Extension = Extension::with_default(TYPE_ID, CARRIER, empty); + let mut c = Carrier::default(); + c.unknown.push(ld(TYPE_ID, vec![1, 2])); + assert_eq!(c.extension(&E), Some(vec![1, 2])); + assert_eq!(c.extension_or_default(&D), vec![1, 2]); + } + // ── Extendee identity check ───────────────────────────────────────────── // An extension declared for a different message. Using it on `Carrier` diff --git a/buffa/src/message_set.rs b/buffa/src/message_set.rs index 537498be..f3b038ac 100644 --- a/buffa/src/message_set.rs +++ b/buffa/src/message_set.rs @@ -30,6 +30,13 @@ pub const TYPE_ID_TAG: u64 = 2 << 3; /// Field 3, wire type `LengthDelimited` (2): `(3 << 3) | 2`. pub const MESSAGE_TAG: u64 = (3 << 3) | 2; +/// The largest MessageSet type ID, `i32::MAX`. +/// +/// `type_id` is a `required int32`, so an item's type ID, and the number of +/// an extension of a MessageSet, can pass +/// [`MAX_FIELD_NUMBER`](crate::encoding::MAX_FIELD_NUMBER). +pub(crate) const MAX_TYPE_ID: u32 = i32::MAX as u32; + /// Parse one MessageSet `Item` group body (between `SGROUP` and `EGROUP`). /// /// The caller must have already consumed the `ITEM_START_TAG`; this function @@ -77,7 +84,7 @@ pub fn merge_item( // type_id is `required int32`; valid protobuf field numbers // are [1, 2^29-1], but MessageSet historically allows the // full positive int32 range. - if v < 1 || v > i32::MAX as u64 { + if v < 1 || v > u64::from(MAX_TYPE_ID) { return Err(DecodeError::InvalidMessageSet("type_id out of range")); } type_id = Some(v as u32); diff --git a/docs/guide.md b/docs/guide.md index 1c49d485..7fcaf962 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -2216,9 +2216,11 @@ unchanged. (Scalar-typed extensions cannot fail.) ### Extendee identity check -`extension()`, `set_extension()`, and `clear_extension()` **panic** if you -pass an extension declared for a different message — for example, passing a -message-level option to a field-level options struct: +The accessors that read, write or clear an extension (`extension()`, +`extension_or_default()`, `set_extension()`, `try_set_extension()` and +`clear_extension()`; the `buffa::extension` module docs keep the list under +"Panics") **panic** if you pass an extension declared for a different message +— for example, passing a message-level option to a field-level options struct: ```rust,ignore // (buf.validate.message) extends MessageOptions, not FieldOptions — this @@ -2231,6 +2233,11 @@ This matches protobuf-go (which panics) and protobuf-es (which throws). "is this extension set here" has a legitimate answer (`false`) even when the extension can't extend here. +If you declare an extension by hand, `Extension::new` and +`Extension::with_default` panic when the field number is 0 or greater than +`i32::MAX`. Both are `const fn`, so in a `const` item the panic is a compile +error. Generated extension constants always have a valid number. + ### Proto2 `[default = ...]` Proto2 extension declarations can carry a default value: