-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
gh-148798: Fix crash in _interpreters.create() on non-UTF-8 string in config #148799
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,6 +117,18 @@ def test_in_main(self): | |
| # GH-126221: Passing an invalid Unicode character used to cause a SystemError | ||
| self.assertRaises(UnicodeEncodeError, _interpreters.create, '\udc80') | ||
|
|
||
| # A config object with a surrogate in a string field must raise, not crash. | ||
| class BadConfig: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a need to set all other fields?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switched to _interpreters.new_config() in 02053a9 so only .gil is set. Also fixes the CI failure on main where BadConfig()'s instance dict was empty. |
||
| use_main_obmalloc = False | ||
| allow_fork = False | ||
| allow_exec = False | ||
| allow_threads = False | ||
| allow_daemon_threads = False | ||
| check_multi_interp_extensions = False | ||
| own_gil = True | ||
| gil = 'own\udc80' | ||
| self.assertRaises(UnicodeEncodeError, _interpreters.create, BadConfig()) | ||
|
|
||
| def test_in_thread(self): | ||
| lock = threading.Lock() | ||
| interp = None | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| Fix a crash in :func:`!_interpreters.create` when a config object passes a | ||
| string with an unpaired surrogate as a value (for example ``gil``). The | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be confused with gil being the "bad" word. Instead, just mention that the crash was fixed if the value had unpaires surrogates (but do not mention which keys they could be associated with)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 02053a9. |
||
| internal helper ``_config_dict_copy_str`` now checks the return of | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not think it is necessary to mention the internals. Just mentioning interpreter's possible failures should be enough.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 02053a9. |
||
| :c:func:`PyUnicode_AsUTF8` before copying, turning the segfault into a | ||
| :exc:`UnicodeEncodeError`. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,7 +133,12 @@ _config_dict_copy_str(PyObject *dict, const char *name, | |
| config_dict_invalid_type(name); | ||
| return -1; | ||
| } | ||
| strncpy(buf, PyUnicode_AsUTF8(item), bufsize-1); | ||
| const char *utf8 = PyUnicode_AsUTF8(item); | ||
| if (utf8 == NULL) { | ||
| Py_DECREF(item); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should maybe add a note to the just-raised exception to indicate for which key it failed. But this can be done as a follow-up.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will send as a follow-up PR. |
||
| return -1; | ||
| } | ||
| strncpy(buf, utf8, bufsize-1); | ||
| buf[bufsize-1] = '\0'; | ||
| Py_DECREF(item); | ||
| return 0; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it a separate test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 02053a9.