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.

Sign up for GitHub

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

Jump to bottom

gh-116326: Handler errors correctly in getwindowsversion in sysmodule #116339

Merged
merged 2 commits into from
Mar 5, 2024
Merged

gh-116326: Handler errors correctly in getwindowsversion in sysmodule #116339

merged 2 commits into from
Mar 5, 2024

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Mar 4, 2024 •
edited by bedevere-app bot
Loading

  • PyErr_Occurred is no longer needed, because we check for all NULL values
  • I've also added an explicit error check when Py_BuildValue("(kkk)", ...) can fail

@sobolevn sobolevn requested a review from colesbury March 4, 2024 23:00
@bedevere-app bedevere-app bot mentioned this pull request Mar 4, 2024
Closed
@sobolevn sobolevn added skip news needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Mar 4, 2024
colesbury
colesbury approved these changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

LGTM!

@sobolevn
Copy link
Member Author

sobolevn commented Mar 4, 2024

Thanks! I would like to wait for a bit for @ericsnowcurrently's review as a code owner :)

serhiy-storchaka
serhiy-storchaka reviewed Mar 5, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

PyErr_Clear() calls in this function looks suspicious too. In good code it should only be called for expected exceptions, the others should be reported as errors.

Python/sysmodule.c Outdated Show resolved Hide resolved
@sobolevn
Copy link
Member Author

sobolevn commented Mar 5, 2024

In good code it should only be called for expected exceptions, the others should be reported as errors.

I think that this is a side-effect of using PyObject_GetAttrString, I can change it to be PyObject_GetOptionalAttrString 👍

@serhiy-storchaka
Copy link
Member

Yes, one of PyErr_Clears is for AttributeError, and PyObject_GetOptionalAttrString is a good alternative. There is another one, the exception type should be checked before it.

serhiy-storchaka
serhiy-storchaka approved these changes Mar 5, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@sobolevn sobolevn changed the title gh-116326: Handler errors correctly in getwindowsversion in `sysmodule gh-116326: Handler errors correctly in getwindowsversion in sysmodule Mar 5, 2024
@sobolevn sobolevn enabled auto-merge (squash) March 5, 2024 12:01
@sobolevn sobolevn merged commit c91bdf8 into python:main Mar 5, 2024
32 checks passed
@miss-islington-app
Copy link

Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 5, 2024
@sobolevn @miss-islington
pythongh-116326: Handler errors correctly in getwindowsversion in `…
6a44fb2 
…sysmodule` (pythonGH-116339)

(cherry picked from commit c91bdf8)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@miss-islington-app
Copy link

Sorry, @sobolevn, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker c91bdf86ef1cf9365b61a46aa2e51e5d1932b00a 3.11

@bedevere-app
Copy link

bedevere-app bot commented Mar 5, 2024

GH-116354 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Mar 5, 2024
@sobolevn
Copy link
Member Author

sobolevn commented Mar 5, 2024

@serhiy-storchaka what is the best way to do backports here? I forgot that PyObject_GetOptionalAttrString is 3.13+ only.

@serhiy-storchaka
Copy link
Member

Explicitly check for AttributeError.

sobolevn added a commit that referenced this pull request Mar 5, 2024
@miss-islington @sobolevn
[3.12] gh-116326: Handler errors correctly in getwindowsversion in …
9a42cfa 
…`sysmodule` (GH-116339) (#116354)

* gh-116326: Handler errors correctly in `getwindowsversion` in `sysmodule` (GH-116339)
(cherry picked from commit c91bdf8)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian root 3.12 has failed when building commit 9a42cfa.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1102/builds/751) and take a look at the build logs.
  4. Check if the failure is related to this commit (9a42cfa) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1102/builds/751

Summary of the results of the build (if available):

==

Click to see traceback logs 
remote: Enumerating objects: 47, done.        
remote: Counting objects:   2% (1/43)        
remote: Counting objects:   4% (2/43)        
remote: Counting objects:   6% (3/43)        
remote: Counting objects:   9% (4/43)        
remote: Counting objects:  11% (5/43)        
remote: Counting objects:  13% (6/43)        
remote: Counting objects:  16% (7/43)        
remote: Counting objects:  18% (8/43)        
remote: Counting objects:  20% (9/43)        
remote: Counting objects:  23% (10/43)        
remote: Counting objects:  25% (11/43)        
remote: Counting objects:  27% (12/43)        
remote: Counting objects:  30% (13/43)        
remote: Counting objects:  32% (14/43)        
remote: Counting objects:  34% (15/43)        
remote: Counting objects:  37% (16/43)        
remote: Counting objects:  39% (17/43)        
remote: Counting objects:  41% (18/43)        
remote: Counting objects:  44% (19/43)        
remote: Counting objects:  46% (20/43)        
remote: Counting objects:  48% (21/43)        
remote: Counting objects:  51% (22/43)        
remote: Counting objects:  53% (23/43)        
remote: Counting objects:  55% (24/43)        
remote: Counting objects:  58% (25/43)        
remote: Counting objects:  60% (26/43)        
remote: Counting objects:  62% (27/43)        
remote: Counting objects:  65% (28/43)        
remote: Counting objects:  67% (29/43)        
remote: Counting objects:  69% (30/43)        
remote: Counting objects:  72% (31/43)        
remote: Counting objects:  74% (32/43)        
remote: Counting objects:  76% (33/43)        
remote: Counting objects:  79% (34/43)        
remote: Counting objects:  81% (35/43)        
remote: Counting objects:  83% (36/43)        
remote: Counting objects:  86% (37/43)        
remote: Counting objects:  88% (38/43)        
remote: Counting objects:  90% (39/43)        
remote: Counting objects:  93% (40/43)        
remote: Counting objects:  95% (41/43)        
remote: Counting objects:  97% (42/43)        
remote: Counting objects: 100% (43/43)        
remote: Counting objects: 100% (43/43), done.        
remote: Compressing objects:   2% (1/34)        
remote: Compressing objects:   5% (2/34)        
remote: Compressing objects:   8% (3/34)        
remote: Compressing objects:  11% (4/34)        
remote: Compressing objects:  14% (5/34)        
remote: Compressing objects:  17% (6/34)        
remote: Compressing objects:  20% (7/34)        
remote: Compressing objects:  23% (8/34)        
remote: Compressing objects:  26% (9/34)        
remote: Compressing objects:  29% (10/34)        
remote: Compressing objects:  32% (11/34)        
remote: Compressing objects:  35% (12/34)        
remote: Compressing objects:  38% (13/34)        
remote: Compressing objects:  41% (14/34)        
remote: Compressing objects:  44% (15/34)        
remote: Compressing objects:  47% (16/34)        
remote: Compressing objects:  50% (17/34)        
remote: Compressing objects:  52% (18/34)        
remote: Compressing objects:  55% (19/34)        
remote: Compressing objects:  58% (20/34)        
remote: Compressing objects:  61% (21/34)        
remote: Compressing objects:  64% (22/34)        
remote: Compressing objects:  67% (23/34)        
remote: Compressing objects:  70% (24/34)        
remote: Compressing objects:  73% (25/34)        
remote: Compressing objects:  76% (26/34)        
remote: Compressing objects:  79% (27/34)        
remote: Compressing objects:  82% (28/34)        
remote: Compressing objects:  85% (29/34)        
remote: Compressing objects:  88% (30/34)        
remote: Compressing objects:  91% (31/34)        
remote: Compressing objects:  94% (32/34)        
remote: Compressing objects:  97% (33/34)        
remote: Compressing objects: 100% (34/34)        
remote: Compressing objects: 100% (34/34), done.        
remote: Total 47 (delta 15), reused 17 (delta 9), pack-reused 4        
From https://github.com/python/cpython
 * branch                  3.12       -> FETCH_HEAD
Note: switching to '9a42cfa368e7cfb7d4e920798e0c3abdc24de301'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 9a42cfa368 [3.12] gh-116326: Handler errors correctly in `getwindowsversion` in `sysmodule` (GH-116339) (#116354)
Switched to and reset branch '3.12'

configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

make: *** [Makefile:2011: buildbottest] Error 3

@sobolevn
Copy link
Member Author

sobolevn commented Mar 5, 2024

This failure also happened on a previous PR: https://buildbot.python.org/all/#/builders/1102/builds/751/steps/5/logs/stdio

Plus, it is a debian failure for a windows-only change.

sobolevn added a commit to sobolevn/cpython that referenced this pull request Mar 5, 2024
@sobolevn
[3.11] pythongh-116326: Handler errors correctly in `getwindowsversio…
210d5a6 
…n` in `sysmodule` (pythonGH-116339)

(cherry picked from commit c91bdf8)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@bedevere-app
Copy link

bedevere-app bot commented Mar 5, 2024

GH-116388 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Mar 5, 2024
sobolevn added a commit that referenced this pull request Mar 6, 2024
@sobolevn
[3.11] gh-116326: Handler errors correctly in getwindowsversion in …
23c17f3 
…`sysmodule` (GH-116339) (#116388)

(cherry picked from commit c91bdf8)
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
@sobolevn @adorilson
pythongh-116326: Handler errors correctly in getwindowsversion in `…
aad0f04 
…sysmodule` (python#116339)
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@sobolevn @diegorusso
pythongh-116326: Handler errors correctly in getwindowsversion in `…
5b90d7d 
…sysmodule` (python#116339)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@colesbury colesbury colesbury approved these changes

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees

@sobolevn sobolevn

Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sobolevn @serhiy-storchaka @bedevere-bot @colesbury