Skip to content

bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem() #11112

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
serhiy-storchaka merged 6 commits into from
Feb 25, 2019
Merged

bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem() #11112

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6b79d33
bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem().
serhiy-storchaka Dec 9, 2018
b2f44b7
Address review comments.
serhiy-storchaka Dec 13, 2018
41f2ec3
Merge branch 'master' into pydict-getitem
serhiy-storchaka Dec 13, 2018
15046cb
Merge branch 'master' into pydict-getitem
serhiy-storchaka Dec 15, 2018
bce5923
Merge branch 'master' into pydict-getitem
serhiy-storchaka Jan 19, 2019
fe7a98f
Merge branch 'master' into pydict-getitem
serhiy-storchaka Feb 14, 2019
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
Prev Previous commit
Next Next commit
Address review comments.
  • Loading branch information
@serhiy-storchaka
serhiy-storchaka committed Dec 13, 2018
commit b2f44b7a6676d8d7b5bc35db3c13cec537dd3bc6
13 changes: 8 additions & 5 deletions Modules/_sre.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1899,10 +1899,7 @@ match_getslice_by_index(MatchObject* self, Py_ssize_t index, PyObject* def)
void* ptr;
Py_ssize_t i, j;

if (index < 0) {
return NULL;
}

assert(0 <= index && index < self->groups);
index *= 2;

if (self->string == Py_None || self->mark[index] < 0) {
Expand Down Expand Up @@ -1961,7 +1958,13 @@ match_getindex(MatchObject* self, PyObject* index)
static PyObject*
match_getslice(MatchObject* self, PyObject* index, PyObject* def)
{
return match_getslice_by_index(self, match_getindex(self, index), def);
Py_ssize_t i = match_getindex(self, index);

if (i < 0) {
return NULL;
}

return match_getslice_by_index(self, i, def);
}

/*[clinic input]
Expand Down
2 changes: 1 addition & 1 deletion Modules/pyexpat.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1701,7 +1701,7 @@ MODULE_INITFUNC(void)
}
Py_DECREF(errmod_name);
model_module = PyDict_GetItemWithError(d, modelmod_name);
Copy link
Copy Markdown
Member

@ericsnowcurrently ericsnowcurrently Dec 12, 2018

Choose a reason for hiding this comment

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

Doesn't the error need to be returned or cleared?

Copy link
Copy Markdown
Member Author

@serhiy-storchaka serhiy-storchaka Dec 13, 2018

Choose a reason for hiding this comment

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

It is returned below, at line 1713.

Copy link
Copy Markdown
Member

@ericsnowcurrently ericsnowcurrently Dec 13, 2018

Choose a reason for hiding this comment

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

The model_module gets reset on line 1705, so the error from the first attempt may remain uncleared or swallowed

Copy link
Copy Markdown
Member Author

@serhiy-storchaka serhiy-storchaka Dec 13, 2018

Choose a reason for hiding this comment

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

Line 1705 is executed only no error was raised at this line.

Error handling in this function (as well as in many other module initialization functions) is pretty poor. Results of PyModule_AddObject() and derived functions are not checked, and references are leaked in case of error. But this is different issue(s).

Copy link
Copy Markdown
Member

@ericsnowcurrently ericsnowcurrently Dec 13, 2018

Choose a reason for hiding this comment

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

Then shouldn't line 1704 have && !PyErr_Occurred() like line 1694 does?

Copy link
Copy Markdown
Member Author

@serhiy-storchaka serhiy-storchaka Dec 13, 2018

Choose a reason for hiding this comment

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

Already added.

if (model_module == NULL) {
if (model_module == NULL && !PyErr_Occurred()) {
model_module = PyModule_New(MODULE_NAME ".model");
if (model_module != NULL) {
_PyImport_SetModule(modelmod_name, model_module);
Expand Down
11 changes: 9 additions & 2 deletions Objects/dictobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2371,8 +2371,15 @@ PyDict_MergeFromSeq2(PyObject *d, PyObject *seq2, int override)
value = PySequence_Fast_GET_ITEM(fast, 1);
Py_INCREF(key);
Py_INCREF(value);
if (override || PyDict_GetItemWithError(d, key) == NULL) {
if ((!override && PyErr_Occurred()) || PyDict_SetItem(d, key, value) < 0) {
if (override) {
if (PyDict_SetItem(d, key, value) < 0) {
Py_DECREF(key);
Py_DECREF(value);
goto Fail;
}
}
else if (PyDict_GetItemWithError(d, key) == NULL) {
if (PyErr_Occurred() || PyDict_SetItem(d, key, value) < 0) {
Py_DECREF(key);
Py_DECREF(value);
goto Fail;
Expand Down
5 changes: 4 additions & 1 deletion Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3767,9 +3767,12 @@ object_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
return NULL;
}
comma = _PyUnicode_FromId(&comma_id);
if (comma == NULL)
if (comma == NULL) {
Py_DECREF(sorted_methods);
return NULL;
Copy link
Copy Markdown
Member

@ericsnowcurrently ericsnowcurrently Dec 12, 2018

Choose a reason for hiding this comment

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

Doesn't sorted_methods need to be decref'ed before returning?

Copy link
Copy Markdown
Member Author

@serhiy-storchaka serhiy-storchaka Dec 13, 2018

Choose a reason for hiding this comment

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

Good catch!

}
joined = PyUnicode_Join(comma, sorted_methods);
Py_DECREF(sorted_methods);
if (joined == NULL)
return NULL;
Copy link
Copy Markdown
Member

@ericsnowcurrently ericsnowcurrently Dec 12, 2018

Choose a reason for hiding this comment

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

Doesn't sorted_methods need to be decref'ed before returning? I suppose you could do it right after calling PyUnicode_Join().

Copy link
Copy Markdown
Member Author

@serhiy-storchaka serhiy-storchaka Dec 13, 2018

Choose a reason for hiding this comment

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

Good catch!


Expand Down