descriptor: reject missing reserved range bounds

.changes/unreleased/fixed-20260903-132823.yaml

@@ -0,0 +1,4 @@
+kind: Fixed
+body: |-
+    **`DescriptorPool` validates reserved-range bounds as `protoc` does** (#418), instead of silently dropping a malformed range and reserving nothing. An unset bound reads as 0; a message reserved range must then satisfy `0 < start < end` (so a missing, zero, negative, empty, or reversed range is `PoolError::InvalidMessageReservedRange`), and an enum reserved range, which is inclusive and may be negative, must satisfy `start <= end` (`PoolError::InvalidEnumReservedRange`). Only hand-built or synthesized descriptor sets can reach either.
+time: 2026-09-03T13:28:23+02:00

buffa-descriptor/src/pool.rs

@@ -86,23 +86,52 @@ pub const MAX_SYMBOL_LEN: usize = 512;
 struct ReservedRanges(Vec<(i64, i64)>);
 
 impl ReservedRanges {
-    fn new(ranges: &[crate::generated::descriptor::descriptor_proto::ReservedRange]) -> Self {
-        Self::from_half_open(ranges.iter().filter_map(|r| match (r.start, r.end) {
-            // An unset bound cannot be honoured; protoc requires both.
-            (Some(start), Some(end)) => Some((i64::from(start), i64::from(end))),
-            _ => None,
-        }))
+    /// Index a message's reserved ranges, validating each as protoc does: an
+    /// unset bound reads as 0, and the half-open range must satisfy
+    /// `0 < start < end`.
+    fn for_message(
+        message_fqn: &str,
+        ranges: &[crate::generated::descriptor::descriptor_proto::ReservedRange],
+    ) -> Result<Self, PoolError> {
+        let mut checked = Vec::with_capacity(ranges.len());
+        for r in ranges {
+            let (start, end) = (r.start.unwrap_or(0), r.end.unwrap_or(0));
+            if start <= 0 || start >= end {
+                return Err(PoolError::InvalidMessageReservedRange {
+                    message: message_fqn.to_string(),
+                    start: r.start,
+                    end: r.end,
+                });
+            }
+            checked.push((i64::from(start), i64::from(end)));
+        }
+        Ok(Self::from_half_open(checked.into_iter()))
     }
 
+    /// Index an enum's reserved ranges, validating each as protoc does: an
+    /// unset bound reads as 0, the range is inclusive, may be negative, and
+    /// must satisfy `start <= end`.
     fn for_enum(
+        enum_fqn: &str,
         ranges: &[crate::generated::descriptor::enum_descriptor_proto::EnumReservedRange],
-    ) -> Self {
-        Self::from_half_open(ranges.iter().filter_map(|r| match (r.start, r.end) {
-            (Some(start), Some(end)) => Some((i64::from(start), i64::from(end) + 1)),
-            _ => None,
-        }))
+    ) -> Result<Self, PoolError> {
+        let mut checked = Vec::with_capacity(ranges.len());
+        for r in ranges {
+            let (start, end) = (r.start.unwrap_or(0), r.end.unwrap_or(0));
+            if start > end {
+                return Err(PoolError::InvalidEnumReservedRange {
+                    enum_name: enum_fqn.to_string(),
+                    start: r.start,
+                    end: r.end,
+                });
+            }
+            checked.push((i64::from(start), i64::from(end) + 1));
+        }
+        Ok(Self::from_half_open(checked.into_iter()))
     }
 
+    /// Sort and coalesce validated half-open ranges. Callers validate first;
+    /// the `start < end` filter only protects the coalescing invariant.
     fn from_half_open(ranges: impl Iterator<Item = (i64, i64)>) -> Self {
         let mut sorted: Vec<(i64, i64)> = ranges.filter(|&(start, end)| start < end).collect();
         sorted.sort_unstable();
@@ -276,6 +305,22 @@ pub enum PoolError {
         name: String,
         number: i32,
     },
+    /// A message reserved range does not satisfy `0 < start < end`. `end` is
+    /// exclusive, as in `DescriptorProto.ReservedRange`, and an unset bound
+    /// reads as 0, as protoc reads it. The bounds are carried as declared.
+    InvalidMessageReservedRange {
+        message: String,
+        start: Option<i32>,
+        end: Option<i32>,
+    },
+    /// An enum reserved range has `start > end`. Both bounds are inclusive,
+    /// as in `EnumDescriptorProto.EnumReservedRange`, may be negative, and an
+    /// unset bound reads as 0. The bounds are carried as declared.
+    InvalidEnumReservedRange {
+        enum_name: String,
+        start: Option<i32>,
+        end: Option<i32>,
+    },
 }
 
 /// Renders an optional range bound for [`PoolError`] messages: the number,
@@ -472,6 +517,26 @@ impl core::fmt::Display for PoolError {
                 f,
                 "enum {enum_name} value {name:?} reuses number {number} without allow_alias"
             ),
+            Self::InvalidMessageReservedRange {
+                message,
+                start,
+                end,
+            } => write!(
+                f,
+                "message {message} reserved range {}..{} is invalid; bounds must satisfy 0 < start < end",
+                Bound(*start),
+                Bound(*end),
+            ),
+            Self::InvalidEnumReservedRange {
+                enum_name,
+                start,
+                end,
+            } => write!(
+                f,
+                "enum {enum_name} reserved range {} to {} is invalid; start must not exceed end",
+                Bound(*start),
+                Bound(*end),
+            ),
         }
     }
 }
@@ -1502,7 +1567,7 @@ impl DescriptorPool {
                 });
             }
         }
-        let reserved_ranges = ReservedRanges::new(&msg.reserved_range);
+        let reserved_ranges = ReservedRanges::for_message(&fqn, &msg.reserved_range)?;
         let mut field_names: BTreeMap<String, usize> = BTreeMap::new();
         // Two fields resolving to one JSON name make JSON lookup ambiguous, but
         // protobuf permits it where JSON is best-effort: protoc emits such a
@@ -1705,7 +1770,7 @@ impl DescriptorPool {
                 });
             }
         }
-        let reserved_ranges = ReservedRanges::for_enum(&e.reserved_range);
+        let reserved_ranges = ReservedRanges::for_enum(&fqn, &e.reserved_range)?;
         let allow_alias = e
             .options
             .as_option()
@@ -2382,7 +2447,7 @@ mod reserved_ranges_tests {
                 ..Default::default()
             })
             .collect();
-        ReservedRanges::new(&raw)
+        ReservedRanges::for_message("t.M", &raw).expect("valid message reserved ranges")
     }
 
     #[test]
@@ -2399,16 +2464,57 @@ mod reserved_ranges_tests {
     }
 
     #[test]
-    fn unset_or_empty_ranges_are_ignored() {
-        let r = ranges(&[
+    fn message_ranges_follow_protoc_bounds_rules() {
+        use super::PoolError;
+        // protoc reads an unset bound as 0 and requires `0 < start < end`.
+        for (start, end) in [
             (Some(1), None),
             (None, Some(4)),
+            (None, None),
             (Some(7), Some(7)),
             (Some(8), Some(3)),
-        ]);
-        assert!(r.0.is_empty());
-        assert!(!r.contains(1));
-        assert!(!r.overlaps(0, 10));
+            (Some(0), Some(3)),
+            (Some(-2), Some(3)),
+        ] {
+            let raw = [ReservedRange {
+                start,
+                end,
+                ..Default::default()
+            }];
+            assert!(
+                matches!(
+                    ReservedRanges::for_message("t.M", &raw),
+                    Err(PoolError::InvalidMessageReservedRange { start: s, end: e, .. }) if (s, e) == (start, end)
+                ),
+                "{start:?}..{end:?} should be rejected"
+            );
+        }
+    }
+
+    #[test]
+    fn enum_ranges_read_unset_bounds_as_zero() {
+        use super::PoolError;
+        use crate::generated::descriptor::enum_descriptor_proto::EnumReservedRange;
+        let range = |start, end| EnumReservedRange {
+            start,
+            end,
+            ..Default::default()
+        };
+        // `reserved 0 to 8`, `reserved -3 to 0`, and `reserved 0` respectively.
+        let r = ReservedRanges::for_enum(
+            "t.E",
+            &[
+                range(None, Some(8)),
+                range(Some(-3), None),
+                range(None, None),
+            ],
+        )
+        .expect("unset enum bounds read as 0 and are valid");
+        assert!(r.contains(-3) && r.contains(0) && r.contains(8) && !r.contains(9));
+        assert!(matches!(
+            ReservedRanges::for_enum("t.E", &[range(Some(5), Some(4))]),
+            Err(PoolError::InvalidEnumReservedRange { .. })
+        ));
     }
 
     #[test]
@@ -2422,7 +2528,7 @@ mod reserved_ranges_tests {
                 ..Default::default()
             })
             .collect();
-        let r = ReservedRanges::for_enum(&raw);
+        let r = ReservedRanges::for_enum("t.E", &raw).expect("valid enum reserved ranges");
         assert_eq!(
             r.0,
             vec![

buffa-descriptor/tests/pool_e2e.rs

@@ -809,6 +809,57 @@ fn reserved_message_field_numbers_are_rejected_without_mutating_pool() {
     }
 }
 
+#[test]
+fn invalid_message_reserved_ranges_are_rejected_transactionally() {
+    use buffa_descriptor::generated::descriptor::descriptor_proto::ReservedRange;
+    use buffa_descriptor::generated::descriptor::DescriptorProto;
+
+    // protoc reads an unset bound as 0 and requires `0 < start < end`, so a
+    // missing, zero, negative, empty, or reversed range is one error.
+    for (suffix, start, end) in [
+        ("missing-start", None, Some(8)),
+        ("missing-end", Some(7), None),
+        ("missing-both", None, None),
+        ("zero-start", Some(0), Some(5)),
+        ("negative-start", Some(-3), Some(5)),
+        ("empty", Some(7), Some(7)),
+        ("reversed", Some(9), Some(5)),
+    ] {
+        let file_name = format!("reserved-message-range-{suffix}.proto");
+        let full_name = "invalid.test.BadMessageRange";
+        assert_rejected_without_mutating_pool(
+            &file_name,
+            full_name,
+            DescriptorProto {
+                name: Some("BadMessageRange".into()),
+                reserved_range: vec![ReservedRange {
+                    start,
+                    end,
+                    ..Default::default()
+                }],
+                ..Default::default()
+            },
+            move |err| {
+                assert!(
+                    matches!(
+                        err,
+                        PoolError::InvalidMessageReservedRange { message, start: s, end: e }
+                            if message == full_name && (*s, *e) == (start, end)
+                    ),
+                    "unexpected error for {suffix}: {err}"
+                );
+                if suffix == "missing-end" {
+                    assert_eq!(
+                        err.to_string(),
+                        "message invalid.test.BadMessageRange reserved range 7..unset is \
+                         invalid; bounds must satisfy 0 < start < end"
+                    );
+                }
+            },
+        );
+    }
+}
+
 #[test]
 fn reserved_message_field_range_end_is_exclusive() {
     use buffa_descriptor::generated::descriptor::descriptor_proto::ReservedRange;
@@ -1449,6 +1500,102 @@ fn reserved_enum_value_numbers_are_rejected_transactionally() {
     }
 }
 
+#[test]
+fn enum_reserved_ranges_follow_protoc_bounds_rules() {
+    use buffa_descriptor::generated::descriptor::enum_descriptor_proto::EnumReservedRange;
+    use buffa_descriptor::generated::descriptor::{
+        EnumDescriptorProto, EnumValueDescriptorProto, FileDescriptorProto, FileDescriptorSet,
+    };
+
+    let make_set = |file: &str, package: &str, start, end, values: &[i32]| FileDescriptorSet {
+        file: vec![FileDescriptorProto {
+            name: Some(file.into()),
+            package: Some(package.into()),
+            syntax: Some("proto2".into()),
+            enum_type: vec![EnumDescriptorProto {
+                name: Some("Ranged".into()),
+                value: values
+                    .iter()
+                    .map(|&n| EnumValueDescriptorProto {
+                        name: Some(format!("V{n}")),
+                        number: Some(n),
+                        ..Default::default()
+                    })
+                    .collect(),
+                reserved_range: vec![EnumReservedRange {
+                    start,
+                    end,
+                    ..Default::default()
+                }],
+                ..Default::default()
+            }],
+            ..Default::default()
+        }],
+        ..Default::default()
+    };
+
+    // Unset bounds read as 0 and enum ranges are inclusive and may be
+    // negative, so `..8` (0 to 8), `-3..` (-3 to 0) and `..` (0 to 0) are all
+    // valid — protoc and protobuf-go accept them — provided no value lands
+    // inside.
+    for (suffix, start, end) in [
+        ("unset-start", None, Some(8)),
+        ("unset-end", Some(-3), None),
+        ("unset-both", None, None),
+        ("negative", Some(-5), Some(-3)),
+    ] {
+        let file = format!("enum-range-{suffix}.proto");
+        let pool = DescriptorPool::new(make_set(&file, "valid.test", start, end, &[10, 11]))
+            .unwrap_or_else(|e| panic!("{suffix} should be accepted: {e}"));
+        assert!(pool
+            .enum_by_name("valid.test.Ranged")
+            .unwrap()
+            .value(10)
+            .is_some());
+    }
+
+    // ...and the unset-start range really is honoured as `0 to 8`.
+    let err = DescriptorPool::new(make_set(
+        "enum-range-hit.proto",
+        "invalid.test",
+        None,
+        Some(8),
+        &[5],
+    ))
+    .unwrap_err();
+    assert!(
+        matches!(&err, PoolError::ReservedEnumValueNumber { number: 5, .. }),
+        "unexpected error: {err}"
+    );
+
+    // Only `start > end` is an error.
+    assert_set_rejected_without_mutating_pool(
+        "enum-range-reversed.proto",
+        "invalid.test.Ranged",
+        make_set(
+            "enum-range-reversed.proto",
+            "invalid.test",
+            Some(9),
+            Some(7),
+            &[0],
+        ),
+        |err| {
+            assert!(
+                matches!(
+                    err,
+                    PoolError::InvalidEnumReservedRange { enum_name, start: Some(9), end: Some(7) }
+                        if enum_name == "invalid.test.Ranged"
+                ),
+                "unexpected error: {err}"
+            );
+            assert_eq!(
+                err.to_string(),
+                "enum invalid.test.Ranged reserved range 9 to 7 is invalid; start must not exceed end"
+            );
+        },
+    );
+}
+
 #[test]
 fn reserved_enum_value_names_are_rejected_transactionally() {
     use buffa_descriptor::generated::descriptor::{