Skip to content

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

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

Merged
zooba merged 18 commits into from
Dec 9, 2022
Merged

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1f2d523
Fix for bpo-43984
shreyanavigyan May 1, 2021
b00fc84
Update - Free retDataBuf before returning FALSE
shreyanavigyan May 2, 2021
7841714
Update winreg.c
shreyanavigyan May 2, 2021
1123340
Add news entry
shreyanavigyan May 2, 2021
7bbb3f2
Move the error check up before allocation
shreyanavigyan May 8, 2021
c5755dd
Update 2021-05-02-15-29-33.bpo-43984.U92jiv.rst
shreyanavigyan May 8, 2021
ad67138
Update PC/winreg.c
shreyanavigyan May 8, 2021
ec0ea5a
Update PC/winreg.c
shreyanavigyan May 8, 2021
62541b8
Add test
shreyanavigyan May 8, 2021
27b8130
Fix typo
shreyanavigyan May 8, 2021
41a70e0
Update 2021-05-02-15-29-33.bpo-43984.U92jiv.rst
shreyanavigyan May 8, 2021
26b1af4
Update Lib/test/test_winreg.py
shreyanavigyan May 8, 2021
3fe16ef
Update winreg.c
shreyanavigyan May 9, 2021
dbe8651
Add test case for REG_QWORD
shreyanavigyan May 9, 2021
1d5df04
Update 2021-05-02-15-29-33.bpo-43984.U92jiv.rst
shreyanavigyan May 9, 2021
666742c
Merge branch 'main' into bpo-43984
iritkatriel Dec 8, 2022
afdee9f
Fix whitespace
AlexWaygood Dec 9, 2022
06da229
Never edit on your phone
AlexWaygood Dec 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions Lib/test/test_winreg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ def _write_test_data(self, root_key, subkeystr="sub_key",
"does not close the actual key!")
except OSError:
pass

def _read_test_data(self, root_key, subkeystr="sub_key", OpenKey=OpenKey):
# Check we can get default value for this key.
val = QueryValue(root_key, test_key_name)
Expand Down Expand Up @@ -340,7 +339,23 @@ def test_setvalueex_value_range(self):
SetValueEx(ck, "test_name", None, REG_DWORD, 0x80000000)
finally:
DeleteKey(HKEY_CURRENT_USER, test_key_name)


Comment thread
AlexWaygood marked this conversation as resolved.
Outdated
def test_setvalueex_negative_one_check(self):
Comment thread
AlexWaygood marked this conversation as resolved.
# 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
Comment thread
shreyanavigyan marked this conversation as resolved.
Outdated
finally:
DeleteKey(HKEY_CURRENT_USER, test_key_name)
Comment on lines +343 to +357

@shreyanavigyan shreyanavigyan May 8, 2021

Copy link
Copy Markdown
Contributor Author

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?

@shreyanavigyan shreyanavigyan May 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

Comment thread
AlexWaygood marked this conversation as resolved.

Comment thread
AlexWaygood marked this conversation as resolved.
Outdated
def test_queryvalueex_return_value(self):
# Test for Issue #16759, return unsigned int from QueryValueEx.
# Reg2Py, which gets called by QueryValueEx, was returning a value
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Windows/2021-05-02-15-29-33.bpo-43984.U92jiv.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
:meth:`winreg.SetValueEx` now leaves the target register untouched in case of conversion errors.
Previously, ``-1`` would be written in case of such errors.

Comment thread
shreyanavigyan marked this conversation as resolved.
Outdated
50 changes: 26 additions & 24 deletions PC/winreg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,42 +564,46 @@ Py2Reg(PyObject *value, DWORD typ, BYTE **retDataBuf, DWORD *retDataSize)
{
Py_ssize_t i,j;
switch (typ) {
case REG_DWORD:
if (value != Py_None && !PyLong_Check(value))
case REG_DWORD: {
DWORD d = 0;
if (PyLong_Check(value)) {
d = PyLong_AsUnsignedLong(value);
if (d == (DWORD)(-1) && PyErr_Occurred()) {
return FALSE;
}
}
else if (value != Py_None) {
return FALSE;
}
*retDataBuf = (BYTE *)PyMem_NEW(DWORD, 1);
if (*retDataBuf == NULL){
if (*retDataBuf == NULL) {
PyErr_NoMemory();
return FALSE;
}
memcpy(*retDataBuf, &d, sizeof(DWORD));
*retDataSize = sizeof(DWORD);
if (value == Py_None) {
DWORD zero = 0;
memcpy(*retDataBuf, &zero, sizeof(DWORD));
}
else {
DWORD d = PyLong_AsUnsignedLong(value);
memcpy(*retDataBuf, &d, sizeof(DWORD));
}
break;
case REG_QWORD:
if (value != Py_None && !PyLong_Check(value))
}
case REG_QWORD: {
DWORD64 d = 0;
if (PyLong_Check(value)) {
d = PyLong_AsUnsignedLongLong(value);
if (d == (DWORD64)(-1) && PyErr_Occurred()) {
return FALSE;
Comment thread
shreyanavigyan marked this conversation as resolved.
}
}
else if (value != Py_None) {
return FALSE;
}
*retDataBuf = (BYTE *)PyMem_NEW(DWORD64, 1);
if (*retDataBuf == NULL){
if (*retDataBuf == NULL) {
PyErr_NoMemory();
return FALSE;
}
memcpy(*retDataBuf, &d, sizeof(DWORD64));
*retDataSize = sizeof(DWORD64);
if (value == Py_None) {
DWORD64 zero = 0;
memcpy(*retDataBuf, &zero, sizeof(DWORD64));
}
else {
DWORD64 d = PyLong_AsUnsignedLongLong(value);
memcpy(*retDataBuf, &d, sizeof(DWORD64));
}
break;
}
case REG_SZ:
case REG_EXPAND_SZ:
{
Expand Down Expand Up @@ -2116,5 +2120,3 @@ PyMODINIT_FUNC PyInit_winreg(void)
ADD_INT(REG_RESOURCE_REQUIREMENTS_LIST);
return m;
}