Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 40 additions & 0 deletions Lib/test/test_winreg.py
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,46 @@ def run(self):
DeleteKey(HKEY_CURRENT_USER, test_key_name+'\\changing_value')
DeleteKey(HKEY_CURRENT_USER, test_key_name)

def test_queryvalueex_race_condition(self):
# gh-142282: QueryValueEx could read garbage buffer under race
# condition when another thread changes the value size
done = False
error_found = None
values = [b'ham', b'spam']

class WriterThread(threading.Thread):
def run(self):
with CreateKey(HKEY_CURRENT_USER, test_key_name) as key:
use_first = True
while not done:
val = values[0] if use_first else values[1]
use_first = not use_first
Comment on lines +331 to +334
Copy link
Member

Choose a reason for hiding this comment

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

I think itertools.cycle() can be used here.

SetValueEx(key, 'test_value', 0, REG_BINARY, val)

thread = WriterThread()
thread.start()
try:
with CreateKey(HKEY_CURRENT_USER, test_key_name) as key:
for _ in range(1000):
try:
result, typ = QueryValueEx(key, 'test_value')
except FileNotFoundError:
# Value not yet created
continue
Comment on lines +341 to +346
Copy link
Member

Choose a reason for hiding this comment

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

Several first iterations can be a blank shot. In worst case this loop can finish before the writer thread starts. I the test is repeated several times, you can see here the result of the previous test before the writer thread starts.

You can use an event. Set it in the writer after the first write and wait for it in the main thread before starting the loop. Then read will never fail.

# The result must be one of the written values,
# not garbage data from uninitialized buffer
if result not in values:
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply use assertIn()?

error_found = result
break
finally:
done = True
thread.join()
DeleteKey(HKEY_CURRENT_USER, test_key_name)

if error_found is not None:
self.fail(f"QueryValueEx returned unexpected value: {error_found!r}, "
f"expected one of {values}")

def test_long_key(self):
# Issue2810, in 2.6 and 3.1 when the key name was exactly 256
# characters, EnumKey raised "WindowsError: More data is
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix :func:`winreg.QueryValueEx` to not accidentally read garbage buffer under race condition.
2 changes: 1 addition & 1 deletion PC/winreg.c
Original file line number Diff line number Diff line change
Expand Up @@ -1651,7 +1651,7 @@ winreg_QueryValueEx_impl(PyObject *module, HKEY key, const wchar_t *name)
return PyErr_SetFromWindowsErrWithFunction(rc,
"RegQueryValueEx");
}
obData = Reg2Py(retBuf, bufSize, typ);
obData = Reg2Py(retBuf, retSize, typ);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it is safe change according to the warning from the documentation:

If the value being queried is a string (REG_SZ, REG_MULTI_SZ, and REG_EXPAND_SZ) the value returned is NOT guaranteed to be null-terminated.

https://learn.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regqueryvalueexw

Copy link
Contributor Author

@youknowone youknowone Dec 5, 2025

Choose a reason for hiding this comment

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

Isn't it even better to use retSize in that case because retSize is the actual size of returned value and bufSize is not?
If the value is not guaranteed to be null-terminated, Reg2Py is able to actually access to the garbage memory beyond retSize

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you are right - it will work properly. Moreover, I checked Reg2Py and for REG_BINARY your fix will be work better than before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I would suggest adding a news entry.

PyMem_Free(retBuf);
if (obData == NULL)
return NULL;
Expand Down
Loading