gh-143004: Fix UAF in _collections Counter update fast path

[Kaushalt2004]

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]

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]

All commit authors signed the Contributor License Agreement.

[StanFromIreland]

Please create an issue and add a blurb.

[skirpichev]

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]

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]

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]

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.

[skirpichev]

Suggested change

	gh-143004: Fix a potential use-after-free in collections.Counter.update() when user code mutates the Counter during an update.
	Fix a potential use-after-free in collections.Counter.update() when user code
	mutates the Counter during an update.

[Kaushalt2004]

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.

[serhiy-storchaka]

Is it needed?

[Kaushalt2004]

Yes, this is intentional.
The custom new ensures the instance starts at 0 and that add is exercised during Counter.update(), which is required to reproduce the re-entrant mutation scenario reliably.

[serhiy-storchaka]

Does not it have zero value by default? What happens when remove the user constructor?

[Kaushalt2004]

While int() defaults to zero, the explicit new ensures a deterministic starting value for the subclass instance and guarantees that add is exercised during Counter.update().
This makes the re-entrant mutation scenario reliable; removing it can make the test fragile or fail to reproduce the issue.

[serhiy-storchaka]

How is it possible? What prevent __add__() to be called if the object use the default constructor?

[Kaushalt2004]

In this test, the intent is indeed that the Counter stores an instance of Evil.

The explicit_new is not because other values are expected, but to make that guarantee explicit and robust.

Without it, CPython is free to materialize or replace the value with a plain int during Counter. update(), which would bypass the overridden_add and make the test implementation-dependent.

The custom constructor ensures the value stored is definitively an Evil instance and

that PyNumber_Add() dispatches to

Evil._add reliably.

[serhiy-storchaka]

Please show how the Evil instance is replaced with a plain int and how the test without a custom __new__ is not robust?

[Kaushalt2004]

In the current implementation, the test intends that the value stored is an instance of Evil, and that Evil.add is invoked during Counter.update().
The explicit new is not because a plain int is known to be substituted today, but to make this invariant explicit and robust.
Without it, the test would rely on incidental behavior of how the initial value is materialized and preserved, which CPython does not guarantee and which could change with future optimizations.
Defining new ensures the value is definitively an Evil instance and that PyNumber_Add() reliably dispatches to the overridden add.

[skirpichev]

Without it, the test would rely on incidental behavior of how the initial value is materialized and preserved, which CPython does not guarantee

Why do you think that Python (not even CPython!) don't guarantee this? Subclass constructor per default will return a subclass instance, not some base class.

[Kaushalt2004]

You’re right — Python guarantees that constructing a subclass returns an instance of that subclass, and the test does not currently demonstrate any loss of subclass identity.
The custom new is therefore unnecessary, and the test can be simplified without losing coverage.

[serhiy-storchaka]

The issue number is already included in the file name.

[skirpichev]

Suggested change

	def __new__(cls):
	return int.__new__(cls, 0)

[Kaushalt2004]

This is intentional.
Overriding new ensures the value starts at 0 and that add is invoked during Counter.update(), which is required to reliably trigger the re-entrant mutation scenario covered by this test.