gh-151862: Fix NULL deref on non-str AttributeError when unpickling - #151863
gh-151862: Fix NULL deref on non-str AttributeError when unpickling#151863tonghuaroot wants to merge 3 commits into
Conversation
…ling An object crossing interpreters via a queue or channel is unpickled on the receive side. When that unpickling raised an ``AttributeError`` whose first argument was not a UTF-8-encodable string, ``PyUnicode_AsUTF8()`` returned ``NULL`` and the subsequent ``strncmp(NULL, ...)`` in ``check_missing___main___attr()`` dereferenced it, crashing the interpreter. Guard against the ``NULL`` result and treat it as not a missing-``__main__``-attribute error.
| } | ||
| } | ||
| const char *err = PyUnicode_AsUTF8(msgobj); | ||
| if (err == NULL) { |
There was a problem hiding this comment.
I believe we should add another PyUnicode_Check before this call.
There was a problem hiding this comment.
We need the result check in any case, because PyUnicode_AsUTF8() can fail for other reasons -- embedded surrogates or failed memory allocation. If the errors were not silenced, PyUnicode_Check() would be more preferable, but in the current code it would not have advantage.
There was a problem hiding this comment.
I proposed PyUnicode_Check not as opposite, but as an addition to current check.
serhiy-storchaka
added
needs backport to 3.15
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
./python -m test test_interpreters.test_queues runs now the whole test suite. This is a regression of this PR.
| } | ||
| } | ||
| const char *err = PyUnicode_AsUTF8(msgobj); | ||
| if (err == NULL) { |
There was a problem hiding this comment.
We need the result check in any case, because PyUnicode_AsUTF8() can fail for other reasons -- embedded surrogates or failed memory allocation. If the errors were not silenced, PyUnicode_Check() would be more preferable, but in the current code it would not have advantage.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
The positive-control arg crafted a "module '__main__' has no attribute" message, which the cross-interpreter path matched and re-executed __main__ (the test runner) in-process. Drop that case and build the module in memory instead of writing a file, so test_interpreters.test_queues runs in isolation again.
|
Fixed in 7cadf94: the positive-control arg spelled a I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @serhiy-storchaka: please review the changes made to this pull request. |
3 participants
Python/crossinterp.c'scheck_missing___main___attr()passed the result ofPyUnicode_AsUTF8()straight tostrncmp()without aNULLcheck.msgobjisargs[0]of anAttributeErrorraised on the receive side of the cross-interpreter pickle fallback (aconcurrent.interpretersqueue or channel). Whenargs[0]is not astr(AttributeError(42),AttributeError(b'x'),AttributeError(None)) or is astrwith lone surrogates (AttributeError('\ud800')),PyUnicode_AsUTF8()returnsNULLandstrncmp(NULL, ...)crashes the interpreter.This adds a
NULLguard matching the checked sibling helper_copy_string_obj_raw(). BecausePyUnicode_AsUTF8sets an exception on failure and the function asserts!PyErr_Occurred()on entry, the guard clears it,Py_DECREF(msgobj)(mirroring the success-path decref), and returns 0 (not a missing-__main__attribute), so the failure degrades to the normalNotShareableError.A regression test is added in
Lib/test/test_interpreters/test_queues.pycovering all fourargs[0]shapes (42,b'x',None,'\ud800'— the surrogate case is a distinct branch:args[0]is unicode but fails UTF-8 encoding), plus a positive control with normalstrargs (including the genuine missing-__main__-attribute message shape) to lock in the non-NULLstrncmp()path. Without the fix the test crashes; with it the queue raisesNotShareableError.This fixes the
check_missing___main___attr()crash reached through the queue/channel receive path. A separate crash on theInterpreter.call()result-preserve path is reported independently.