[PATCH v2 1/2] advapi32: Return ERROR_BADKEY from RegCreateKeyEx[AW] when appropriate
Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> --- Fixes Coverity #731771 --- dlls/advapi32/registry.c | 4 +++- dlls/advapi32/tests/registry.c | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/dlls/advapi32/registry.c b/dlls/advapi32/registry.c index 9989097ff4..0a8ccfbf20 100644 --- a/dlls/advapi32/registry.c +++ b/dlls/advapi32/registry.c @@ -371,6 +371,7 @@ LSTATUS WINAPI RegCreateKeyExW( HKEY hkey, LPCWSTR name, DWORD reserved, LPWSTR UNICODE_STRING nameW, classW; if (reserved) return ERROR_INVALID_PARAMETER; + if (!name || !retkey) return ERROR_BADKEY; if (!(hkey = get_special_root_hkey( hkey, access ))) return ERROR_INVALID_HANDLE; attr.Length = sizeof(attr); @@ -420,10 +421,11 @@ LSTATUS WINAPI RegCreateKeyExA( HKEY hkey, LPCSTR name, DWORD reserved, LPSTR cl NTSTATUS status; if (reserved) return ERROR_INVALID_PARAMETER; + if (!name || !retkey) return ERROR_BADKEY; if (!is_version_nt()) { access = MAXIMUM_ALLOWED; /* Win95 ignores the access mask */ - if (name && *name == '\\') name++; /* win9x,ME ignores one (and only one) beginning backslash */ + if (name[0] == '\\') name++; /* win9x,ME ignores one (and only one) beginning backslash */ } if (!(hkey = get_special_root_hkey( hkey, access ))) return ERROR_INVALID_HANDLE; diff --git a/dlls/advapi32/tests/registry.c b/dlls/advapi32/tests/registry.c index 6043cab184..ed14ae92b4 100644 --- a/dlls/advapi32/tests/registry.c +++ b/dlls/advapi32/tests/registry.c @@ -1297,6 +1297,30 @@ static void test_reg_create_key(void) PACL key_acl; SECURITY_DESCRIPTOR *sd; + ret = RegCreateKeyExA(INVALID_HANDLE_VALUE, NULL, 1, NULL, 0, 0, NULL, NULL, NULL); + ok(ret == ERROR_INVALID_PARAMETER, "RegCreateKeyExA returned %d\n", ret); + + ret = RegCreateKeyExA(INVALID_HANDLE_VALUE, NULL, 0, NULL, 0, 0, NULL, NULL, NULL); + ok(ret == ERROR_BADKEY, "RegCreateKeyExA returned %d\n", ret); + + ret = RegCreateKeyExA(INVALID_HANDLE_VALUE, NULL, 0, NULL, 0, 0, NULL, (HKEY *)0xdeadbeef, NULL); + ok(ret == ERROR_BADKEY, "RegCreateKeyExA returned %d\n", ret); + + ret = RegCreateKeyExA(INVALID_HANDLE_VALUE, (const char *)0xdeadbeef, 0, NULL, 0, 0, NULL, NULL, NULL); + ok(ret == ERROR_BADKEY, "RegCreateKeyExA returned %d\n", ret); + + ret = RegCreateKeyExW(INVALID_HANDLE_VALUE, NULL, 1, NULL, 0, 0, NULL, NULL, NULL); + ok(ret == ERROR_INVALID_PARAMETER, "RegCreateKeyExA returned %d\n", ret); + + ret = RegCreateKeyExW(INVALID_HANDLE_VALUE, NULL, 0, NULL, 0, 0, NULL, NULL, NULL); + ok(ret == ERROR_BADKEY, "RegCreateKeyExA returned %d\n", ret); + + ret = RegCreateKeyExW(INVALID_HANDLE_VALUE, NULL, 0, NULL, 0, 0, NULL, (HKEY *)0xdeadbeef, NULL); + ok(ret == ERROR_BADKEY, "RegCreateKeyExA returned %d\n", ret); + + ret = RegCreateKeyExW(INVALID_HANDLE_VALUE, (const WCHAR *)0xdeadbeef, 0, NULL, 0, 0, NULL, NULL, NULL); + ok(ret == ERROR_BADKEY, "RegCreateKeyExA returned %d\n", ret); + ret = RegCreateKeyExA(hkey_main, "Subkey1", 0, NULL, 0, KEY_NOTIFY, NULL, &hkey1, NULL); ok(!ret, "RegCreateKeyExA failed with error %d\n", ret); /* should succeed: all versions of Windows ignore the access rights -- 2.19.1
Signed-off-by: Alex Henrie <alexhenrie24(a)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; + if ((HandleToUlong(hkey) >= HandleToUlong(HKEY_SPECIAL_ROOT_FIRST)) && (HandleToUlong(hkey) <= HandleToUlong(HKEY_SPECIAL_ROOT_LAST))) { diff --git a/dlls/advapi32/tests/registry.c b/dlls/advapi32/tests/registry.c index ed14ae92b4..dae535253e 100644 --- a/dlls/advapi32/tests/registry.c +++ b/dlls/advapi32/tests/registry.c @@ -1309,6 +1309,9 @@ static void test_reg_create_key(void) ret = RegCreateKeyExA(INVALID_HANDLE_VALUE, (const char *)0xdeadbeef, 0, NULL, 0, 0, NULL, NULL, NULL); ok(ret == ERROR_BADKEY, "RegCreateKeyExA returned %d\n", ret); + ret = RegCreateKeyExA(INVALID_HANDLE_VALUE, (const char *)0xdeadbeef, 0, NULL, 0, 0, NULL, (HKEY *)0xdeadbeef, NULL); + ok(ret == ERROR_INVALID_HANDLE, "RegCreateKeyExA returned %d\n", ret); + ret = RegCreateKeyExW(INVALID_HANDLE_VALUE, NULL, 1, NULL, 0, 0, NULL, NULL, NULL); ok(ret == ERROR_INVALID_PARAMETER, "RegCreateKeyExA returned %d\n", ret); @@ -1321,6 +1324,9 @@ static void test_reg_create_key(void) ret = RegCreateKeyExW(INVALID_HANDLE_VALUE, (const WCHAR *)0xdeadbeef, 0, NULL, 0, 0, NULL, NULL, NULL); ok(ret == ERROR_BADKEY, "RegCreateKeyExA returned %d\n", ret); + ret = RegCreateKeyExW(INVALID_HANDLE_VALUE, (const WCHAR *)0xdeadbeef, 0, NULL, 0, 0, NULL, (HKEY *)0xdeadbeef, NULL); + ok(ret == ERROR_INVALID_HANDLE, "RegCreateKeyExA returned %d\n", ret); + ret = RegCreateKeyExA(hkey_main, "Subkey1", 0, NULL, 0, KEY_NOTIFY, NULL, &hkey1, NULL); ok(!ret, "RegCreateKeyExA failed with error %d\n", ret); /* should succeed: all versions of Windows ignore the access rights -- 2.19.1
Alex Henrie <alexhenrie24(a)gmail.com> writes:
Signed-off-by: Alex Henrie <alexhenrie24(a)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. -- Alexandre Julliard julliard(a)winehq.org
On Wed, Nov 7, 2018 at 9:11 AM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
Signed-off-by: Alex Henrie <alexhenrie24(a)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? -Alex
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Wed, Nov 7, 2018 at 9:11 AM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
Signed-off-by: Alex Henrie <alexhenrie24(a)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. -- Alexandre Julliard julliard(a)winehq.org
On Wed, Nov 7, 2018 at 11:57 PM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Wed, Nov 7, 2018 at 9:11 AM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
Signed-off-by: Alex Henrie <alexhenrie24(a)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? -Alex
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Wed, Nov 7, 2018 at 11:57 PM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Wed, Nov 7, 2018 at 9:11 AM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
Signed-off-by: Alex Henrie <alexhenrie24(a)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. -- Alexandre Julliard julliard(a)winehq.org
On Thu, Nov 8, 2018 at 9:49 AM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Wed, Nov 7, 2018 at 11:57 PM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Wed, Nov 7, 2018 at 9:11 AM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
Signed-off-by: Alex Henrie <alexhenrie24(a)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
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Thu, Nov 8, 2018 at 9:49 AM Alexandre Julliard <julliard(a)winehq.org> wrote:
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.
On the contrary, because the function is commonly used, if there was something wrong with it we'd most likely have heard about it by now. Also AFAICT Coverity is mistaken, there's nothing wrong here. -- Alexandre Julliard julliard(a)winehq.org
participants (2)
-
Alex Henrie -
Alexandre Julliard