Skip to content
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

bpo-37012: Fix a possible crash due to PyType_FromSpecWithBases() #10304

Merged
merged 1 commit into from May 9, 2019

Conversation

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented Nov 2, 2018

If the PyObject_MALLOC() call failed in PyType_FromSpecWithBases(),
PyObject_Free() would be called on a static string in type_dealloc().

https://bugs.python.org/issue37012

If the PyObject_MALLOC() call failed in PyType_FromSpecWithBases(),
PyObject_Free() would be called on a static string in type_dealloc().
@andresdelfino
Copy link
Contributor

andresdelfino commented Nov 14, 2018

@ZackerySpytz the "skip issue" label (and any other label for that matter) can only be added by developers. I was also confused when I started doing PRs, and tried adding "skip issue" in several places myself :) I mention this just to let you know why it didn't work.

@Mariatta
Copy link
Sponsor Member

Mariatta commented Nov 14, 2018

I think this might require issue and news entry.

@@ -2944,6 +2944,7 @@ PyType_FromSpecWithBases(PyType_Spec *spec, PyObject *bases)
size_t len = strlen(old_doc)+1;
char *tp_doc = PyObject_MALLOC(len);
if (tp_doc == NULL) {
type->tp_doc = NULL;
Copy link
Member

@serhiy-storchaka serhiy-storchaka Nov 14, 2018

Choose a reason for hiding this comment

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

In what circumstances it is not NULL? PyType_GenericAlloc() fills the type object with zeros.

@vstinner
Copy link
Member

vstinner commented Jan 10, 2019

I removed the " needs backport to 3.6" label, the 3.6 branch no longer accept bugfixes (only security fixes are accepted): https://devguide.python.org/#status-of-python-branches

@encukou
Copy link
Member

encukou commented May 8, 2019

I believe Serhyi's question is valid. If you have an answer, please re-open the PR.

(Is this silencing some static analyzer? )

@encukou encukou closed this May 8, 2019
@ZackerySpytz
Copy link
Contributor Author

ZackerySpytz commented May 8, 2019

@encukou Please reopen this PR.

*(void**)(res_start + slotoffsets[slot->slot]) = slot->pfunc; sets type->tp_doc to a pointer to a static string. A copy of the docstring is then made, but if the PyObject_MALLOC() call fails, type->tp_doc still points to the static string (e.g. SSLError_doc) when the type is decrefed.

@encukou encukou reopened this May 9, 2019
@encukou
Copy link
Member

encukou commented May 9, 2019

My apologies, you're right.

@encukou encukou merged commit 0613c1e into python:master May 9, 2019
5 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented May 9, 2019

Thanks @ZackerySpytz for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.7.
🐍🍒🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 9, 2019
If the PyObject_MALLOC() call failed in PyType_FromSpecWithBases(),
PyObject_Free() would be called on a static string in type_dealloc().
(cherry picked from commit 0613c1e)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
@bedevere-bot
Copy link

bedevere-bot commented May 9, 2019

GH-13223 is a backport of this pull request to the 3.7 branch.

@miss-islington
Copy link
Contributor

miss-islington commented May 9, 2019

Sorry, @ZackerySpytz and @encukou, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0613c1e481440aa8f54ba7f6056924c175fbcc13 2.7

@vstinner
Copy link
Member

vstinner commented May 10, 2019

Instead of reverting the change that we just made, what about not writing into tp_doc if it's value makes the type inconsistent? What about this patch:

diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index b28f494962..59d8a9d15f 100644
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -2986,7 +2986,6 @@ PyType_FromSpecWithBases(PyType_Spec *spec, PyObject *bases)
         if (slot->slot == Py_tp_base || slot->slot == Py_tp_bases)
             /* Processed above */
             continue;
-        *(void**)(res_start + slotoffsets[slot->slot]) = slot->pfunc;
 
         /* need to make a copy of the docstring slot, which usually
            points to a static string literal */
@@ -2995,13 +2994,15 @@ PyType_FromSpecWithBases(PyType_Spec *spec, PyObject *bases)
             size_t len = strlen(old_doc)+1;
             char *tp_doc = PyObject_MALLOC(len);
             if (tp_doc == NULL) {
-                type->tp_doc = NULL;
                 PyErr_NoMemory();
                 goto fail;
             }
             memcpy(tp_doc, old_doc, len);
             type->tp_doc = tp_doc;
         }
+        else {
+            *(void**)(res_start + slotoffsets[slot->slot]) = slot->pfunc;
+        }
 
         /* Move the slots to the heap type itself */
         if (slot->slot == Py_tp_members) {

@encukou encukou changed the title [Skip Issue] Fix a possible crash due to PyType_FromSpecWithBases() bpo-37012: Fix a possible crash due to PyType_FromSpecWithBases() May 22, 2019
encukou pushed a commit that referenced this pull request May 22, 2019
If the PyObject_MALLOC() call failed in PyType_FromSpecWithBases(),
PyObject_Free() would be called on a static string in type_dealloc().
(cherry picked from commit 0613c1e)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
encukou added a commit to encukou/cpython that referenced this pull request May 22, 2019
…signments

The main slot assignment loop is now if-else if ladder, making the
control flow clearer.

Based on suggestion by Victor Stinner in:
python#10304
ned-deily pushed a commit that referenced this pull request May 29, 2019
… (GH-13495)

If the PyObject_MALLOC() call failed in PyType_FromSpecWithBases(),
PyObject_Free() would be called on a static string in type_dealloc().
(cherry picked from commit 0613c1e)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
encukou added a commit that referenced this pull request Jun 2, 2019
…signments (GH-13496)

The main slot assignment loop is now if-else if ladder, making the
control flow clearer.

Based on suggestion by Victor Stinner in:
#10304
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
…signments (pythonGH-13496)

The main slot assignment loop is now if-else if ladder, making the
control flow clearer.

Based on suggestion by Victor Stinner in:
python#10304
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants