⚡ Match Highlights: protocolbuffers/protobuf/pull/

jortles · 2026-05-08T03:53:10Z

Summary

Reflection::SwapFieldsImpl() validates that both messages belong to the correct Reflection object (GetReflection() == this), but does not verify that each field descriptor in the fields vector belongs to the message's own descriptor. This means a field from a different message type can be passed in, and its index() value can exceed the bounds of the target descriptor's offsets_ array.

When this happens, GetFieldOffset() at generated_message_reflection.h:127 reads offsets_[field->index()] out of bounds, producing an arbitrary offset value. The subsequent std::swap then writes 8 bytes at that offset relative to the message pointer, corrupting adjacent heap memory.

Root cause

Every other Reflection write method (SetInt32, SetString, HasField, ClearField, Swap, RemoveLast, MutableRawRepeatedField, etc.) includes USAGE_CHECK_MESSAGE_TYPE which validates field->containing_type() == descriptor_. The SwapFieldsImpl path was the only write method missing this check.

Fix

Add ABSL_CHECK_EQ(field->containing_type(), descriptor_) for each field at the top of the loop in SwapFieldsImpl, before any field operations. This matches the validation pattern used by all other Reflection methods.

Impact

Without this check, passing a foreign field descriptor causes:

Out-of-bounds read on offsets_[] array (reads heap data as a field offset)
Out-of-bounds write via std::swap at the corrupted offset (overwrites adjacent heap objects)

Test plan

Added SwapFieldsForeignFieldCheck death test: passes a ForeignMessage field to SwapFields on a TestAllTypes message, verifies the check fires with "does not belong to message type"
Follows the same pattern as the existing UsageErrors death test
All existing SwapFields* tests should pass unchanged (they only use fields from the correct descriptor)

jortles · 2026-05-18T15:05:45Z

Hi, is there an update for this?

Thank you,
Anthony Hurtado

karenwuz · 2026-06-09T16:25:04Z

@jortles looks like there are failing tests, can you fix?

jortles · 2026-09-08T15:52:51Z

Hi @karenwuz, sorry for the delay. The CI failure was the Apply Safe for Tests Label step (a fork-based PR permissions issue), not an actual test failure — it blocked all downstream test jobs from running.

I've rebased the branch on current main (clean, no conflicts) and force-pushed. The relevant tests pass locally:

SwapFieldsForeignFieldCheck (new death test) — PASSED
All 20 existing SwapFields* + UsageErrors tests — PASSED

Please let me know if there's anything else needed.

tonyliaoss · 2026-09-08T15:55:05Z

I'll apply the tag and retry.

tonyliaoss · 2026-09-08T15:56:53Z

Did you reply to my review comment above or update your commit? I don't see any diffs compared to before and I don't see a response to my review comment.

jortles · 2026-09-08T16:01:01Z

Sorry @tonyliaoss — I think your review comment may have been lost when the branch was force-pushed. I'm not seeing it in the conversation thread or the files tab. Could you re-post it? Happy to address whatever was needed.

tonyliaoss

Ugh, I didn't hit "submit review" :(

tonyliaoss · 2026-06-03T21:24:19Z

  absl::flat_hash_set<int> swapped_oneof;

  for (const auto* field : fields) {
+    ABSL_CHECK_EQ(field->containing_type(), descriptor_)


Why don't we use USAGE_CHECK_MESSAGE_TYPE here then? I think I slightly prefer more code reuse to a more specific error message.

jortles · 2026-09-08T17:11:04Z

No problem! Switched to USAGE_CHECK_MESSAGE_TYPE(SwapFields) and updated the death test to match. Pushed in 0e41240.

tonyliaoss

Thanks!

SwapFieldsImpl() validates that messages belong to this Reflection object, but does not check that each field descriptor in the fields vector actually belongs to the message's descriptor. When a field from a different message type is passed, its index can exceed the bounds of the target's offsets_ array, causing an out-of-bounds read that produces an arbitrary write offset for the subsequent std::swap. Add an ABSL_CHECK_EQ verifying field->containing_type() == descriptor_ for each field before processing, consistent with USAGE_CHECK_MESSAGE_TYPE used by all other Reflection write methods (SetInt32, SetString, etc.). Added regression test SwapFieldsForeignFieldCheck that verifies SwapFields rejects a field from a different message type.

…ory access (#30395) ## Summary `Reflection::HasOneof()`, `Reflection::ClearOneof()` and `Reflection::GetOneofFieldDescriptor()` compute the oneof-case address from the caller-supplied descriptor's index (via the private `GetOneofCase()`/`MutableOneofCase()` helpers and `ReflectionSchema::GetOneofCaseOffset()`, `generated_message_reflection.h:163-168`): ```cpp uint32_t GetOneofCaseOffset(const OneofDescriptor* oneof_descriptor) const { return static_cast<uint32_t>(oneof_case_offset_) + static_cast<uint32_t>(static_cast<size_t>(oneof_descriptor->index()) * sizeof(uint32_t)); } ``` None of the three methods validates that the descriptor belongs to the message they are called on, so a oneof descriptor from a different message type with a larger oneof index addresses memory outside the target object's oneof-case array: an out-of-bounds 4-byte read, and — in `ClearOneof()`, when the stale value resolves to a pointer-typed member of the target (string/message) — a `delete` of a value read from the message's oneof union, followed by a 4-byte write of 0 at the same offset. This is the oneof counterpart of the field-side check added to `SwapFieldsImpl` in #27289: every field-taking method validates `field->containing_type() == descriptor_` via `USAGE_CHECK_MESSAGE_TYPE`; there is no equivalent validation for oneof descriptors anywhere in the tree. (Private `GetOneofCase()`/`MutableOneofCase()` are additionally reached from field-based paths that are already checked.) ## Root cause - `HasOneof()` — `generated_message_reflection.cc:3383` (real-oneof branch) - `ClearOneof()` — `:3403`; the destroy path (`delete *MutableRaw<Message*>(...)`) at `:3439` - `GetOneofFieldDescriptor()` — `:3023` - offset computation — `ReflectionSchema::GetOneofCaseOffset()`, `generated_message_reflection.h:163-168` ## Fix Add a `USAGE_CHECK_ONEOF_MESSAGE_TYPE` guard mirroring `USAGE_CHECK_MESSAGE_TYPE`, reported through a oneof-specific reporter (`ReportReflectionUsageOneofError`; the existing reporter prints a `FieldDescriptor` name). The check runs at the top of all three public methods, fails fast in all build modes (it is not `NDEBUG`-gated, matching the field-side check), and is unreachable for the library's own internal callers, which always pass the message's own oneof descriptors. ## Test plan - New death test `GeneratedMessageReflectionTest.OneofForeignDescriptorCheck` — passes a `unittest::TestOneof2` oneof descriptor to `unittest::TestAllTypes` reflection for all three APIs and expects the usage error. - Full `//src/google/protobuf:generated_message_reflection_unittest` suite passes. - Verified on an ASan build of the runtime: before the change, the three calls show `heap-buffer-overflow` on the oneof-case read and `ClearOneof()` a `heap-use-after-free`; after the change each call terminates with the usage error instead. Closes #30395 COPYBARA_INTEGRATE_REVIEW=#30395 from kemrec:add-oneof-ownership-check aadb8ce PiperOrigin-RevId: 1015015987

JasonLunn added c++ 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed 🅰️ safe for tests Mark a commit as safe to run presubmits over labels May 13, 2026

github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 13, 2026

rgoldfinger6 added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 18, 2026

github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 18, 2026

rgoldfinger6 requested a review from sbenzaquen May 18, 2026 20:21

tonyliaoss self-assigned this Jun 3, 2026

tonyliaoss added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 3, 2026

github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 3, 2026

karenwuz assigned jortles Jun 9, 2026

tonyliaoss force-pushed the fix-swapfields-ownership-check branch from 4195606 to c9d448d Compare June 10, 2026 14:38

tonyliaoss added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 10, 2026

github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 10, 2026

jortles force-pushed the fix-swapfields-ownership-check branch 2 times, most recently from 5fc75e2 to c7640e2 Compare September 8, 2026 15:52

tonyliaoss reviewed Sep 8, 2026

View reviewed changes

jortles force-pushed the fix-swapfields-ownership-check branch from c7640e2 to 0e41240 Compare September 8, 2026 17:11

tonyliaoss added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 8, 2026

github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 8, 2026

tonyliaoss approved these changes Sep 8, 2026

View reviewed changes

tonyliaoss force-pushed the fix-swapfields-ownership-check branch from 0e41240 to 04bff40 Compare September 28, 2026 15:11

tonyliaoss added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 28, 2026

github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 28, 2026

copybara-service Bot closed this in 6ab198d Sep 30, 2026

kemrec mentioned this pull request Oct 1, 2026

Add oneof ownership check to Reflection oneof APIs to prevent OOB memory access #30395

Closed

kalt2212 mentioned this pull request Oct 6, 2026

Add field ownership check to SwapField and UnsafeShallowSwapField #30476

Open

karenwuz mentioned this pull request Oct 20, 2026

fix: [C++] validate field ownership in the reflection map APIs #30793

Closed

kemrec mentioned this pull request Nov 6, 2026

Add field ownership check to Get(Mutable)RepeatedFieldRef to prevent OOB memory access #31355

Open

Navigation Menu

Repository navigation

Add field ownership check to SwapFields to prevent OOB memory access - #27289

Add field ownership check to SwapFields to prevent OOB memory access#27289
jortles wants to merge 1 commit into
protocolbuffers:mainfrom
jortles:fix-swapfields-ownership-check

jortles commented May 8, 2026

Uh oh!

jortles commented May 18, 2026

Uh oh!

karenwuz commented Jun 9, 2026

Uh oh!

jortles commented Sep 8, 2026

Uh oh!

tonyliaoss commented Sep 8, 2026

Uh oh!

tonyliaoss commented Sep 8, 2026

Uh oh!

jortles commented Sep 8, 2026

Uh oh!

tonyliaoss left a comment

Uh oh!

tonyliaoss Jun 3, 2026

Uh oh!

jortles commented Sep 8, 2026

Uh oh!

tonyliaoss left a comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

jortles commented May 8, 2026

Summary

Root cause

Fix

Impact

Test plan

Uh oh!

jortles commented May 18, 2026

Uh oh!

karenwuz commented Jun 9, 2026

Uh oh!

jortles commented Sep 8, 2026

Uh oh!

tonyliaoss commented Sep 8, 2026

Uh oh!

tonyliaoss commented Sep 8, 2026

Uh oh!

jortles commented Sep 8, 2026

Uh oh!

tonyliaoss left a comment

Choose a reason for hiding this comment

Uh oh!

tonyliaoss Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

jortles commented Sep 8, 2026

Uh oh!

tonyliaoss left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants