On Thu, Nov 8, 2018 at 9:49 AM Alexandre Julliard julliard@winehq.org wrote:
Alex Henrie alexhenrie24@gmail.com writes:
On Wed, Nov 7, 2018 at 11:57 PM Alexandre Julliard julliard@winehq.org wrote:
Alex Henrie alexhenrie24@gmail.com writes:
On Wed, Nov 7, 2018 at 9:11 AM Alexandre Julliard julliard@winehq.org wrote:
Alex Henrie alexhenrie24@gmail.com writes:
Signed-off-by: Alex Henrie alexhenrie24@gmail.com
dlls/advapi32/registry.c | 2 ++ dlls/advapi32/tests/registry.c | 6 ++++++ 2 files changed, 8 insertions(+)
diff --git a/dlls/advapi32/registry.c b/dlls/advapi32/registry.c index 0a8ccfbf20..4b33ade88c 100644 --- a/dlls/advapi32/registry.c +++ b/dlls/advapi32/registry.c @@ -313,6 +313,8 @@ static inline HKEY get_special_root_hkey( HKEY hkey, REGSAM access ) { HKEY ret = hkey;
- if (hkey == INVALID_HANDLE_VALUE) return 0;
There shouldn't be any reason to check for INVALID_HANDLE_VALUE, it doesn't have a special meaning. I expect you'll get the same behavior with any other invalid handle.
Is there a straightforward way to check whether the handle is valid?
You shouldn't need to add explicit checks, it should fall off from the normal function behavior. That's probably true for your other patch too.
Leaving aside the second patch for the moment, the tests show that RegCreateKeyEx[AW] checks the retkey parameter before checking the hkey parameter. If you don't want retkey to be explicitly checked at the beginning of RegCreateKeyEx[AW], how do you want the check to be done?
Is there an app that depends on this? Adding a lot of error checking only to satisfy artificial test cases is usually not desirable.
Considering how commonly this function is used and the fact that it has a Coverity warning, I think it's very likely that at least one application depends on at least checking the name parameter. The main reason why I added checks for retkey and hkey too was for completeness; if for some reason you think that these parameters should not be checked then I'm happy to submit a patch that does the minimum required to fix the Coverity warning.
-Alex