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

gh-106307: Fix PyMapping_GetOptionalItemString() #108797

Conversation

serhiy-storchaka
Copy link
Member

The resulting pointer was not set to NULL if the creation of a temporary string object was failed.

The tests were also missed due to oversight.

The resulting pointer was not set to NULL if the creation of a temporary
string object was failed.

The tests were also missed due to oversight.
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I just propose adding a comment on the new macro.

Does Valgrind or similar tool detect the usage of uninitialized memory?

Modules/_testcapi/util.h Outdated Show resolved Hide resolved
@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) September 6, 2023 19:39
@serhiy-storchaka serhiy-storchaka merged commit 3a08db8 into python:main Sep 6, 2023
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

The _testcapi now intializes the variable used in the test:

static PyObject *
mapping_getoptionalitemstring(PyObject *self, PyObject *args)
{
    PyObject *obj, *value = UNINITIALIZED_PTR;

@vstinner
Copy link
Member

vstinner commented Sep 6, 2023

@erlend-aasland: Idea for the devguide: We should suggest to always initialized the &value parameter to NULL in case of errors. I just avoid any risk of using an uninitialized variable by misusing the C API. What do you think?

@erlend-aasland
Copy link
Contributor

@erlend-aasland: Idea for the devguide: We should suggest to always initialized the &value parameter to NULL in case of errors. I just avoid any risk of using an uninitialized variable by misusing the C API. What do you think?

I have a PR for this, and you already approved it twice 😃 python/devguide#1128

@serhiy-storchaka
Copy link
Member Author

There are exceptions, like _PyEval_SliceIndex(), where the output parameter should not be set in some cases.

@serhiy-storchaka serhiy-storchaka deleted the PyMapping_GetOptionalItemString-errors branch September 7, 2023 09:07
@erlend-aasland
Copy link
Contributor

Exceptions are fine; guidelines are for the general case, not the special case.

@vstinner
Copy link
Member

vstinner commented Sep 7, 2023

I have a PR for this, and you already approved it twice 😃 python/devguide#1128

Alright, I should approve it 3 or 4 times 👍👍👍👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants