aio-libs / multidict / 36473694874

87%

push

github

Build popitem()'s result after removing the pair (#1625)

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

## What do these changes do?

`popitem()` built its result while it still pointed at the popped entry,
and read the entry's value afterwards. Building the result can run
Python code that mutates the multidict: a `str` subclass key's
`__str__()` when a `CIMultiDict` makes its `istr`, and on Python 3.10
and 3.11 a garbage collection the tuple allocation triggers, whose
finalizers run there. If that code replaced the table, the value came
from freed memory (a wrong value, or a crash). The C implementation now
takes its own references, removes the pair, and only then builds the
result, the order the pure-Python implementation already used.

The pure-Python `popitem()` decremented its size after building the key,
so it miscounted `len()` in the same case, and kept the old entry list
alive in a local until it returned; both are fixed.

## Are there changes in behavior for the user?

Yes: the use-after-free is gone. If building the key raises, the pair is
removed before the exception propagates, as the pure-Python
implementation always did; the C one used to leave it in place.

## Is it a substantial burden for the maintainers to support this?

No.

## Related issue number

Found by the finalizer audit in #1631; same class as #1619.

## Checklist

- [x] I think the code is well written
- [x] Unit tests for the changes exist
- [x] Documentation reflects the changes
- [ ] If you provide code modification, please add yourself to
`CONTRIBUTORS.txt` (N/A)
- [x] Add a new news fragment into the `CHANGES/` folder
- [x] `make doc-spelling` passes and any new technical words are added
to `docs/spelling_wordlist.txt`

<details>
<summary>Agent run details (optional, for reviewers)</summary>

New tests: `test_popitem_key_str_mutates` (fails on both backends
against master: C returns the value `'1'` read from the freed table,
pure Python miscounts `len`), `test_popitem_ke... (continued)

789 of 1578 branches covered (50.0%)

Branch coverage included in aggregate %.

56 of 56 new or added lines in 2 files covered. (100.0%)

8833 of 9473 relevant lines covered (93.24%)

1.86 hits per line