-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
gh-142282 Fix winreg_QueryValueEx_impl to use retSize #142283
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not simply use |
||
| 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 | ||
|
|
||
| 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
https://learn.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regqueryvalueexw
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it even better to use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you are right - it will work properly. Moreover, I checked
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
|
||
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.
I think
itertools.cycle()can be used here.