Skip to content

Python: fix heap-use-after-free in MapIterator after map.clear()#27257

Closed
vhullto wants to merge 8 commits into
protocolbuffers:mainfrom
vhullto:fix/python-map-clear-uaf
Closed

Python: fix heap-use-after-free in MapIterator after map.clear()#27257
vhullto wants to merge 8 commits into
protocolbuffers:mainfrom
vhullto:fix/python-map-clear-uaf

Conversation

@vhullto

@vhullto vhullto commented May 6, 2026

Copy link
Copy Markdown
Contributor

Bug

Clear() in map_container.cc (line 294-302) calls
reflection->ClearField() which destroys all underlying map nodes via
ClearTable(reset=true), but does not increment self->version.

All other mutators (ScalarMapSetItem, MessageMapSetItem, MergeFrom, etc.)
increment self->version after mutation. IterNext() relies on version
mismatch to detect concurrent modification and raise RuntimeError.
Without the version bump, a live iterator proceeds to dereference the
freed NodeBase* via SetMapIteratorValue → UntypedMapIterator::PlusPlus.

ASAN confirmed: heap-use-after-free, READ size 8 at
UntypedMapIterator::PlusPlus (map.h:599), freed by ClearTable
(map.h:345), allocated by ScalarMapSetItem (map_container.cc:416).

Fix

Add self->version++ after ClearField in Clear(), matching every
other mutator in the same file.

Reproducer

msg = M()  # proto3 with map<string, int32> mp
for k in ("a","b","c","d"): msg.mp[k] = 1
it = iter(msg.mp)
next(it)
msg.mp.clear()   # frees nodes, version NOT bumped
next(it)         # heap-use-after-free

@vhullto
vhullto requested a review from a team as a code owner May 6, 2026 20:03
@vhullto
vhullto requested review from anandolee and removed request for a team May 6, 2026 20:03
@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 13, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 13, 2026
@JasonLunn JasonLunn added python 🅰️ safe for tests Mark a commit as safe to run presubmits over labels May 13, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 13, 2026
@vhullto

vhullto commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

@JasonLunn could you re-apply the safe for tests label? Fixed the indentation issue in the test. Thank you!

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

sucloudflare commented May 19, 2026

Copy link
Copy Markdown

This PR fixes a heap-use-after-free reported to Google OSS VRP (Issue 514465085). The VRP team is waiting for the merge to process the report. Could a maintainer review this?

@vhullto

vhullto commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

Note: this PR also fixes the heap-use-after-free reported to Google OSS VRP (issue #510416822).

@vhullto

vhullto commented May 22, 2026

Copy link
Copy Markdown
Contributor Author

@anandolee could you take a look when you have a moment? All checks are passing. Thank you!

@vhullto

vhullto commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

@JasonLunn @rgoldfinger6 Could one of you review this when you have a moment? All 199 checks are passing and the fix is a single line (self->version++ in Clear()). Thank you!

@tonyliaoss tonyliaoss self-assigned this Jun 4, 2026
tonyliaoss
tonyliaoss approved these changes Jun 4, 2026
View reviewed changes

@tonyliaoss tonyliaoss left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This seems like a reasonable change.

@tonyliaoss tonyliaoss added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 4, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 4, 2026
@vhullto

vhullto commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

@tonyliaoss Could you re-apply the safe for tests label? Just merged upstream/main to fix the out-of-date branch. Thank you!

@tonyliaoss tonyliaoss added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 8, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 8, 2026
@vhullto

vhullto commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

@tonyliaoss The feedback/copybara check is failing on Google's internal side (the import/copybara import succeeded and the change merges cleanly). Could you take a look when you have a moment? The failure appears unrelated to this one-line change (self->version++ in Clear()). Thank you!

@tonyliaoss

tonyliaoss commented Jun 10, 2026

Copy link
Copy Markdown
Member

Yes, I see the corresponding CL internally is approved. I'm not sure why Copybara hasn't merged this change in yet, but for now there is no action needed on your side. Thanks!

@esrauchg esrauchg added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 17, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 17, 2026
@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 22, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 22, 2026
@sucloudflare

sucloudflare commented Jun 29, 2026

Copy link
Copy Markdown

This fix has been verified as merged via commit 9e9dbae (landed through Google's Copybara workflow).

This PR resolves the heap-use-after-free reported to Google OSS VRP as Issue 514465085. The root cause — MapClear() not incrementing self->version after ClearField() — allowed live iterators to dereference freed map nodes without triggering the version-mismatch guard in IterNext().

The regression test added in message_test.py (test_map_clear_during_iteration) reproduces the exact PoC from the report and confirms the fix is effective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonyliaoss tonyliaoss tonyliaoss approved these changes
@esrauchg esrauchg esrauchg approved these changes
@anandolee anandolee Awaiting requested review from anandolee anandolee is a code owner automatically assigned from protocolbuffers/protobuf-python

Assignees

@tonyliaoss tonyliaoss

Labels

python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vhullto @sucloudflare @tonyliaoss @esrauchg @JasonLunn @rgoldfinger6