gh-143004: Fix UAF in _collections Counter update fast path by Kaushalt2004 · Pull Request #143044 · python/cpython

Kaushalt2004 · 2025-12-21T15:15:22Z

Problem
_collections._count_elements has a fast-path for dict that reads oldval via _PyDict_GetItem_KnownHash() (borrowed reference) and then calls PyNumber_Add(oldval, one). PyNumber_Add() can run arbitrary Python code (e.g. add), allowing re-entrant mutation such as Counter.clear(). That can drop the last reference to oldval while the C code still holds a borrowed pointer, resulting in a use-after-free (ASAN).

Reproducer

Fix
In the fast dict path, keep oldval alive across PyNumber_Add() by temporarily owning a reference:

Py_INCREF(oldval); newval = PyNumber_Add(oldval, one); Py_DECREF(oldval);
This prevents oldval from being freed if user code re-enters and clears/mutates the mapping during the add.

Changes

_collectionsmodule.c: INCREF/DECREF oldval around PyNumber_Add() in the fast path.
test_collections.py: add regression test test_update_reentrant_add_clears_counter.
Tests

./python -m test test_collections -k reentrant_add -v

Issue: Use-after-free in Counter.update via re-entrant __add__ #143004

bedevere-app · 2025-12-21T15:15:25Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

python-cla-bot · 2025-12-21T15:15:25Z

All commit authors signed the Contributor License Agreement.

StanFromIreland

Please create an issue and add a blurb.

skirpichev · 2025-12-21T20:54:29Z

Please create an issue

@StanFromIreland, it's #143004.

@Kaushalt2004, while technically this seems to be correct, this is not something that solves real-world problem. Could you please provide benchmarks to show how this will impact more typical use-cases for Counter()?

Kaushalt2004 · 2025-12-22T06:25:05Z

I ran a small microbenchmark to estimate the impact of the Py_INCREF(oldval) / Py_DECREF(oldval) pair added around PyNumber_Add() in the dict fast-path.

Setup

Windows x64
Built both revisions with the same compiler/toolchain to keep the comparison fair.
Note: my VS install is VS 2026; build.bat expects older MSBuild toolset targets (v143). I built both revisions via MSBuild using PlatformToolset=v145 (same for “before” and “after”).
How

Before: origin/main (b8d3fdd)
After: this PR branch (ce88c7e)
Build + run commands (same for both revisions; only git checkout changes)

subst X: "C:\Users\...\New folder (12)"
cd /d X:\cpython
MSBuild pcbuild.proj /t:Build /m /nologo /v:m /clp:summary /p:Configuration=Release /p:Platform=x64 /p:PlatformToolset=v145 /p:IncludeExternals=true /p:IncludeCTypes=true /p:IncludeSSL=true /p:IncludeTkinter=false
PCbuild\amd64\python.exe Tools\scripts\bench_counter_update.py --repeats 25 --inner-loops 50 --n-keys 1000 --n-elems 200000
Results (ns/call; lower is better)

update(unique) from empty: 26218.0 → 26438.0 (+0.84%)
update(dupes) from empty: 4520476.0 → 4502494.0 (-0.40%)
update(dupes) preseeded (hits the INCREF/DECREF path every time): 4474634.0 → 4499386.0 (+0.55%)
update(string dupes) empty: 6083882.0 → 6028120.0 (-0.92%)
Conclusion

Deltas are within ~±1% on this machine; no meaningful regression observed, including the “preseeded” case that exercises the new INCREF/DECREF on every update.
If you want it even tighter (1–2 paragraphs), tell me whether you prefer “minimal” or “detailed,” and I’ll rewrite it.I ran a small microbenchmark to estimate the impact of the Py_INCREF(oldval) / Py_DECREF(oldval) pair added around PyNumber_Add() in the dict fast-path.

Setup

Windows x64
Built both revisions with the same compiler/toolchain to keep the comparison fair.
Note: my VS install is VS 2026; build.bat expects older MSBuild toolset targets (v143). I built both revisions via MSBuild using PlatformToolset=v145 (same for “before” and “after”).
How

Before: origin/main (b8d3fdd)
After: this PR branch (ce88c7e)
Build + run commands (same for both revisions; only git checkout changes)

subst X: "C:\Users\...\New folder (12)"
cd /d X:\cpython
MSBuild pcbuild.proj /t:Build /m /nologo /v:m /clp:summary /p:Configuration=Release /p:Platform=x64 /p:PlatformToolset=v145 /p:IncludeExternals=true /p:IncludeCTypes=true /p:IncludeSSL=true /p:IncludeTkinter=false
PCbuild\amd64\python.exe Tools\scripts\bench_counter_update.py --repeats 25 --inner-loops 50 --n-keys 1000 --n-elems 200000
Results (ns/call; lower is better)

update(unique) from empty: 26218.0 → 26438.0 (+0.84%)
update(dupes) from empty: 4520476.0 → 4502494.0 (-0.40%)
update(dupes) preseeded (hits the INCREF/DECREF path every time): 4474634.0 → 4499386.0 (+0.55%)
update(string dupes) empty: 6083882.0 → 6028120.0 (-0.92%)
Conclusion

Deltas are within ~±1% on this machine; no meaningful regression observed, including the “preseeded” case that exercises the new INCREF/DECREF on every update.

bedevere-app · 2025-12-22T06:30:55Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

skirpichev · 2025-12-22T06:45:18Z

I ran a small microbenchmark

I still see no benchmark code.

PS: @Kaushalt2004, do not click Update branch without a good reason because it notifies everyone watching the PR that there are new changes, when there are not, and it uses up limited CI resources.

Misc/NEWS.d/next/Library/2025-12-22-00-00-00.gh-issue-143004.uaf-counter.rst

Tools/scripts/bench_counter_update.py

Kaushalt2004 · 2025-12-22T08:10:24Z

Thanks for the feedback.

I’ve removed the benchmark script from the PR as requested.
I’ve also adjusted the NEWS entry formatting.

Benchmark methodology and results were shared earlier in comments;
they show no meaningful regression (~±1%).

Please let me know if you’d like the benchmark rerun using pyperf
or if further changes are needed.

Lib/test/test_collections.py

Misc/NEWS.d/next/Library/2025-12-22-00-00-00.gh-issue-143004.uaf-counter.rst

…af-counter.rst Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

Lib/test/test_collections.py

Misc/NEWS.d/next/Library/2025-12-22-00-00-00.gh-issue-143004.uaf-counter.rst

…af-counter.rst Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…new__ override, rely on __add__ only. Clarify with reviewer note. (python#1 branch)

Kaushalt2004 · 2025-12-25T07:02:32Z

Thanks for the clarification. You’re right that Python guarantees subclass construction, and the test does not demonstrate any loss of subclass identity.
The custom new was added defensively, but since this behavior cannot be concretely shown, it is unnecessary.
I will simplify the test by removing the custom constructor and keep it focused on the re-entrant mutation via add.

Lib/test/test_collections.py

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

serhiy-storchaka

LGTM. 👍

miss-islington-app · 2025-12-25T09:13:43Z

Thanks @Kaushalt2004 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

…pdate() (pythonGH-143044) This happened when the Counter was mutated when incrementing the value for an existing key. (cherry picked from commit 86d9045) Co-authored-by: kaushal trivedi <155625932+Kaushalt2004@users.noreply.github.com>

bedevere-app · 2025-12-25T09:13:55Z

GH-143166 is a backport of this pull request to the 3.14 branch.

bedevere-app · 2025-12-25T09:13:59Z

GH-143167 is a backport of this pull request to the 3.13 branch.

…update() (GH-143044) (GH-143166) This happened when the Counter was mutated when incrementing the value for an existing key. (cherry picked from commit 86d9045) Co-authored-by: kaushal trivedi <155625932+Kaushalt2004@users.noreply.github.com>

…update() (GH-143044) (GH-143167) This happened when the Counter was mutated when incrementing the value for an existing key. (cherry picked from commit 86d9045) Co-authored-by: kaushal trivedi <155625932+Kaushalt2004@users.noreply.github.com>

…pdate() (pythonGH-143044) This happened when the Counter was mutated when incrementing the value for an existing key.

Kaushalt2004 · 2026-02-26T08:25:49Z

I’m interested in continuing work in this area and exploring whether it could form the basis of a structured GSoC project. I’d appreciate guidance on current priorities.

Fix UAF in _collections Counter update fast path

ce88c7e

Kaushalt2004 requested a review from rhettinger as a code owner December 21, 2025 15:15

bedevere-app bot added the awaiting review label Dec 21, 2025

StanFromIreland reviewed Dec 21, 2025

View reviewed changes

skirpichev changed the title ~~Fix UAF in _collections Counter update fast path~~ gh-143004: Fix UAF in _collections Counter update fast path Dec 21, 2025

bedevere-app bot mentioned this pull request Dec 21, 2025

Use-after-free in Counter.update via re-entrant __add__ #143004

Closed

Merge branch 'main' into python#1

e0036f1

pythongh-143004: Add Counter UAF NEWS entry and benchmark script

9ed24b4

skirpichev reviewed Dec 22, 2025

View reviewed changes

Misc/NEWS.d/next/Library/2025-12-22-00-00-00.gh-issue-143004.uaf-counter.rst Outdated Show resolved Hide resolved

Tools/scripts/bench_counter_update.py Outdated Show resolved Hide resolved

fixed after review

ae5cb7e

serhiy-storchaka reviewed Dec 22, 2025

View reviewed changes

Lib/test/test_collections.py Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Library/2025-12-22-00-00-00.gh-issue-143004.uaf-counter.rst Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Library/2025-12-22-00-00-00.gh-issue-143004.uaf-counter.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Library/2025-12-22-00-00-00.gh-issue-143004.u…

7712af2

…af-counter.rst Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

skirpichev reviewed Dec 23, 2025

View reviewed changes

Lib/test/test_collections.py Outdated Show resolved Hide resolved

serhiy-storchaka reviewed Dec 23, 2025

View reviewed changes

Misc/NEWS.d/next/Library/2025-12-22-00-00-00.gh-issue-143004.uaf-counter.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Library/2025-12-22-00-00-00.gh-issue-143004.u…

93a6546

…af-counter.rst Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

rhettinger approved these changes Dec 23, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Dec 23, 2025

serhiy-storchaka added the DO-NOT-MERGE label Dec 23, 2025

Simplify Evil class in re-entrant Counter test: remove unnecessary __…

49cff07

…new__ override, rely on __add__ only. Clarify with reviewer note. (python#1 branch)

skirpichev approved these changes Dec 25, 2025

View reviewed changes

skirpichev requested a review from serhiy-storchaka December 25, 2025 07:16

serhiy-storchaka reviewed Dec 25, 2025

View reviewed changes

Lib/test/test_collections.py Outdated Show resolved Hide resolved

Apply suggestion from @serhiy-storchaka

81d4672

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

serhiy-storchaka approved these changes Dec 25, 2025

View reviewed changes

serhiy-storchaka added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes and removed DO-NOT-MERGE labels Dec 25, 2025

serhiy-storchaka enabled auto-merge (squash) December 25, 2025 09:00

serhiy-storchaka merged commit 86d9045 into python:main Dec 25, 2025
51 of 52 checks passed

bedevere-app bot removed the awaiting merge label Dec 25, 2025

bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Dec 25, 2025

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Dec 25, 2025

Uh oh!

Conversation

Kaushalt2004 commented Dec 21, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented Dec 21, 2025

Uh oh!

python-cla-bot bot commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StanFromIreland left a comment

Choose a reason for hiding this comment

Uh oh!

skirpichev commented Dec 21, 2025

Uh oh!

Kaushalt2004 commented Dec 22, 2025

Uh oh!

bedevere-app bot commented Dec 22, 2025

Uh oh!

skirpichev commented Dec 22, 2025

Uh oh!

Uh oh!

Uh oh!

Kaushalt2004 commented Dec 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kaushalt2004 commented Dec 25, 2025

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington-app bot commented Dec 25, 2025

Uh oh!

bedevere-app bot commented Dec 25, 2025

Uh oh!

bedevere-app bot commented Dec 25, 2025

Uh oh!

Kaushalt2004 commented Feb 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Kaushalt2004 commented Dec 21, 2025 •

edited by bedevere-app bot

Loading

python-cla-bot bot commented Dec 21, 2025 •

edited

Loading