[PATCH 1/2] bcrypt/tests: Test return on invalid flag passed to BCryptCreateHash.
Signed-off-by: Brendan McGrath <brendan(a)redmandi.com> --- dlls/bcrypt/tests/bcrypt.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dlls/bcrypt/tests/bcrypt.c b/dlls/bcrypt/tests/bcrypt.c index 5c1a9dc8340..92b5a30e9fd 100644 --- a/dlls/bcrypt/tests/bcrypt.c +++ b/dlls/bcrypt/tests/bcrypt.c @@ -247,6 +247,12 @@ static void test_hash(const struct hash_test *test) hash = NULL; len = sizeof(buf); + + /* test return on invalid flag */ + ret = pBCryptCreateHash(alg, &hash, buf, len, NULL, 0, 1); + todo_wine + ok(ret == STATUS_INVALID_PARAMETER, "got %08x\n", ret); + ret = pBCryptCreateHash(alg, &hash, buf, len, NULL, 0, 0); ok(ret == STATUS_SUCCESS, "got %08x\n", ret); ok(hash != NULL, "hash not set\n"); -- 2.17.1
Signed-off-by: Brendan McGrath <brendan(a)redmandi.com> --- dlls/bcrypt/bcrypt_main.c | 2 +- dlls/bcrypt/tests/bcrypt.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/dlls/bcrypt/bcrypt_main.c b/dlls/bcrypt/bcrypt_main.c index 1b84ace8abe..4fc50b51664 100644 --- a/dlls/bcrypt/bcrypt_main.c +++ b/dlls/bcrypt/bcrypt_main.c @@ -602,7 +602,7 @@ NTSTATUS WINAPI BCryptCreateHash( BCRYPT_ALG_HANDLE algorithm, BCRYPT_HASH_HANDL if (flags) { FIXME( "unimplemented flags %08x\n", flags ); - return STATUS_NOT_IMPLEMENTED; + return STATUS_INVALID_PARAMETER; } if (!alg || alg->hdr.magic != MAGIC_ALG) return STATUS_INVALID_HANDLE; diff --git a/dlls/bcrypt/tests/bcrypt.c b/dlls/bcrypt/tests/bcrypt.c index 92b5a30e9fd..534b0326f2c 100644 --- a/dlls/bcrypt/tests/bcrypt.c +++ b/dlls/bcrypt/tests/bcrypt.c @@ -250,7 +250,6 @@ static void test_hash(const struct hash_test *test) /* test return on invalid flag */ ret = pBCryptCreateHash(alg, &hash, buf, len, NULL, 0, 1); - todo_wine ok(ret == STATUS_INVALID_PARAMETER, "got %08x\n", ret); ret = pBCryptCreateHash(alg, &hash, buf, len, NULL, 0, 0); -- 2.17.1
On Tue, 2019-03-05 at 19:52 +1100, Brendan McGrath wrote:
@@ -602,7 +602,7 @@ NTSTATUS WINAPI BCryptCreateHash( BCRYPT_ALG_HANDLE algorithm, BCRYPT_HASH_HANDL if (flags) { FIXME( "unimplemented flags %08x\n", flags ); - return STATUS_NOT_IMPLEMENTED; + return STATUS_INVALID_PARAMETER;
Now you're rejecting all flags while native accepts at least BCRYPT_HASH_REUSABLE_FLAG, according to the documentation.
This patch is only changing the return value (to be consistent with Windows). It looks like BCRYPT_HASH_REUSABLE_FLAG is yet to be implemented? On 5/3/19 8:05 pm, Hans Leidekker wrote:
On Tue, 2019-03-05 at 19:52 +1100, Brendan McGrath wrote:
@@ -602,7 +602,7 @@ NTSTATUS WINAPI BCryptCreateHash( BCRYPT_ALG_HANDLE algorithm, BCRYPT_HASH_HANDL if (flags) { FIXME( "unimplemented flags %08x\n", flags ); - return STATUS_NOT_IMPLEMENTED; + return STATUS_INVALID_PARAMETER; Now you're rejecting all flags while native accepts at least BCRYPT_HASH_REUSABLE_FLAG, according to the documentation.
On Tue, 2019-03-05 at 20:24 +1100, Brendan McGrath wrote:
This patch is only changing the return value (to be consistent with Windows). It looks like BCRYPT_HASH_REUSABLE_FLAG is yet to be implemented?
Right, but returning STATUS_INVALID_PARAMETER instead of STATUS_NOT_IMPLEMENTED when BCRYPT_HASH_REUSABLE_FLAG is passed is not an improvement.
OK - so it should return STATUS_NOT_IMPLEMENTED when BCRYPT_HASH_REUSABLE_FLAG is passed and STATUS_INVALID_PARAMETER otherwise? On 5/3/19 8:31 pm, Hans Leidekker wrote:
On Tue, 2019-03-05 at 20:24 +1100, Brendan McGrath wrote:
This patch is only changing the return value (to be consistent with Windows). It looks like BCRYPT_HASH_REUSABLE_FLAG is yet to be implemented? Right, but returning STATUS_INVALID_PARAMETER instead of STATUS_NOT_IMPLEMENTED when BCRYPT_HASH_REUSABLE_FLAG is passed is not an improvement.
On Tue, 2019-03-05 at 20:33 +1100, Brendan McGrath wrote:
OK - so it should return STATUS_NOT_IMPLEMENTED when BCRYPT_HASH_REUSABLE_FLAG is passed and STATUS_INVALID_PARAMETER otherwise?
Yes, though note that your test is not exhaustive, it checks just one flag while your fix rejects all of them. There might be undocumented flags, and there's always the possibility that new flags are added. It's very unlikely that applications depend on the exact error code, so it's not worth spending a lot of time getting these checks exactly right.
I just sent a patch to testbot - the patch tests using the BCRYPT_HASH_REUSABLE_FLAG and expects a return code of STATUS_INVALID_PARAMETER: https://testbot.winehq.org/JobDetails.pl?Key=48735 This passes on everything but Windows 8 and 10. On Win8 and 10, BCRYPT_HASH_REUSABLE_FLAG is implemented - so the return code is STATUS_SUCCESS. My patch to the mailing list is based on a change I made to my local build to fix an issue while running .NET Core. I can see in the .NET Core code, that it passes BCRYPT_HASH_REUSABLE_FLAG to BCryptCreateHash and expects STATUS_INVALID_PARAMETER if the OS is pre-Win8 (I'm using Win7). That code can be seen here (it's under the MIT licence): https://github.com/dotnet/corefx/blob/master/src/Common/src/Internal/Cryptog... So until BCRYPT_HASH_REUSABLE_FLAG is implemented - maybe it is better to return STATUS_INVALID_PARAMETER? On 5/3/19 9:07 pm, Hans Leidekker wrote:
On Tue, 2019-03-05 at 20:33 +1100, Brendan McGrath wrote:
OK - so it should return STATUS_NOT_IMPLEMENTED when BCRYPT_HASH_REUSABLE_FLAG is passed and STATUS_INVALID_PARAMETER otherwise? Yes, though note that your test is not exhaustive, it checks just one flag while your fix rejects all of them. There might be undocumented flags, and there's always the possibility that new flags are added.
It's very unlikely that applications depend on the exact error code, so it's not worth spending a lot of time getting these checks exactly right.
On Tue, 2019-03-05 at 21:23 +1100, Brendan McGrath wrote:
I just sent a patch to testbot - the patch tests using the BCRYPT_HASH_REUSABLE_FLAG and expects a return code of STATUS_INVALID_PARAMETER: https://testbot.winehq.org/JobDetails.pl?Key=48735
This passes on everything but Windows 8 and 10. On Win8 and 10, BCRYPT_HASH_REUSABLE_FLAG is implemented - so the return code is STATUS_SUCCESS.
My patch to the mailing list is based on a change I made to my local build to fix an issue while running .NET Core. I can see in the .NET Core code, that it passes BCRYPT_HASH_REUSABLE_FLAG to BCryptCreateHash and expects STATUS_INVALID_PARAMETER if the OS is pre-Win8 (I'm using Win7). That code can be seen here (it's under the MIT licence): https://github.com/dotnet/corefx/blob/master/src/Common/src/Internal/Cryptog...
So until BCRYPT_HASH_REUSABLE_FLAG is implemented - maybe it is better to return STATUS_INVALID_PARAMETER?
Sure, if you restrict it to BCRYPT_HASH_REUSABLE_FLAG. But implementing the flag would be even better, and doesn't look too hard.
On Tue, 2019-03-05 at 11:42 +0100, Hans Leidekker wrote:
On Tue, 2019-03-05 at 21:23 +1100, Brendan McGrath wrote:
My patch to the mailing list is based on a change I made to my local build to fix an issue while running .NET Core. I can see in the .NET Core code, that it passes BCRYPT_HASH_REUSABLE_FLAG to BCryptCreateHash and expects STATUS_INVALID_PARAMETER if the OS is pre-Win8 (I'm using Win7). That code can be seen here (it's under the MIT licence): https://github.com/dotnet/corefx/blob/master/src/Common/src/Internal/Cryptog...
So until BCRYPT_HASH_REUSABLE_FLAG is implemented - maybe it is better to return STATUS_INVALID_PARAMETER?
Sure, if you restrict it to BCRYPT_HASH_REUSABLE_FLAG. But implementing the flag would be even better, and doesn't look too hard.
Here's a draft patch, can you give it a try?
That seems to work well. Thanks Hans. On 5/3/19 10:39 pm, Hans Leidekker wrote:
On Tue, 2019-03-05 at 11:42 +0100, Hans Leidekker wrote:
On Tue, 2019-03-05 at 21:23 +1100, Brendan McGrath wrote:
My patch to the mailing list is based on a change I made to my local build to fix an issue while running .NET Core. I can see in the .NET Core code, that it passes BCRYPT_HASH_REUSABLE_FLAG to BCryptCreateHash and expects STATUS_INVALID_PARAMETER if the OS is pre-Win8 (I'm using Win7). That code can be seen here (it's under the MIT licence): https://github.com/dotnet/corefx/blob/master/src/Common/src/Internal/Cryptog...
So until BCRYPT_HASH_REUSABLE_FLAG is implemented - maybe it is better to return STATUS_INVALID_PARAMETER? Sure, if you restrict it to BCRYPT_HASH_REUSABLE_FLAG. But implementing the flag would be even better, and doesn't look too hard. Here's a draft patch, can you give it a try?
participants (2)
-
Brendan McGrath -
Hans Leidekker