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-43984: Update winreg.SetValueEx to make sure the value set is not -1 by shreyanavigyan · Pull Request #25775 · python/cpython

Conversation

@shreyanavigyan

@shreyanavigyan shreyanavigyan commented May 1, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

bpo-43984: Update winreg.SetValueEx to make sure the value set is not -1

  • Fix REG_QWORD case in Py2Reg
  • Fix REG_DWORD case in Py2Reg
  • Add test
  • Make sure the test does not appear as virus to the antivirus software!

https://bugs.python.org/issue43984

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@shreyanavigyan
Fix for bpo-43984
1f2d523
@shreyanavigyan
shreyanavigyan requested a review from a team as a code owner May 1, 2021 09:48
@bedevere-bot bedevere-bot added the awaiting review label May 1, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 1, 2021
@shreyanavigyan

shreyanavigyan commented May 1, 2021

Copy link
Copy Markdown
Contributor Author

Skip news
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shreyanavigyan shreyanavigyan changed the title bpo-43984: Update winreg.SetValueEx to make sure the returned value is not -1 bpo-43984: Update winreg.SetValueEx to make sure the value set is not -1 May 1, 2021
erlend-aasland
erlend-aasland reviewed May 2, 2021
View reviewed changes
Comment thread PC/winreg.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@shreyanavigyan
Update - Free retDataBuf before returning FALSE
b00fc84
erlend-aasland
erlend-aasland reviewed May 2, 2021
View reviewed changes
Comment thread PC/winreg.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@shreyanavigyan
Update winreg.c
7841714
@shreyanavigyan

shreyanavigyan commented May 2, 2021

Copy link
Copy Markdown
Contributor Author

Can anyone please add a skip news label to this PR?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented May 2, 2021

Copy link
Copy Markdown
Contributor

Can anyone please add a skip news label to this PR?

Isn't this a change of behaviour? If so, I'd say it calls for a NEWS item.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shreyanavigyan
Add news entry
1123340
@shreyanavigyan

shreyanavigyan commented May 2, 2021

Copy link
Copy Markdown
Contributor Author

News entry added
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shreyanavigyan
shreyanavigyan requested review from pfmoore and removed request for a team May 2, 2021 10:55
@erlend-aasland

erlend-aasland commented May 2, 2021

Copy link
Copy Markdown
Contributor

Perhaps you could use the bug report to write a test case? (Lib/test/test_winreg.py)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@python python deleted a comment May 4, 2021
erlend-aasland
erlend-aasland reviewed May 8, 2021
View reviewed changes
Comment thread Misc/NEWS.d/next/Windows/2021-05-02-15-29-33.bpo-43984.U92jiv.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
shreyanavigyan added 2 commits May 8, 2021 23:59
@shreyanavigyan
Move the error check up before allocation
7bbb3f2
@shreyanavigyan
Update 2021-05-02-15-29-33.bpo-43984.U92jiv.rst
c5755dd
erlend-aasland
erlend-aasland requested changes May 8, 2021
View reviewed changes
Comment thread PC/winreg.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread PC/winreg.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Windows/2021-05-02-15-29-33.bpo-43984.U92jiv.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels May 8, 2021
shreyanavigyan and others added 4 commits May 9, 2021 00:35
@shreyanavigyan Erlend Egeberg Aasland
Update PC/winreg.c
ad67138 
Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@shreyanavigyan Erlend Egeberg Aasland
Update PC/winreg.c
ec0ea5a 
Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@shreyanavigyan
Add test
62541b8
@shreyanavigyan
Fix typo
27b8130
shreyanavigyan
shreyanavigyan commented May 8, 2021
View reviewed changes
Comment thread Lib/test/test_winreg.py
Comment on lines +343 to +357
def test_setvalueex_negative_one_check(self):
# Test for Issue #43984, check -1 was not set by SetValueEx.
# Py2Reg, which gets called by SetValueEx, wasn't checking return
# value by PyLong_AsUnsignedLong, thus setting -1 as value in the registry.
# The implementation now checks PyLong_AsUnsignedLong return value to assure
# the value set was not -1.
try:
with CreateKey(HKEY_CURRENT_USER, test_key_name) as ck:
with self.assertRaises(OverflowError):
SetValueEx(ck, "test_name", None, REG_DWORD, -1)
with self.assertRaises(FileNotFoundError):
with QueryValueEx(ck, "test_name") as subkey:
pass
finally:
DeleteKey(HKEY_CURRENT_USER, test_key_name)

@shreyanavigyan shreyanavigyan May 8, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

There's something wrong with this test I added. My antivirus is identifying this piece of code as virus. Any thoughts?

Sorry, something went wrong.

Uh oh!

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

@shreyanavigyan shreyanavigyan May 8, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If any maintainers figure out the problem please add the required commit. Who cannot add commits please suggest the patch, I'll commit it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@shreyanavigyan
Update 2021-05-02-15-29-33.bpo-43984.U92jiv.rst
41a70e0
@github-actions

github-actions Bot commented Jun 10, 2021

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 Jun 10, 2021
@shreyanavigyan

shreyanavigyan commented Jun 10, 2021

Copy link
Copy Markdown
Contributor Author

A Friendly Ping.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jun 11, 2021
@zooba

zooba commented Jul 6, 2021

Copy link
Copy Markdown
Member

Sorry for the lack of review. Unfortunately, it looks like someone added a new required check, so I'll close/reopen to re-run it, but the change looks okay to me. If someone else gets back to this before I do, they can merge.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zooba zooba closed this Jul 6, 2021
@zooba zooba reopened this Jul 6, 2021
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@iritkatriel
Merge branch 'main' into bpo-43984
666742c
@netlify

netlify Bot commented Dec 8, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

✅ Deploy Preview for python-cpython-preview canceled.

Name Link
🔨 Latest commit 06da229
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/6393215daba0e50008844eb4
All reactions

Sorry, something went wrong.

Uh oh!

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

zooba commented Dec 9, 2022

Copy link
Copy Markdown
Member

The test failure on Azure Pipelines is patchcheck reporting extra whitespace in the new test. I can't see it in the diff, but I find it's usually trailing whitespace in a comment.

We shouldn't merge with that failure, or it'll show up randomly in the future.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood
AlexWaygood reviewed Dec 9, 2022
View reviewed changes
Comment thread Lib/test/test_winreg.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_winreg.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
Fix whitespace
afdee9f
AlexWaygood
AlexWaygood reviewed Dec 9, 2022
View reviewed changes
Comment thread Lib/test/test_winreg.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_winreg.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
Never edit on your phone
06da229
@zooba zooba added needs backport to 3.10 needs backport to 3.11 only security fixes labels Dec 9, 2022
@zooba
zooba merged commit a29a7b9 into python:main Dec 9, 2022
@miss-islington

miss-islington commented Dec 9, 2022

Copy link
Copy Markdown
Contributor

Thanks @shreyanavigyan for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting core review label Dec 9, 2022
@bedevere-bot

bedevere-bot commented Dec 9, 2022

Copy link
Copy Markdown

GH-100134 is a backport of this pull request to the 3.11 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Dec 9, 2022
@bedevere-bot

bedevere-bot commented Dec 9, 2022

Copy link
Copy Markdown

GH-100135 is a backport of this pull request to the 3.10 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Dec 9, 2022
@zooba

zooba commented Dec 9, 2022

Copy link
Copy Markdown
Member

Thanks! FWIW, I suspect the antimalware detection was probably because it's an unsigned binary messing with the registry. Once we do our signed release, I'm sure it'll be fine.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 9, 2022
@shreyanavigyan @miss-islington
bpo-43984: Allow winreg.SetValueEx to set -1 without treating it as a…
f037156 
…n error (pythonGH-25775)

(cherry picked from commit a29a7b9)

Co-authored-by: Shreyan Avigyan <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 9, 2022
@shreyanavigyan @miss-islington
bpo-43984: Allow winreg.SetValueEx to set -1 without treating it as a…
93c7e67 
…n error (pythonGH-25775)

(cherry picked from commit a29a7b9)

Co-authored-by: Shreyan Avigyan <[email protected]>
@r3pwnx r3pwnx mannequin mentioned this pull request Apr 10, 2022
winreg.SetValueEx should check the returned value #88150
Closed
miss-islington added a commit that referenced this pull request Dec 9, 2022
@miss-islington @shreyanavigyan
bpo-43984: Allow winreg.SetValueEx to set -1 without treating it as a…
3939a4b 
…n error (GH-25775)

(cherry picked from commit a29a7b9)

Co-authored-by: Shreyan Avigyan <[email protected]>
miss-islington added a commit that referenced this pull request Dec 9, 2022
@miss-islington @shreyanavigyan
bpo-43984: Allow winreg.SetValueEx to set -1 without treating it as a…
580165d 
…n error (GH-25775)

(cherry picked from commit a29a7b9)

Co-authored-by: Shreyan Avigyan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tjguk tjguk tjguk left review comments
@eryksun eryksun eryksun left review comments
@erlend-aasland erlend-aasland erlend-aasland requested changes
@AlexWaygood AlexWaygood AlexWaygood left review comments
@pfmoore pfmoore Awaiting requested review from pfmoore

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

12 participants

@shreyanavigyan @erlend-aasland @eryksun @zooba @miss-islington @bedevere-bot @tjguk @pfmoore @AlexWaygood @the-knights-who-say-ni @ezio-melotti @iritkatriel
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.