aio-libs / multidict / 35592134197

87%

push

github

Stop items() iteration from reading a freed entry in CIMultiDict (#1496)

<!-- Thank you for your contribution! -->

## What do these changes do?

Fixes a use-after-free in the C extension's key conversion that crashed
the cp315t wheel test on master
([job](https://github.com/aio-libs/multidict/actions/runs/35579952505/job/106270507947),
`list(md.items())` in `test_race_condition_iterator_vs_mutation`).

When iterating a `CIMultiDict` whose stored key is a plain `str`,
`_md_ensure_key()` caches the `istr` in the entry, drops the old key,
and then read `entry->key` again. Dropping the old key can run a `str`
subclass's `__del__`, or on free-threaded builds suspend the iterator's
critical section; either lets the table be mutated and the entry freed
before that reload. Building the `istr` itself can run a `str`
subclass's `__str__`, which freed `entry->identity` while `IStr_New()`
was still using it.

`_md_ensure_key()` now holds its own references across the conversion,
only caches the `istr` if the version is unchanged, and returns the
computed key instead of reloading it. `md_clone_from_ht()` now assigns a
new version instead of copying the source's, so a `__str__` that calls
`d.__init__(d.copy())` can no longer restore the version that check
relies on; the pure Python implementation already behaved this way.
`md_next()`, `md_prev()` and both finders read the value before the key,
so the decrefs are the last access to the entry. The pure Python
implementation does not cache the converted key and is unaffected.

## Are there changes in behavior for the user?

No, apart from the crash being gone. `getversion()` of a `copy()` or of
a multidict re-initialized from another one now differs from the
source's on the C extension too, as it already did in pure Python. A
mutation from a key's `__str__`/`__del__` during iteration now surfaces
as the usual `RuntimeError: MultiDict is changed during iteration`.

## Is it a substantial burden for the maintainers to... (continued)

762 of 1524 branches covered (50.0%)

Branch coverage included in aggregate %.

46 of 48 new or added lines in 2 files covered. (95.83%)

6455 of 6727 relevant lines covered (95.96%)

1.92 hits per line