Skip to content

Commit 4641a3c

Browse files
yanmo42iainmcgin
andauthored
codegen: floor descriptor_pool()'s scaled element-memory bound at the default (#360)
Fixes #336. ## The problem Generated `descriptor_pool()` scales its decode bound to `FILE_DESCRIPTOR_SET_BYTES.len() * 64`. For descriptor sets built mostly from short single-character type names, the real per-byte amplification runs above 64x — up to 352x for an empty `FileDescriptorProto` element, which is the tightest possible case for `descriptor.proto` on 64-bit targets. So the scaled bound can come out *smaller* than the flat 32 MiB `DEFAULT_ELEMENT_MEMORY_LIMIT` it replaced. The issue's own trigger shape (50 outer messages × 26 one-character nested messages) reproduces this: it's well under 1 KiB, needs ~71.5x to decode, and the 64x-scaled bound rejects it even though the pre-0.9.1 flat default accepted it fine. (Small correction while reproducing: the issue's own repro table reports 68-69x for this shape; the actual measured ratio is 71.5x. Traced to a `format!("{:0width$}", i, width=1)` name-padding bug in whatever generated that table, not a discrepancy in the shape itself — doesn't change the conclusion, just flagging it since the corrected number is what this PR's test locks in.) ## The fix ```rust FILE_DESCRIPTOR_SET_BYTES .len() .saturating_mul(64) .max(::buffa::DEFAULT_ELEMENT_MEMORY_LIMIT) ``` Floors the scaled bound at the old default. This makes the emitted bound provably monotonic relative to what it replaced — it can only loosen, never tighten — which is the specific property missing today. This intentionally does **not** try to resolve whether 64x is the right multiplier in general. It isn't a proven bound; 352x is, for `descriptor.proto` on 64-bit targets, today (32-bit wasn't measured and would need re-deriving — `size_of` scales with pointer width). Landing the floor now doesn't foreclose adopting 352, or an exact/trusted-input budget, later; all of those remain `.max()`-compatible with this floor or supersede it cleanly. Worth noting: @iainmcgin's own PR description for #332 — the commit that introduced this bound — already named the real fix and deliberately deferred it: > the durable fix is a bound expressed as a multiple of the encoded input > length, which would need no per-call-site exemption and would > self-tune... That is an API change to `DecodeOptions` touching the > untrusted-input posture, so it does not belong in a regression fix. This PR doesn't attempt that larger API change either — it's scoped the same way #332 was, to the narrow regression this issue reports. Picking a tighter proven multiplier (or the exact-budget/trusted-input alternatives) is a separate, larger decision for whoever owns that tradeoff. ## What else this touches `buffa-types/src/generated/google.protobuf.mod.rs` is regenerated via `task gen-wkt-types` so the bundled well-known-types pool picks up the same fix — confirmed the regen is idempotent (ran it twice, diffed the output directly, byte-identical). ## Testing New `buffa-descriptor/tests/issue_336_floor.rs`: - `old_formula_rejects_the_trigger_shape` — documents the bug on the issue's own trigger shape. - `floored_formula_accepts_the_trigger_shape` — proves the fix on that same shape, and that it's the floor (not the scaled term) doing the work. - `floor_is_monotonic_never_below_default` — sweeps several input lengths proving the floor never dips below the default, and checks the crossover point (where scaling starts to dominate the floor) lands at exactly 512 KiB, matching the issue's own math. Strengthened the existing `reflect_pool_module_emits_well_formed_tokens` codegen test to assert the emitted tokens reference `DEFAULT_ELEMENT_MEMORY_LIMIT` — it previously only checked that the module parses and mentions the bytes constant, so it wouldn't have caught a future edit silently dropping the floor. `cargo test --workspace`, `cargo fmt --all --check`, and `cargo clippy --workspace --all-targets -- -D warnings` all pass. --------- Co-authored-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
1 parent 2827cbf commit 4641a3c

4 files changed

Lines changed: 102 additions & 2 deletions

File tree

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
kind: Fixed
2+
body: '**Generated `descriptor_pool()` no longer rejects small schemas built from short type names** (#336, #360). The scaled element-memory bound (`len * 64`) could come out tighter than the untrusted-input default it replaced: descriptor sets whose bulk is one-character type names amplify past 64x (the issue''s shape measures ~71.5x on 64-bit), so a few-kilobyte schema was rejected even though the flat 32 MiB default before 0.9.1 accepted it. The bound is now floored at `DEFAULT_ELEMENT_MEMORY_LIMIT`, so it can only ever be looser than that default, never tighter.'
3+
time: 2026-07-28T10:39:43.538612562-04:00

‎buffa-codegen/src/reflect.rs‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,10 @@ pub(crate) fn reflect_pool_module(fds_bytes: &[u8]) -> TokenStream {
209209
/// finite, so corrupt embedded bytes still fail rather than
210210
/// exhausting memory.
211211
///
212+
/// The scaled bound is floored at
213+
/// [`DEFAULT_ELEMENT_MEMORY_LIMIT`](::buffa::DEFAULT_ELEMENT_MEMORY_LIMIT),
214+
/// so it is never tighter than the untrusted-input default.
215+
///
212216
/// # Panics
213217
///
214218
/// Panics on first access if the embedded bytes are malformed —
@@ -222,7 +226,10 @@ pub(crate) fn reflect_pool_module(fds_bytes: &[u8]) -> TokenStream {
222226
POOL.get_or_init(|| {
223227
let options = ::buffa::DecodeOptions::new()
224228
.with_element_memory_limit(
225-
FILE_DESCRIPTOR_SET_BYTES.len().saturating_mul(64),
229+
FILE_DESCRIPTOR_SET_BYTES
230+
.len()
231+
.saturating_mul(64)
232+
.max(::buffa::DEFAULT_ELEMENT_MEMORY_LIMIT),
226233
);
227234
::buffa::alloc::sync::Arc::new(
228235
::buffa_descriptor::DescriptorPool::decode_with_options(
@@ -488,6 +495,16 @@ mod tests {
488495
let parsed = syn::parse2::<syn::ItemMod>(tokens.clone());
489496
assert!(parsed.is_ok(), "generated module must parse: {tokens}");
490497
assert!(tokens.to_string().contains("FILE_DESCRIPTOR_SET_BYTES"));
498+
// #336: the emitted bound must floor at the untrusted-input default,
499+
// not just scale with length. Match the code form with spacing
500+
// stripped — the doc comment above it names the same constant, so a
501+
// bare substring check would pass on the doc text alone.
502+
let code = tokens.to_string().replace(' ', "");
503+
assert!(
504+
code.contains(".saturating_mul(64).max(::buffa::DEFAULT_ELEMENT_MEMORY_LIMIT)"),
505+
"descriptor_pool() must floor its scaled bound at \
506+
DEFAULT_ELEMENT_MEMORY_LIMIT (#336): {tokens}"
507+
);
491508
}
492509

493510
#[test]

‎buffa-descriptor/tests/pool_decode_limits.rs‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,3 +204,76 @@ fn nesting_deeper_than_the_name_cap_is_refused() {
204204
Err(PoolError::NameTooLong { .. })
205205
));
206206
}
207+
208+
/// The issue's trigger shape for #336: 50 messages each nesting 26 empty
209+
/// one-character messages. Every nested element costs ~5 wire bytes and
210+
/// materializes a full `DescriptorProto`, so the element-to-encoded ratio
211+
/// (~71.5x on 64-bit) exceeds the 64x multiplier generated
212+
/// `descriptor_pool()` scales its bound by.
213+
fn short_name_descriptor_set() -> Vec<u8> {
214+
const NAMES: &[u8] = b"abcdefghijklmnopqrstuvwxyz";
215+
let name = |i: usize| Some((NAMES[i % NAMES.len()] as char).to_string());
216+
let mut file = FileDescriptorProto {
217+
name: Some("a".to_string()),
218+
..Default::default()
219+
};
220+
for i in 0..50 {
221+
file.message_type.push(DescriptorProto {
222+
name: name(i),
223+
nested_type: (0..26)
224+
.map(|j| DescriptorProto {
225+
name: name(j),
226+
..Default::default()
227+
})
228+
.collect(),
229+
..Default::default()
230+
});
231+
}
232+
FileDescriptorSet {
233+
file: vec![file],
234+
..Default::default()
235+
}
236+
.encode_to_vec()
237+
}
238+
239+
fn decodes_under(bytes: &[u8], limit: usize) -> bool {
240+
DecodeOptions::new()
241+
.with_element_memory_limit(limit)
242+
.decode_from_slice::<FileDescriptorSet>(bytes)
243+
.is_ok()
244+
}
245+
246+
/// Why generated `descriptor_pool()` floors its scaled bound (#336): the
247+
/// element charge is `size_of::<DescriptorProto>()` per nested message, so a
248+
/// schema made of short names amplifies past 64x and the bare `len * 64`
249+
/// bound rejects what the flat default accepted.
250+
///
251+
/// The ratio scales with pointer width (descriptor types are mostly `Vec`
252+
/// and `Option<String>` fields), so on 32-bit targets the same shape lands
253+
/// under 64x and this no longer demonstrates the bug.
254+
#[cfg(target_pointer_width = "64")]
255+
#[test]
256+
fn scaled_bound_alone_rejects_short_name_schemas() {
257+
let bytes = short_name_descriptor_set();
258+
assert!(
259+
!decodes_under(&bytes, bytes.len().saturating_mul(64)),
260+
"expected the un-floored 64x bound to reject this shape; if it now \
261+
decodes, the shape is no longer a #336 trigger and needs replacing"
262+
);
263+
}
264+
265+
/// With the floor, the generated bound can never be tighter than the
266+
/// untrusted-input default, so anything the default accepts still decodes.
267+
#[test]
268+
fn floored_bound_accepts_what_the_default_accepts() {
269+
let bytes = short_name_descriptor_set();
270+
let floored = bytes
271+
.len()
272+
.saturating_mul(64)
273+
.max(buffa::DEFAULT_ELEMENT_MEMORY_LIMIT);
274+
assert!(decodes_under(&bytes, floored), "{} bytes", bytes.len());
275+
// The floor is what binds here; the scaled term only takes over past
276+
// 512 KiB of embedded descriptor bytes.
277+
assert_eq!(floored, buffa::DEFAULT_ELEMENT_MEMORY_LIMIT);
278+
assert_eq!(buffa::DEFAULT_ELEMENT_MEMORY_LIMIT / 64, 512 * 1024);
279+
}

‎buffa-types/src/generated/google.protobuf.mod.rs‎

Lines changed: 8 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)