bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem() by serhiy-storchaka · Pull Request #11112 · python/cpython

serhiy-storchaka · Dec 11, 2018

https://bugs.python.org/issue35459

Objects/dictobject.c

ericsnowcurrently

First of all, thanks for working on this! :) Overall it looks good.

My main concern with this PR is changing semantics. From what I can tell you're introducing a bunch of changes in behavior, albeit it corner error cases. What is the risk to compatibility? My gut tells me there's at least a slight risk.

Secondly, you've touched a lot of critical code. Please make sure to run the benchmark suite to ensure the PR doesn't slow down Python. :)

Also, there a number of places where I wanted to suggest a better spelling. However, such changes would be slightly riskier and would mostly clutter up the PR, obscuring the core changes. So I've left out those comments and focused mostly on checking correctness.

Finally, the most likely thing I might have missed in this review is refcounts. You've added quite a few places that exit early when there's an error. I'm not sure that I checked to make sure everything was properly decref'ed in those new error cases.

Modules/_csv.c

ericsnowcurrently · Dec 12, 2018

Modules/_sre.c

-            PyExc_IndexError,
-            "no such group"
-            );
+    if (index < 0) {


Was this change intentional? At first glance it looks like the code you've removed actually matters.

Is this function only ever called with a pre-validated index (e.g. the one returned from match_getindex())? If so, it would be helpful to have a comment here indicating that validation of the index must be done by the caller. And if that's the case then why have this check (and short-circuit) here?

The removed code was moved into match_getindex() because it was repeated after every call of match_getindex(). This function is only called with valid index or the result of match_getindex(). This check was here just to make the caller place simpler. Will move it to the caller place and add an assert instead.

Modules/pyexpat.c

ericsnowcurrently · Dec 12, 2018

Modules/pyexpat.c

    }
    Py_DECREF(errmod_name);
-    model_module = PyDict_GetItem(d, modelmod_name);
+    model_module = PyDict_GetItemWithError(d, modelmod_name);


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

It is returned below, at line 1713.

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

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).

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

Already added.

ericsnowcurrently · Dec 12, 2018

Objects/dictobject.c

-            int status = PyDict_SetItem(d, key, value);
-            if (status < 0) {
+        if (override || PyDict_GetItemWithError(d, key) == NULL) {
+            if ((!override && PyErr_Occurred()) || PyDict_SetItem(d, key, value) < 0) {


This is a little hard to read. There's a lot going on in these two lines. Perhaps split it up a little?

ericsnowcurrently · Dec 12, 2018

Objects/typeobject.c

 add_methods(PyTypeObject *type, PyMethodDef *meth)
 {
    PyObject *dict = type->tp_dict;
+    PyObject *name;


I'd put this down where it's first used (i.e. right above line 4852 if (isdescr) {).

Objects/typeobject.c

Python/ceval.c

Python/getargs.c

bedevere-bot · Dec 12, 2018

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

ericsnowcurrently · Dec 12, 2018

Incidentally, how many uses of the non-WithError API remain after this?

serhiy-storchaka

Thank you for your review @ericsnowcurrently!

serhiy-storchaka · Dec 13, 2018

Modules/_sre.c

-            PyExc_IndexError,
-            "no such group"
-            );
+    if (index < 0) {


The removed code was moved into match_getindex() because it was repeated after every call of match_getindex(). This function is only called with valid index or the result of match_getindex(). This check was here just to make the caller place simpler. Will move it to the caller place and add an assert instead.

Modules/_csv.c

Modules/pyexpat.c

serhiy-storchaka · Dec 13, 2018

Modules/pyexpat.c

    }
    Py_DECREF(errmod_name);
-    model_module = PyDict_GetItem(d, modelmod_name);
+    model_module = PyDict_GetItemWithError(d, modelmod_name);


It is returned below, at line 1713.

Objects/dictobject.c

Objects/typeobject.c

Python/getargs.c

serhiy-storchaka · Dec 16, 2018

As for benchmarks, running the benchmark suite exposes some slowdown on some tests, but results can have significant random component. I'll research this in more details to get more trustworthy result.

serhiy-storchaka · Feb 14, 2019

I have made the requested changes; please review again.

bedevere-bot · Feb 14, 2019

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem().

6b79d33

serhiy-storchaka added the skip issue label Dec 11, 2018

serhiy-storchaka requested a review from rhettinger as a code owner December 11, 2018 10:56

serhiy-storchaka requested a review from a team December 11, 2018 10:56

the-knights-who-say-ni added the CLA signed label Dec 11, 2018

bedevere-bot added the awaiting merge label Dec 11, 2018

serhiy-storchaka added skip news and removed skip issue labels Dec 11, 2018

ZackerySpytz reviewed Dec 11, 2018

View reviewed changes

Objects/dictobject.c Outdated Show resolved Hide resolved

ericsnowcurrently requested changes Dec 12, 2018

View reviewed changes

bedevere-bot removed the awaiting merge label Dec 12, 2018

bedevere-bot added the awaiting changes label Dec 12, 2018

serhiy-storchaka added 2 commits December 13, 2018 08:53

Address review comments.

b2f44b7

Merge branch 'master' into pydict-getitem

41f2ec3

serhiy-storchaka commented Dec 13, 2018

View reviewed changes

Merge branch 'master' into pydict-getitem

15046cb

Merge branch 'master' into pydict-getitem

bce5923

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 14, 2019

Merge branch 'master' into pydict-getitem

fe7a98f

brettcannon changed the title ~~bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem().~~ bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem() Feb 16, 2019

serhiy-storchaka merged commit a24107b into python:master Feb 25, 2019

bedevere-bot removed the awaiting change review label Feb 25, 2019

serhiy-storchaka deleted the pydict-getitem branch February 25, 2019 15:59

Jongy mentioned this pull request Oct 26, 2020

bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object #22953

Merged

virtuald mentioned this pull request Feb 17, 2021

[BUG] Unchecked errors can potentially swallow signals/other exceptions pybind/pybind11#2862

Open

serhiy-storchaka mentioned this pull request Jun 23, 2023

PyDict_GetItem and PyObject_HasAttr should not be used #106033

Closed

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

serhiy-storchaka commented Dec 11, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Dec 12, 2018

Uh oh!

ericsnowcurrently commented Dec 12, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka commented Dec 16, 2018

Uh oh!

serhiy-storchaka commented Feb 14, 2019

Uh oh!

bedevere-bot commented Feb 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

serhiy-storchaka commented Dec 11, 2018 •

edited by bedevere-bot

Loading