Skip to content

gh-126108: Fix potential null pointer dereference in PySys_AddWarnOptionUnicode #126118

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jan 31, 2025
Merged

gh-126108: Fix potential null pointer dereference in PySys_AddWarnOptionUnicode #126118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e355186
Fix potential null pointer dereference in PySys_AddWarnOptionUnicode
federicovalenso Oct 29, 2024
2ca65fe
📜🤖 Added by blurb_it.
blurb-it[bot] Oct 29, 2024
0fb17bc
Update Misc/NEWS.d/next/Security/2024-10-29-09-15-10.gh-issue-126108.…
federicovalenso Oct 29, 2024
6c22e44
add assert
federicovalenso Oct 29, 2024
6cd01fd
Merge branch 'bug/potential-null-pointer-dereference-in-PySys_AddWarn…
federicovalenso Oct 29, 2024
a969026
fix spaces
federicovalenso Oct 29, 2024
1667599
fix unbalanced literal markup
federicovalenso Oct 29, 2024
9323183
fix backticks
federicovalenso Oct 29, 2024
5cd8e30
Update Python/sysmodule.c
federicovalenso Oct 29, 2024
f234711
Merge branch 'main' into bug/potential-null-pointer-dereference-in-Py…
federicovalenso Oct 30, 2024
67b9de2
Apply suggession for Python/sysmodule.c
federicovalenso Oct 31, 2024
7d0754f
Apply suggession for Python/sysmodule.c
federicovalenso Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Security/2024-10-29-09-15-10.gh-issue-126108.eTIjHY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a possible ``NULL`` pointer dereference in :c:func:`!PySys_AddWarnOptionUnicode`.
7 changes: 4 additions & 3 deletions Python/sysmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2846,6 +2846,7 @@ PySys_ResetWarnOptions(void)
static int
_PySys_AddWarnOptionWithError(PyThreadState *tstate, PyObject *option)
{
assert(tstate != NULL);
PyObject *warnoptions = get_warnoptions(tstate);
if (warnoptions == NULL) {
return -1;
Expand All @@ -2861,11 +2862,11 @@ PyAPI_FUNC(void)
PySys_AddWarnOptionUnicode(PyObject *option)
{
PyThreadState *tstate = _PyThreadState_GET();
_Py_EnsureTstateNotNULL(tstate);
assert(!_PyErr_Occurred(tstate));
if (_PySys_AddWarnOptionWithError(tstate, option) < 0) {
/* No return value, therefore clear error state if possible */
if (tstate) {
_PyErr_Clear(tstate);
}
_PyErr_Clear(tstate);
Copy link
Member

Choose a reason for hiding this comment

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

I assume that there's no way to make tstate NULL in _PySys_AddWarnOptionWithError.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, assert is the way to go here. We should not call these functions in places where tstate can be NULL (while interpreter startup / shutdown).

Copy link
Member

Choose a reason for hiding this comment

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

Would it actually make sense to have _PyErr_Clear do this automatically?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, in that case, I would like PySys_AddWarnOptionUnicode to fail with a fatal error if called without the GIL. No-oping because of a null thread state isn't common at all in the C API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I do some other changes or leave as is?

Copy link
Member

@picnixz picnixz Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should we also do it in _PySys_AddWarnOptionWithError? WDYT @ZeroIntensity?

Copy link
Member

Choose a reason for hiding this comment

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

I think assert(tstate != NULL) is fine for the private API.

}
}

Expand Down
Loading