Skip to content

Commit edb61e8

Browse files
Vizonexbdracowebknjaz
authored
fix format error that leads to segfaults (#1310)
<!-- Thank you for your contribution! --> ## 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 ```python """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? <!-- Outline any notable behaviour for the end users. --> ## 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 <!-- Are there any issues opened that will be resolved by merging this change? --> <!-- Remember to prefix with 'Fixes' if it should close the issue (e.g. 'Fixes #123'). --> ## Checklist - [x] I think the code is well written - [x] Unit tests for the changes exist - [x] Documentation reflects the changes --------- Co-authored-by: J. Nick Koston <nick@koston.org> Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua> Co-authored-by: J. Nick Koston <nick@home-assistant.io>
1 parent 6e5577d commit edb61e8

File tree

4 files changed

+57
-5
lines changed

4 files changed

+57
-5
lines changed

CHANGES/1310.bugfix.rst

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
A segmentation fault that could be triggered when getting an item is now fixed
2+
-- by :user:`Vizonex`.

multidict/_multidict_py.py

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -834,8 +834,22 @@ def _parse_args(
834834
f"multidict update sequence element #{pos} "
835835
f"has length {len(item)}; 2 is required"
836836
)
837-
identity = identity_func(item[0])
838-
yield _Entry(hash(identity), identity, item[0], item[1])
837+
try:
838+
key = item[0]
839+
except Exception as exc:
840+
raise ValueError(
841+
f"multidict update sequence element #{pos}'s "
842+
f"key could not be fetched"
843+
) from exc
844+
try:
845+
value = item[1]
846+
except Exception as exc:
847+
raise ValueError(
848+
f"multidict update sequence element #{pos}'s "
849+
f"value could not be fetched"
850+
) from exc
851+
identity = identity_func(key)
852+
yield _Entry(hash(identity), identity, key, value)
839853
else:
840854
yield len(kwargs)
841855
for key, value in kwargs.items():

multidict/_multilib/hashtable.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1470,8 +1470,8 @@ _err_cannot_fetch(Py_ssize_t i, const char *name)
14701470
PyErr_Format(PyExc_ValueError,
14711471
"multidict update sequence element #%zd's "
14721472
"%s could not be fetched",
1473-
name,
1474-
i);
1473+
i,
1474+
name);
14751475
}
14761476

14771477
static int
@@ -1507,11 +1507,11 @@ _md_parse_item(Py_ssize_t i, PyObject *item, PyObject **pkey,
15071507
goto fail;
15081508
}
15091509
*pkey = PySequence_ITEM(item, 0);
1510-
*pvalue = PySequence_ITEM(item, 1);
15111510
if (*pkey == NULL) {
15121511
_err_cannot_fetch(i, "key");
15131512
goto fail;
15141513
}
1514+
*pvalue = PySequence_ITEM(item, 1);
15151515
if (*pvalue == NULL) {
15161516
_err_cannot_fetch(i, "value");
15171517
goto fail;

tests/test_multidict.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,42 @@ def test_cannot_create_from_unaccepted(
300300
):
301301
cls([(1, 2, 3)]) # type: ignore[call-arg]
302302

303+
def test_cannot_create_from_item_with_failing_getitem(
304+
self,
305+
cls: type[MutableMultiMapping[str]],
306+
) -> None:
307+
class BadItem:
308+
def __len__(self) -> int:
309+
return 2
310+
311+
def __getitem__(self, i: int) -> object:
312+
raise RuntimeError("intentional getitem failure")
313+
314+
with pytest.raises(
315+
ValueError,
316+
match=r"^multidict update sequence element #0's key could not be fetched$",
317+
):
318+
cls([BadItem()]) # type: ignore[call-arg]
319+
320+
def test_cannot_create_from_item_with_failing_getitem_value(
321+
self,
322+
cls: type[MutableMultiMapping[str]],
323+
) -> None:
324+
class BadValueItem:
325+
def __len__(self) -> int:
326+
return 2
327+
328+
def __getitem__(self, i: int) -> object:
329+
if i == 0:
330+
return "key"
331+
raise RuntimeError("intentional getitem failure")
332+
333+
with pytest.raises(
334+
ValueError,
335+
match=r"^multidict update sequence element #0's value could not be fetched$",
336+
):
337+
cls([BadValueItem()]) # type: ignore[call-arg]
338+
303339
def test_keys_is_set_less(self, cls: type[MultiDict[str]]) -> None:
304340
d = cls([("key", "value1")])
305341

0 commit comments

Comments
 (0)