Skip to content

UPSTREAM PR #26906: Reject overflowing descriptor allocation plans#140

Open
loci-dev wants to merge 1 commit into
mainfrom
loci/pr-26906-fix-descriptorpool-planning-overflow
Open

UPSTREAM PR #26906: Reject overflowing descriptor allocation plans#140
loci-dev wants to merge 1 commit into
mainfrom
loci/pr-26906-fix-descriptorpool-planning-overflow

Conversation

@loci-dev
Copy link
Copy Markdown

Note

Source pull request: protocolbuffers/protobuf#26906

This change makes DescriptorPool allocation planning fail cleanly when large descriptor counts would overflow flat-allocation byte accounting.

Changes:

  • add checked accounting in flat allocation planning
  • make flat allocation creation fail cleanly on overflow
  • return a descriptor error instead of proceeding after under-accounted planning
  • add a regression test for oversized top-level message_type counts

@loci-review
Copy link
Copy Markdown

loci-review Bot commented Apr 15, 2026

Overview

Analysis of 10,168 functions (37 modified, 7 new, 3 removed) shows modest compile-time performance regressions from security hardening that adds integer overflow protection to descriptor allocation planning. Changes affect protoc compilation phase only—runtime message parsing/serialization hot paths remain unaffected.

Binary: build.protoc-stable

  • Power consumption: 587,798 nJ → 587,863 nJ (+0.011%)

Function Analysis

Most Impacted Functions:

  • BuildMessage: Response time +72.2 ms (+5.0%), throughput -362 ns (-6.4%). Largest absolute regression due to cumulative overhead from nested descriptor building operations (BuildEnum, BuildEnumValue). Self-time improved despite overall regression.

  • BuildEnum: Response time +19.5 ms (+8.3%), throughput -51 ns (-2.1%). Adds overflow validation in allocation planning with new reserved range processing. Calls BuildEnumValue which contributes +4.9 ms per invocation.

  • PlanAllocationSize (DescriptorProto): Response time -25 μs (-0.2%), throughput +800 ns (+134%). CFG expanded from 42 to 103 blocks with comprehensive overflow checks using magic constants and per-iteration validation. Response time stable despite throughput increase.

  • CreateFlatAlloc: Response time +611 ns (+60.6%), throughput +611 ns (+84.9%). Now calls CalculateEnds() returning absl::optional for failure signaling, with SIMD-based initialization and bounds checking against 0x7fffffff limits.

  • AllocateArray: Response time +5.0 ms (+99.6%), throughput +55 ns (+38.2%). Adds explicit size_t casting and ABSL_CHECK_LE validation. Response time increase reflects error handling paths with logging/stack traces.

Source Code Changes:
Commit dfc07ab implements comprehensive overflow protection: changed array sizes from int to size_t, added TryAddToTotal() for checked arithmetic, modified FinalizePlanning() to return bool for error propagation, and introduced planning_failed_ flag. These prevent silent integer wraparound that could cause heap corruption from malformed .proto files.

Performance Improvements:
Several functions benefited from compiler optimizations: Write (TextFormat) -356 ns (-48.9%), BuildService -298 μs (-0.18%), NewPlaceholderFile -56 ns (-19.9%), StrReplaceAll -21 ns (-19.6%).

Justification:
All regressions occur in compile-time descriptor building (not runtime hot paths). The 72 ms increase in BuildMessage represents negligible overhead for one-time .proto compilation. Security benefits of preventing integer overflow vulnerabilities fully justify the modest performance cost.

💬 Questions? Tag @loci-dev

@loci-dev loci-dev force-pushed the main branch 27 times, most recently from fa3f834 to 96f6b2a Compare April 21, 2026 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants