From 8132a6956778c13c4d9ca6d5e0fd64ff95bedd35 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Wed, 7 Oct 2026 23:27:53 +0200 Subject: [PATCH 1/4] extension: reject invalid field numbers --- .../unreleased/fixed-20261007-232231.yaml | 3 + buffa/src/extension.rs | 80 +++++++++++++++++-- 2 files changed, 77 insertions(+), 6 deletions(-) create mode 100644 .changes/unreleased/fixed-20261007-232231.yaml diff --git a/.changes/unreleased/fixed-20261007-232231.yaml b/.changes/unreleased/fixed-20261007-232231.yaml new file mode 100644 index 00000000..c277b01f --- /dev/null +++ b/.changes/unreleased/fixed-20261007-232231.yaml @@ -0,0 +1,3 @@ +kind: Fixed +body: '**Reject invalid extension field numbers at construction.**' +time: 2026-10-07T23:22:31.588977+02:00 diff --git a/buffa/src/extension.rs b/buffa/src/extension.rs index 93cfaad1..af8b8feb 100644 --- a/buffa/src/extension.rs +++ b/buffa/src/extension.rs @@ -33,9 +33,11 @@ //! //! # 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 invalid. [`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 +68,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 @@ -109,11 +112,18 @@ 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. + /// `extension_or_default()` / `set_extension()` / `try_set_extension()` / + /// `clear_extension()` will panic. /// - /// Field number `0` is invalid in protobuf. Codegen never emits it; - /// a descriptor constructed with `0` will never match valid wire data. + /// # Panics + /// + /// Panics if `number` is less than 1, greater than + /// [`MAX_FIELD_NUMBER`](crate::encoding::MAX_FIELD_NUMBER), or in the + /// implementation-reserved range `19000..=19999` (from + /// [`FIRST_RESERVED_FIELD_NUMBER`](crate::encoding::FIRST_RESERVED_FIELD_NUMBER) + /// through [`LAST_RESERVED_FIELD_NUMBER`](crate::encoding::LAST_RESERVED_FIELD_NUMBER)). pub const fn new(number: u32, extendee: &'static str) -> Self { + assert_valid_extension_field_number(number); Self { number, extendee, @@ -132,11 +142,20 @@ 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 less than 1, greater than + /// [`MAX_FIELD_NUMBER`](crate::encoding::MAX_FIELD_NUMBER), or in the + /// implementation-reserved range `19000..=19999` (from + /// [`FIRST_RESERVED_FIELD_NUMBER`](crate::encoding::FIRST_RESERVED_FIELD_NUMBER) + /// through [`LAST_RESERVED_FIELD_NUMBER`](crate::encoding::LAST_RESERVED_FIELD_NUMBER)). 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,6 +175,18 @@ impl Extension { } } +const fn assert_valid_extension_field_number(number: u32) { + assert!( + number >= 1 && number <= crate::encoding::MAX_FIELD_NUMBER, + "extension field number must be in [1, MAX_FIELD_NUMBER]" + ); + assert!( + number < crate::encoding::FIRST_RESERVED_FIELD_NUMBER + || number > crate::encoding::LAST_RESERVED_FIELD_NUMBER, + "extension field number is in the implementation-reserved range" + ); +} + /// 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` @@ -1196,6 +1227,43 @@ mod tests { assert_eq!(copy.number(), 1); } + #[test] + fn extension_constructors_validate_field_numbers() { + let valid_numbers = [ + 1, + crate::encoding::FIRST_RESERVED_FIELD_NUMBER - 1, + crate::encoding::LAST_RESERVED_FIELD_NUMBER + 1, + crate::encoding::MAX_FIELD_NUMBER, + ]; + 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, + crate::encoding::FIRST_RESERVED_FIELD_NUMBER, + crate::encoding::LAST_RESERVED_FIELD_NUMBER, + crate::encoding::MAX_FIELD_NUMBER + 1, + ]; + 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}" + ); + } + } + // ── Extendee identity check ───────────────────────────────────────────── // An extension declared for a different message. Using it on `Carrier` From e9a82e2fbbfadc92ce4913b0cd29315153a9c10d Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Mon, 12 Oct 2026 12:40:18 -0700 Subject: [PATCH 2/4] extension: accept numbers up to i32::MAX and the reserved band An extension of a MessageSet can be numbered above 2^29 - 1, and Extension does not know its extendee's wire format, so the constructors assert only that the number is nonzero and at most i32::MAX. The reserved band 19000 to 19999 is a schema rule that DescriptorPool applies; a number in it encodes and decodes like any other. The changelog entry moves to Changed: the constructors gain a panic. --- .../unreleased/changed-20261007-232231.yaml | 4 ++ .../unreleased/fixed-20261007-232231.yaml | 3 - buffa/src/extension.rs | 70 ++++++++++++------- 3 files changed, 48 insertions(+), 29 deletions(-) create mode 100644 .changes/unreleased/changed-20261007-232231.yaml delete mode 100644 .changes/unreleased/fixed-20261007-232231.yaml diff --git a/.changes/unreleased/changed-20261007-232231.yaml b/.changes/unreleased/changed-20261007-232231.yaml new file mode 100644 index 00000000..39d5e262 --- /dev/null +++ b/.changes/unreleased/changed-20261007-232231.yaml @@ -0,0 +1,4 @@ +kind: Changed +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 `set_extension` with an extension numbered 0 stored a field that encodes with a tag decoders reject. The bound is `i32::MAX`, not `MAX_FIELD_NUMBER`, because an extension of a message with `option message_set_wire_format = true` can have a number above 2^29 - 1. 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. Generated code is unaffected, because codegen rejects an extension number outside 1 to 2^29 - 1 before it emits the constant. +time: 2026-10-07T23:22:31.588977+02:00 diff --git a/.changes/unreleased/fixed-20261007-232231.yaml b/.changes/unreleased/fixed-20261007-232231.yaml deleted file mode 100644 index c277b01f..00000000 --- a/.changes/unreleased/fixed-20261007-232231.yaml +++ /dev/null @@ -1,3 +0,0 @@ -kind: Fixed -body: '**Reject invalid extension field numbers at construction.**' -time: 2026-10-07T23:22:31.588977+02:00 diff --git a/buffa/src/extension.rs b/buffa/src/extension.rs index af8b8feb..f385836a 100644 --- a/buffa/src/extension.rs +++ b/buffa/src/extension.rs @@ -34,7 +34,7 @@ //! # Panics //! //! [`Extension::new`] and [`Extension::with_default`] panic if the extension -//! field number is invalid. [`extension`], [`extension_or_default`], +//! 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 @@ -117,11 +117,15 @@ impl Extension { /// /// # Panics /// - /// Panics if `number` is less than 1, greater than - /// [`MAX_FIELD_NUMBER`](crate::encoding::MAX_FIELD_NUMBER), or in the - /// implementation-reserved range `19000..=19999` (from - /// [`FIRST_RESERVED_FIELD_NUMBER`](crate::encoding::FIRST_RESERVED_FIELD_NUMBER) - /// through [`LAST_RESERVED_FIELD_NUMBER`](crate::encoding::LAST_RESERVED_FIELD_NUMBER)). + /// 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. pub const fn new(number: u32, extendee: &'static str) -> Self { assert_valid_extension_field_number(number); Self { @@ -145,11 +149,8 @@ impl Extension { /// /// # Panics /// - /// Panics if `number` is less than 1, greater than - /// [`MAX_FIELD_NUMBER`](crate::encoding::MAX_FIELD_NUMBER), or in the - /// implementation-reserved range `19000..=19999` (from - /// [`FIRST_RESERVED_FIELD_NUMBER`](crate::encoding::FIRST_RESERVED_FIELD_NUMBER) - /// through [`LAST_RESERVED_FIELD_NUMBER`](crate::encoding::LAST_RESERVED_FIELD_NUMBER)). + /// Panics if `number` is 0 or greater than `i32::MAX`, as + /// [`new`](Self::new) does. pub const fn with_default( number: u32, extendee: &'static str, @@ -175,15 +176,17 @@ impl Extension { } } +/// Asserts that `number` is nonzero and at most `i32::MAX`. +/// +/// `Extension` does not know whether its extendee uses the MessageSet wire +/// format, where a number can pass `MAX_FIELD_NUMBER`, so the bound is the +/// MessageSet one. The implementation-reserved band (19000 to 19999) is a +/// schema rule, which `DescriptorPool` applies when it loads a descriptor +/// set; a number in the band encodes and decodes like any other. const fn assert_valid_extension_field_number(number: u32) { assert!( - number >= 1 && number <= crate::encoding::MAX_FIELD_NUMBER, - "extension field number must be in [1, MAX_FIELD_NUMBER]" - ); - assert!( - number < crate::encoding::FIRST_RESERVED_FIELD_NUMBER - || number > crate::encoding::LAST_RESERVED_FIELD_NUMBER, - "extension field number is in the implementation-reserved range" + number >= 1 && number <= i32::MAX as u32, + "extension field number must be in [1, i32::MAX]" ); } @@ -1231,9 +1234,11 @@ mod tests { fn extension_constructors_validate_field_numbers() { let valid_numbers = [ 1, - crate::encoding::FIRST_RESERVED_FIELD_NUMBER - 1, - crate::encoding::LAST_RESERVED_FIELD_NUMBER + 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); @@ -1243,12 +1248,7 @@ mod tests { ); } - let invalid_numbers = [ - 0, - crate::encoding::FIRST_RESERVED_FIELD_NUMBER, - crate::encoding::LAST_RESERVED_FIELD_NUMBER, - crate::encoding::MAX_FIELD_NUMBER + 1, - ]; + 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(), @@ -1264,6 +1264,24 @@ mod tests { } } + /// 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` From d9ea78389650c8257caf86e44195ffca2f8ae1f9 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Mon, 12 Oct 2026 12:56:02 -0700 Subject: [PATCH 3/4] extension: mark the number check as a breaking change and name the rule Field number 0 was documented as accepted and inert, so a hand-written const with 0 compiled; the changelog entry is now a breaking change and says so. The constructors and the check carry track_caller, the check has one assertion per rule, and a compile_fail doctest shows the const case. The MessageSet type-ID ceiling gets one name that the item decoder and the constructors share. The module Panics section is the one list of accessors that check the extendee; the other docs and the guide refer to it. --- .../breaking-changes-20261007-232231.yaml | 5 ++ .../unreleased/changed-20261007-232231.yaml | 4 - buffa/src/extension.rs | 83 ++++++++++++++----- buffa/src/message_set.rs | 9 +- docs/guide.md | 13 ++- 5 files changed, 85 insertions(+), 29 deletions(-) create mode 100644 .changes/unreleased/breaking-changes-20261007-232231.yaml delete mode 100644 .changes/unreleased/changed-20261007-232231.yaml diff --git a/.changes/unreleased/breaking-changes-20261007-232231.yaml b/.changes/unreleased/breaking-changes-20261007-232231.yaml new file mode 100644 index 00000000..bf17fae0 --- /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`, only for hand-written extension constants of a message with `option message_set_wire_format = true`, whose numbers can pass 2^29 - 1. 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/.changes/unreleased/changed-20261007-232231.yaml b/.changes/unreleased/changed-20261007-232231.yaml deleted file mode 100644 index 39d5e262..00000000 --- a/.changes/unreleased/changed-20261007-232231.yaml +++ /dev/null @@ -1,4 +0,0 @@ -kind: Changed -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 `set_extension` with an extension numbered 0 stored a field that encodes with a tag decoders reject. The bound is `i32::MAX`, not `MAX_FIELD_NUMBER`, because an extension of a message with `option message_set_wire_format = true` can have a number above 2^29 - 1. 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. Generated code is unaffected, because codegen rejects an extension number outside 1 to 2^29 - 1 before it emits the constant. -time: 2026-10-07T23:22:31.588977+02:00 diff --git a/buffa/src/extension.rs b/buffa/src/extension.rs index f385836a..91d398f1 100644 --- a/buffa/src/extension.rs +++ b/buffa/src/extension.rs @@ -34,10 +34,12 @@ //! # Panics //! //! [`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 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 @@ -94,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, @@ -111,9 +113,9 @@ 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()` / - /// `extension_or_default()` / `set_extension()` / `try_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 /// @@ -126,6 +128,27 @@ impl Extension { /// [`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 + /// + /// ``` + /// 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 { @@ -151,6 +174,7 @@ impl Extension { /// /// 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, @@ -176,24 +200,27 @@ impl Extension { } } -/// Asserts that `number` is nonzero and at most `i32::MAX`. +/// Asserts the range that [`Extension::new`] documents, where the reason for +/// the upper bound is given. /// -/// `Extension` does not know whether its extendee uses the MessageSet wire -/// format, where a number can pass `MAX_FIELD_NUMBER`, so the bound is the -/// MessageSet one. The implementation-reserved band (19000 to 19999) is a +/// 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 >= 1 && number <= i32::MAX as u32, - "extension field number must be in [1, i32::MAX]" + 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] @@ -278,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 @@ -1264,6 +1293,18 @@ mod tests { } } + #[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 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: From d7e243b5d7a481cc38f35f89f6f4bc85eb8e4af7 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Mon, 12 Oct 2026 13:11:23 -0700 Subject: [PATCH 4/4] changelog: say why the extension number bound is i32::MAX The bound applies to every call; MessageSet extensions are the reason for it, not its scope. --- .changes/unreleased/breaking-changes-20261007-232231.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changes/unreleased/breaking-changes-20261007-232231.yaml b/.changes/unreleased/breaking-changes-20261007-232231.yaml index bf17fae0..d9fded6f 100644 --- a/.changes/unreleased/breaking-changes-20261007-232231.yaml +++ b/.changes/unreleased/breaking-changes-20261007-232231.yaml @@ -1,5 +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`, only for hand-written extension constants of a message with `option message_set_wire_format = true`, whose numbers can pass 2^29 - 1. 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. + 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