aio-libs / multidict / 34862819179

87%

push

github

Fix items() set algebra corrupting state on reentrant equality (#1453)

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

## What do these changes do?

`items()` set algebra (`&`, `|`, `-`, `in`, `isdisjoint()`) walks a
key's
hash chain while temporarily marking visited entries, then compares
stored
values against a caller-supplied one before unmarking. That comparison
can
run arbitrary user code through a custom `__eq__`, and code that calls
back
into the same `MultiDict` from there (`getall()`, for example) while the
chain is still marked observes some matching entries as missing.
Depending
on the operation this returns wrong data, raises a bogus `KeyError`, or,
in
the C extension, hangs the process outright (see
[#1317
(review)](https://github.com/aio-libs/multidict/pull/1317#pullrequestreview-5187377912)
for the report and a reproduction).

The fix materializes the matching `(key, value)` pairs into a plain list
after the hash chain has been fully walked and restored, and only then
runs
the value comparisons against that snapshot. This covers the C
extension's
`and1`/`and2`/`or1`/`sub2`/`contains`/`isdisjoint` paths via a new
`md_finder_collect()` helper, and the pure-Python `__and__()`, the only
pure-Python method using the same mark-then-compare pattern (the other
pure-Python methods don't mark and were never affected).

## Are there changes in behavior for the user?

No, other than the crash/corruption going away. Ordinary comparisons (an
`__eq__` that doesn't touch the multidict) behave exactly as before.

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

No. The fix is a mechanical materialize-then-compare change with no new
public API, plus regression tests.

## Related issue number

Follows up on the review comment at

https://github.com/aio-libs/multidict/pull/1317#pullrequestreview-5187377912.
No separate issue is filed.

## Checklist

- [x] I think the code is well written
- [x] Unit tests for the changes exist
- [x] Documenta... (continued)

719 of 1438 branches covered (50.0%)

Branch coverage included in aggregate %.

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

5259 of 5505 relevant lines covered (95.53%)

1.91 hits per line