gh-142282 Fix winreg_QueryValueEx_impl to use retSize #142283
Conversation
retSize was ignored bufSize is the size of buffer including uninitializd buffers.
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
The current code works in most cases, because the size of the buffer is determined by the previous call of RegQueryValueExW(). But this will not work in case of race condition, when the value was change between the RegQueryValueExW() calls. Please mention race condition in the NEWS entry.
It would also be nice to add tests. Try to write different values (perhaps b'ham' and b'spam' would be enough) in one thread while simultaneously reading them in other thread and checking the the result is one of possible written value. If it fails with unpatched code and passes with patched code, then we have a working test. If this does not work, try something other. In worst case, if it is too difficult to reproduce the bug, we can accept the fix without working test.
|
@serhiy-storchaka Isn't it also possible when |
|
I think that |
| @@ -0,0 +1 @@ | |||
| Fix winreg.QueryValueEx not to accidently read garbage buffer | |||
There was a problem hiding this comment.
| Fix winreg.QueryValueEx not to accidently read garbage buffer | |
| Fix :func:`winreg.QueryValueEx` to not accidentally read garbage buffer. |
|
@serhiy-storchaka Thanks! It was reproducible with the test and I confirmed it is fixed after patch. |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Great!
How long does it take to complete the new test?
Lib/test/test_winreg.py
Outdated
| use_first = True | ||
| while not done: | ||
| val = values[0] if use_first else values[1] | ||
| use_first = not use_first |
There was a problem hiding this comment.
I think itertools.cycle() can be used here.
Lib/test/test_winreg.py
Outdated
| continue | ||
| # The result must be one of the written values, | ||
| # not garbage data from uninitialized buffer | ||
| if result not in values: |
There was a problem hiding this comment.
Why not simply use assertIn()?
Lib/test/test_winreg.py
Outdated
| for _ in range(1000): | ||
| try: | ||
| result, typ = QueryValueEx(key, 'test_value') | ||
| except FileNotFoundError: | ||
| # Value not yet created | ||
| continue |
There was a problem hiding this comment.
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.
|
Thanks, I applied the review. The test takes 0.005 seconds on my machine. |
serhiy-storchaka
added
needs backport to 3.13
|
Thanks @youknowone for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…nGH-142283) (cherry picked from commit 3ec941b) Co-authored-by: Jeong, YunWon <[email protected]>
|
Sorry, @youknowone and @serhiy-storchaka, I could not cleanly backport this to |
|
GH-142453 is a backport of this pull request to the 3.14 branch. |
…42283) (GH-142453) (cherry picked from commit 3ec941b) Co-authored-by: Jeong, YunWon <[email protected]>
…pythonGH-142283) (cherry picked from commit 3ec941b) Co-authored-by: Jeong, YunWon <[email protected]>
|
GH-142456 is a backport of this pull request to the 3.13 branch. |
|
I just noticed that the NEWS entry was misplaced. It should be in the "Library" section. I'll fix this in other issue, but please take a note for future. |
…42283) (GH-142456) (cherry picked from commit 3ec941b) Co-authored-by: Jeong, YunWon <[email protected]>
|
Oh, you're right. Sorry about that |
4 participants
Uh oh!
There was an error while loading. Please reload this page.