GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-42034:Fixed unchecked return in Objects/typeobject.c by monocle-ai · Pull Request #22695 · python/cpython

Conversation

@monocle-ai

@monocle-ai monocle-ai commented Oct 14, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

The return value of a function that is potentially used to initialize a local variable is not checked. Therefore, reading the local variable may result in undefined behavior.

Our AI analyzer found that this function is called for a total of 43 times. Out of these 43 times, the return value from the function call is checked at 42 instances. This is the only instance where the code misses to check the return value for success or failure.

Once such correct reference usage found in Python/hamt.c at line 2805 .

Code Extract:

PyObject *key;
PyObject *def = NULL;

if (!PyArg_UnpackTuple(args, "get", 1, 2, &key, &def)) {
    return NULL;
}

https://bugs.python.org/issue42034

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@monocle-ai
Fixed unchecked return in Objects/typeobject.c
f5452af 
Unchecked return in Objects/typeobject.c and possible uninitialized variables in cls and new_mro
@the-knights-who-say-ni

the-knights-who-say-ni commented Oct 14, 2020

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@monocle-ai

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Oct 14, 2020
@bedevere-bot bedevere-bot added the awaiting review label Oct 14, 2020
@da-woods

da-woods commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

Isn't the point that this is the function clean-up code? It wants to be run in full whether or not everything succeeds. By returning early you leak temp and also all the clean-up in the bail section.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@monocle-ai

monocle-ai commented Oct 15, 2020

Copy link
Copy Markdown
Author

Yes, you are right. The function of the cleanup code is to run everything be it a success or a failure. But what happens when cls remains uninitialized after the function call PyArg_UnpackTuple on Line 826 and soon after that the code tries to access cls and use it's attribute tp_mro on Line 831 (if (cls->tp_mro == new_mro))? The code would then throw an exception and code to clean up temp would not be called and this exception would then be passed up the function call till it is handled gracefully.

But you are right in pointing out that if we return based on the function failure, the code to clean up temp Py_DECREF(temp) will never be called. So, now I will only call the temp cleanup no matter what and instead of returning in case of failure, the code will now execute if block only in case of success. I believe this should solve both problems.

As for the bail section, it is called before a call is made to the undo section (Line 794). So any change to the undo section would not have any impact on the bail section.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
monocle-ai added 2 commits October 15, 2020 06:22
@monocle-ai
Update typeobject.c
0a13d68 
Removed return to call cleanup on temp in all the cases
@monocle-ai
Create 2020-10-15-06-28-50.bpo-42034.MonocleAI.rst
a196e6a
@github-actions

github-actions Bot commented Dec 17, 2020

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Dec 17, 2020
@hugovk hugovk mentioned this pull request Feb 17, 2022
Cron job to close stale PRs python/core-workflow#93
Closed
@github-actions

github-actions Bot commented Feb 19, 2022

Copy link
Copy Markdown

Closing this stale PR because the CLA is still not signed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot closed this Feb 19, 2022
@monocle-ai monocle-ai mannequin mentioned this pull request Apr 10, 2022
Unchecked return in Objects/typeobject.c and possible uninitialized variables in cls and new_mro #86200
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@monocle-ai @the-knights-who-say-ni @da-woods @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.