gh-142282 Fix winreg_QueryValueEx_impl to use retSize #142283
Conversation
retSize was ignored bufSize is the size of buffer including uninitializd buffers.
| "RegQueryValueEx"); | ||
| } | ||
| obData = Reg2Py(retBuf, bufSize, typ); | ||
| obData = Reg2Py(retBuf, retSize, typ); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah, you are right - it will work properly. Moreover, I checked Reg2Py and for REG_BINARY your fix will be work better than before.
There was a problem hiding this comment.
Then I would suggest adding a news entry.
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?
| 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.
| 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()?
| 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.
4 participants
Uh oh!
There was an error while loading. Please reload this page.