gh-142282 Fix winreg_QueryValueEx_impl to use retSize

[youknowone]

• Issue: winreg_QueryValueEx_impl doesn't use retSize #142282

[sergey-miryanov]

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

[youknowone]

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

[sergey-miryanov]

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

[sergey-miryanov]

Then I would suggest adding a news entry.

[serhiy-storchaka]

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.

[youknowone]

@serhiy-storchaka Isn't it also possible when rc == ERROR_MORE_DATA hits? That will be easier to check. Unfortunately I know barely nothing about windows api. How it will be?

[serhiy-storchaka]

I think that rc == ERROR_MORE_DATA only hits when there is a race condition. So, the code was already written to handle race condition, it just contains a bug, but since this was not tested, it was not discovered until your report.

[aisk]

Suggested change

	Fix winreg.QueryValueEx not to accidently read garbage buffer
	Fix :func:`winreg.QueryValueEx` to not accidentally read garbage buffer.