bpo-28129: Add NULL checks #403
Conversation
|
@tiran, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @amauryfa, @Haypo, @vadmium and @birkenfeld to be potential reviewers. |
It looks like one of them should be CID 1401655. |
Coverity complains about three new potential NULL derefs. One is a false positive but the two remaining look valid. CID 1401656 CID 1401655 Signed-off-by: Christian Heimes <[email protected]>
tiran
force-pushed
the
bpo-28129-coverity
branch
from
69a1455 to
0efd813
Compare
|
At first glance both cases look false positive to me. Is there a reproducer? |
| } | ||
|
|
||
| dict = PyType_stgdict((PyObject *)type); | ||
| if (dict == NULL) |
There was a problem hiding this comment.
We've started to put curly braces around one-liners: https://www.python.org/dev/peps/pep-0007/#code-lay-out.
There was a problem hiding this comment.
Aaaaah, thanks for the confirmation @brettcannon ! I had a long discussion with @serhiy-storchaka about that ;-)
| } | ||
|
|
||
| dict = PyType_stgdict((PyObject *)type); | ||
| if (dict == NULL) |
There was a problem hiding this comment.
Aaaaah, thanks for the confirmation @brettcannon ! I had a long discussion with @serhiy-storchaka about that ;-)
|
LGTM, but please see @brettcannon comment on coding style. |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I have checked the code yet once and confirm that these changes are not needed. PyType_stgdict() never returns NULL in that cases.
| Py_DECREF(result); | ||
| return NULL; | ||
| } | ||
| sw_dict = PyType_stgdict(swapped); |
There was a problem hiding this comment.
PyType_stgdict() never returns NULL here since CreateSwappedType() just created a type with PyCStgDict.
| return -1; | ||
| } | ||
|
|
||
| dict = PyType_stgdict((PyObject *)type); |
There was a problem hiding this comment.
PyType_stgdict() never returns NULL here since _init_pos_args() is called only for _ctypes.Structure and _ctypes.Union which creates types with PyCStgDict or recursively for their base classes after checking that PyType_stgdict() is not NULL.
|
If the code path cannot happen (false positive), can be fix the warning using an assertion? |
zhangyangyu
left a comment
zhangyangyu
left a comment
There was a problem hiding this comment.
I scan the code and concur with Serhiy don't see the NULL code path.
|
I think it is worth to add asserts. If NULL is happen here, this is our programming error. |
|
Sorry, pressed wrong button. |
9 participants
Coverity complains about three new potential NULL derefs. One is a false
positive but the two remaining look valid.
CID 1401656
CID 1401655
Signed-off-by: Christian Heimes [email protected]