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

PYTHON-3959 - NULL Initialize PyObjects #1859

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aclark4life
Copy link

NULL initialize PyObjects before calling PyErr_*

Copy link

There is an existing patch(es) for this commit SHA:

Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'.

Copy link
Contributor

@Jibola Jibola left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Jibola Jibola left a comment

Choose a reason for hiding this comment

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

Actually, I see there are some non NULL-initialized PyObjects. I think we should target all the PyObject* instantiations as I'm assuming this has only targeted the ones that leverage PyErr, the original scope of the PR. Looking for suggestions from others.

bson/_cbsonmodule.c Show resolved Hide resolved
@ShaneHarvey
Copy link
Member

ShaneHarvey commented Sep 16, 2024

The jira ticket, PYTHON-3959, mentions initializing vars for PyErr_Fetch. Should this PR be updating those?

@aclark4life
Copy link
Author

The jira ticket, PYTHON-3959, mentions initializing vars for PyErr_Fetch. Should this PR be updating those?

PYTHON-3959 mentions initializing PyObject variables so that PyErr_* calls are less noisy in Coverity reports. This PR only initializes PyObject variables that precede calls to PyErr_*. As @Jibola mentions above we could also initialize all PyObject variables too for "good measure", but if there are other proposed changes to be made in addition to that, I'm not aware of them yet.

@ShaneHarvey
Copy link
Member

So are we going to update the PyErr_Fetch calls?

@aclark4life
Copy link
Author

So are we going to update the PyErr_Fetch calls?

Yes! Sorry, In my first pass I misread which PyObject variables were called by which PyErr_ functions. Fixed.

@ShaneHarvey
Copy link
Member

I know this is only changing NULLS but just to be safe we should wait until 4.9 is released to merge.

@ShaneHarvey ShaneHarvey removed their request for review September 17, 2024 23:10
@ShaneHarvey
Copy link
Member

I'll defer to Jib for the rest.

Copy link
Contributor

@Jibola Jibola left a comment

Choose a reason for hiding this comment

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

Missed two in _cbsonmodule.c

Thanks for the work thus far, though! I imagine navigating this c file is a bit pesky.

pymongo/_cmessagemodule.c Outdated Show resolved Hide resolved
@aclark4life
Copy link
Author

Thanks for the work thus far, though! I imagine navigating this c file is a bit pesky.

Got'em, thanks! C is a pleasure to work with 👼

Copy link
Contributor

@Jibola Jibola left a comment

Choose a reason for hiding this comment

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

LGTM!!!

@blink1073
Copy link
Member

Let's hold off on merging this until we address https://jira.mongodb.org/browse/PYTHON-4636

NULL initialize PyObjects before calling PyErr_*
Initialize all PyObjects, not just those that included in calls to
PyErr_*.
Missed a few, thanks Jib!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants