Skip to content

fix format error that leads to segfaults#1310

Open
Vizonex wants to merge 18 commits intoaio-libs:masterfrom
Vizonex:err-fix
Open

fix format error that leads to segfaults#1310
Vizonex wants to merge 18 commits intoaio-libs:masterfrom
Vizonex:err-fix

Conversation

@Vizonex
Copy link
Copy Markdown
Member

@Vizonex Vizonex commented Mar 28, 2026

What do these changes do?

This was based off a user-accessible segfault found by @devdanzin. Not sure how this appeared out of nowhere but my best assumption it could've been caused by developer fatigue or burn out. The reproducible seen in this gist here https://gist.github.com/devdanzin/a36588ae7e2b73f0d85f8a925fb13b3d is not just limited to 3.14 but also 3.10 (which I have now recently tested myself) so I've gone ahead and fixed it. Feel free to let me know if this should somehow be added to pytest although IDK where to put it. Originally was going to write this off as an ffmpeg fiasco but this does not seem to be the case. Seems we might have couple of other sneaky bugs to patch.

Reproducer from the gist brought here for reference

"""Reproducer: multidict _err_cannot_fetch swapped format args — abort.

hashtable.h:1467-1475 passes (name, i) but format expects (i, name).
  %zd receives a char* → prints garbage number
  %s receives an int → dereferences integer as pointer → crash

No special setup required.
Tested: multidict 6.7.1, Python 3.14.
"""
from multidict import MultiDict

class BadItem:
    """Looks like a 2-element sequence but __getitem__ raises."""
    def __len__(self):
        return 2
    def __getitem__(self, i):
        raise RuntimeError("intentional getitem failure")

MultiDict([BadItem()])
# Expected: ValueError with proper error message
# Actual: Aborted (core dumped)
#   Assertion `!_PyErr_Occurred(tstate)' failed. 

Are there changes in behavior for the user?

Related issue number

Note

This does not close this issue as there is still other information about other bugs that have yet to be patched.

#1306

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@Vizonex Vizonex requested a review from asvetlov as a code owner March 28, 2026 18:32
@Vizonex Vizonex requested a review from webknjaz as a code owner March 28, 2026 18:35
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Mar 28, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 28, 2026

Merging this PR will not alter performance

✅ 245 untouched benchmarks


Comparing Vizonex:err-fix (edc2a98) with master (ecc83e1)1

Open in CodSpeed

Footnotes

  1. No successful run was found on master (8233cb3) during the generation of this report, so ecc83e1 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 29, 2026

Fix looks good. Can you add a test?

Comment thread tests/isolated/multidict_bad_item.py Fixed
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 31, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.85%. Comparing base (8233cb3) to head (edc2a98).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1310   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          26       26           
  Lines        3513     3539   +26     
  Branches      253      254    +1     
=======================================
+ Hits         3508     3534   +26     
  Misses          3        3           
  Partials        2        2           
Flag Coverage Δ
CI-GHA 99.85% <100.00%> (+<0.01%) ⬆️
pytest 99.85% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Vizonex Vizonex mentioned this pull request Apr 3, 2026
3 tasks
Comment thread CHANGES/1310.bugfix.rst Outdated
Vizonex and others added 2 commits April 3, 2026 20:29
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Comment thread tests/isolated/multidict_bad_item.py Outdated
Comment thread tests/test_multidict.py Dismissed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a user-triggerable crash during MultiDict construction from an update sequence when an element’s __getitem__ fails, by correcting the C extension error formatting and adding a regression test (with a matching behavior tweak in the pure-Python implementation).

Changes:

  • Fix swapped PyErr_Format arguments in _err_cannot_fetch to prevent invalid pointer formatting/segfaults.
  • Add a regression test covering an update-sequence element whose __getitem__ raises.
  • Update the pure-Python argument parsing path to raise a consistent ValueError when element indexing fails.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/test_multidict.py Adds regression test for failing __getitem__ during construction.
multidict/_multilib/hashtable.h Fixes _err_cannot_fetch format argument order to prevent crash.
multidict/_multidict_py.py Wraps item indexing failures into a ValueError for consistency with C extension.
CHANGES/1310.bugfix.rst Adds changelog note for the crash fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread multidict/_multidict_py.py Outdated
Comment thread CHANGES/1310.bugfix.rst Outdated
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 13, 2026

stall in test_cannot_create_from_item_with_failing_getitem[c-cls0]

Comment thread tests/test_multidict.py Dismissed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants