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

shreyanavigyan · 2021-05-01T09:48:28Z

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

shreyanavigyan · 2021-05-01T09:48:51Z

Skip news

PC/winreg.c

shreyanavigyan · 2021-05-02T09:25:01Z

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

erlend-aasland · 2021-05-02T09:45:57Z

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.

shreyanavigyan · 2021-05-02T10:00:33Z

News entry added

erlend-aasland · 2021-05-02T11:12:27Z

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

Misc/NEWS.d/next/Windows/2021-05-02-15-29-33.bpo-43984.U92jiv.rst

PC/winreg.c

Misc/NEWS.d/next/Windows/2021-05-02-15-29-33.bpo-43984.U92jiv.rst

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>

shreyanavigyan · 2021-05-08T19:49:48Z

Lib/test/test_winreg.py

+    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)


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

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

github-actions · 2021-06-10T00:04:29Z

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

shreyanavigyan · 2021-06-10T16:14:03Z

A Friendly Ping.

zooba · 2021-07-06T23:37:01Z

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.

netlify · 2022-12-08T18:56:06Z

✅ 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

zooba · 2022-12-09T11:39:51Z

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.

Lib/test/test_winreg.py

miss-islington · 2022-12-09T12:47:21Z

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

bedevere-bot · 2022-12-09T12:47:43Z

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

bedevere-bot · 2022-12-09T12:47:48Z

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

zooba · 2022-12-09T12:47:51Z

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.

…n error (pythonGH-25775) (cherry picked from commit a29a7b9) Co-authored-by: Shreyan Avigyan <shreyan.avigyan@gmail.com>

…n error (GH-25775) (cherry picked from commit a29a7b9) Co-authored-by: Shreyan Avigyan <shreyan.avigyan@gmail.com>

Fix for bpo-43984

1f2d523

shreyanavigyan requested a review from a team as a code owner May 1, 2021 09:48

bedevere-bot added the awaiting review label May 1, 2021

the-knights-who-say-ni added the CLA signed label May 1, 2021

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 reviewed May 2, 2021

View reviewed changes

PC/winreg.c Show resolved Hide resolved

Update - Free retDataBuf before returning FALSE

b00fc84

erlend-aasland reviewed May 2, 2021

View reviewed changes

PC/winreg.c Outdated Show resolved Hide resolved

Update winreg.c

7841714

Add news entry

1123340

shreyanavigyan requested review from pfmoore and removed request for a team May 2, 2021 10:55

python deleted a comment May 4, 2021

erlend-aasland reviewed May 8, 2021

View reviewed changes

Misc/NEWS.d/next/Windows/2021-05-02-15-29-33.bpo-43984.U92jiv.rst Outdated Show resolved Hide resolved

shreyanavigyan added 2 commits May 8, 2021 23:59

Move the error check up before allocation

7bbb3f2

Update 2021-05-02-15-29-33.bpo-43984.U92jiv.rst

c5755dd

erlend-aasland requested changes May 8, 2021

View reviewed changes

PC/winreg.c Outdated Show resolved Hide resolved

PC/winreg.c Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Windows/2021-05-02-15-29-33.bpo-43984.U92jiv.rst Outdated Show resolved Hide resolved

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

Update PC/winreg.c

ad67138

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>

Update PC/winreg.c

ec0ea5a

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>

Add test

62541b8

Fix typo

27b8130

shreyanavigyan commented May 8, 2021

View reviewed changes

Update 2021-05-02-15-29-33.bpo-43984.U92jiv.rst

41a70e0

github-actions bot added the stale Stale PR or inactive for long period of time. label Jun 10, 2021

github-actions bot removed the stale Stale PR or inactive for long period of time. label Jun 11, 2021

zooba closed this Jul 6, 2021

zooba reopened this Jul 6, 2021

ezio-melotti removed the CLA signed label Jul 13, 2022

Merge branch 'main' into bpo-43984

666742c

AlexWaygood reviewed Dec 9, 2022

View reviewed changes

Lib/test/test_winreg.py Outdated Show resolved Hide resolved

Lib/test/test_winreg.py Outdated Show resolved Hide resolved

Fix whitespace

afdee9f

AlexWaygood reviewed Dec 9, 2022

View reviewed changes

Lib/test/test_winreg.py Show resolved Hide resolved

Lib/test/test_winreg.py Show resolved Hide resolved

Never edit on your phone

06da229

zooba added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Dec 9, 2022

zooba merged commit a29a7b9 into python:main Dec 9, 2022

bedevere-bot removed the awaiting core review label Dec 9, 2022

bedevere-bot removed the needs backport to 3.11 only security fixes label Dec 9, 2022

bedevere-bot removed the needs backport to 3.10 only security fixes label Dec 9, 2022

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

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 <shreyan.avigyan@gmail.com>

miss-islington added a commit that referenced this pull request Dec 9, 2022

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 <shreyan.avigyan@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shreyanavigyan commented May 1, 2021 •

edited

Loading

shreyanavigyan commented May 1, 2021

shreyanavigyan commented May 2, 2021

erlend-aasland commented May 2, 2021

shreyanavigyan commented May 2, 2021

erlend-aasland commented May 2, 2021

shreyanavigyan May 8, 2021

shreyanavigyan May 8, 2021 •

edited

Loading

github-actions bot commented Jun 10, 2021

shreyanavigyan commented Jun 10, 2021

zooba commented Jul 6, 2021

netlify bot commented Dec 8, 2022 •

edited

Loading

zooba commented Dec 9, 2022

miss-islington commented Dec 9, 2022

bedevere-bot commented Dec 9, 2022

bedevere-bot commented Dec 9, 2022

zooba commented Dec 9, 2022

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

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

Conversation

shreyanavigyan commented May 1, 2021 • edited Loading

shreyanavigyan commented May 1, 2021

shreyanavigyan commented May 2, 2021

erlend-aasland commented May 2, 2021

shreyanavigyan commented May 2, 2021

erlend-aasland commented May 2, 2021

shreyanavigyan May 8, 2021

Choose a reason for hiding this comment

shreyanavigyan May 8, 2021 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jun 10, 2021

shreyanavigyan commented Jun 10, 2021

zooba commented Jul 6, 2021

netlify bot commented Dec 8, 2022 • edited Loading

✅ Deploy Preview for python-cpython-preview canceled.

zooba commented Dec 9, 2022

miss-islington commented Dec 9, 2022

bedevere-bot commented Dec 9, 2022

bedevere-bot commented Dec 9, 2022

zooba commented Dec 9, 2022

shreyanavigyan commented May 1, 2021 •

edited

Loading

shreyanavigyan May 8, 2021 •

edited

Loading

netlify bot commented Dec 8, 2022 •

edited

Loading