Re: [PATCH] Some tests for lsa

[Sebastian Lackner]

On 17.02.2017 00:29, Julius Schwartzenberg wrote:
> dlls/secur32/tests/Makefile.in |   1 +
>  dlls/secur32/tests/lsa.c       | 206 +++++++++++++++++++++++++++++++++++++++++
>  include/ntsecapi.h             |  50 ++++++++++
>  3 files changed, 257 insertions(+)
>  create mode 100644 dlls/secur32/tests/lsa.c

Thanks for providing the updated version. Unfortunately there are still various
problems left, see the comments below.

Patches should have a subject like "component: title". In this case something
like "secur32/tests: Add some tests for lsa functions." might be appropriate.
> diff --git a/dlls/secur32/tests/Makefile.in b/dlls/secur32/tests/Makefile.in
> index 089d1c3..d9f140a 100644
> --- a/dlls/secur32/tests/Makefile.in
> +++ b/dlls/secur32/tests/Makefile.in
> @@ -2,6 +2,7 @@ TESTDLL   = secur32.dll
>  IMPORTS   = secur32 crypt32 advapi32 ws2_32
>
> C_SRCS = \
> - lsa.c \
> main.c \
> negotiate.c \
> ntlm.c \
>
> diff --git a/dlls/secur32/tests/lsa.c b/dlls/secur32/tests/lsa.c
> new file mode 100644
> index 0000000..7668df1
> --- /dev/null
> +++ b/dlls/secur32/tests/lsa.c

It is preferred to add tests to existing files. You can still add your own
copyright of course. Splitting in separate files is usually done when there
are sufficient tests.
> @@ -0,0 +1,206 @@
> +/*
> -
> - Lsa tests
> -
> -
> -
> - Copyright 2017 Julius Schwartzenberg
> -
> -
> -
> - This library is free software; you can redistribute it and/or
> -
> - modify it under the terms of the GNU Lesser General Public
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> - */
> -
>
> +#define SECURITY_WIN32
> -
>
> +#include <stdio.h>
> +#include <ntstatus.h>
> +#define WIN32_NO_STATUS
> +#include <windows.h>
> +#include <ntsecapi.h>
> -
>
> +#include "wine/test.h"
> -
>
> +static void test_LsaConnectUntrusted_null_handle(void)
> +{
> - NTSTATUS status;
> -
> - status = LsaConnectUntrusted(NULL);
> -
> - todo_wine ok(status == STATUS_ACCESS_VIOLATION || status == RPC_NT_NULL_REF_POINTER,
> -
>   "LsaConnectUntrusted(NULL) returned 0x%x\n", status);
>
> +}
> -
>
> +static void test_LsaConnectUntrusted_correct_handle(void)

There is no need to add a separate function for each testcase. Its better to group
them, for example all tests for LsaConnectUntrusted in one function, and so on.
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> -

There are still some lines (see for example above) with trailing whitespace in your patch.
> - status = LsaConnectUntrusted(&lsa_handle);
> -
> - ok(status == STATUS_SUCCESS, "LsaConnectUntrusted(valid_handle) returned 0x%x\n", status);

You should try to avoid leaks and always close handles at the end of each test - in
this case with LsaDeregisterLogonProcess().
> +}
> -
>
> +static void test_LsaLookupAuthenticationPackage_null_handle(void)
> +{
> - NTSTATUS status;
> - LSA_STRING package_name;
> - ULONG auth_package;
> - char name[] = MICROSOFT_KERBEROS_NAME_A;
> - package_name.Buffer = name;
> - package_name.Length = sizeof(MICROSOFT_KERBEROS_NAME_A) - 1;
> -
> - status = LsaLookupAuthenticationPackage(NULL, &package_name, &auth_package);
> -
> - todo_wine ok(status == STATUS_INVALID_HANDLE || status == STATUS_INVALID_CONNECTION,

Please always try to document where you observed which status value. Depending on what you
consider more correct, you could mark the other value as broken() and a comment which Windows
versions are affected.
> -
>   "LsaLookupAuthenticationPackage(NULL, \"Kerberos\", ...) returned 0x%x\n", status);
>
> +}
> -
>
> +static void test_LsaLookupAuthenticationPackage_correct_handle(void)
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> - LSA_STRING package_name;
> - ULONG auth_package;
> - char name[] = MICROSOFT_KERBEROS_NAME_A;
> - package_name.Buffer = name;
> - package_name.Length = sizeof(MICROSOFT_KERBEROS_NAME_A) - 1;
> -
> - LsaConnectUntrusted(&lsa_handle);
> - status = LsaLookupAuthenticationPackage(lsa_handle, &package_name, &auth_package);
> - LsaDeregisterLogonProcess(&lsa_handle);

You should remove the & here. Also, you might want to check the result of each function,
even if they are also tested separately.
> -
> - ok(status == STATUS_SUCCESS,
> -
>  "LsaLookupAuthenticationPackage(valid_handle, \"Kerberos\", ...) returned 0x%x\n", status);

You might also want to test the auth_package output parameter here.
> +}
> -
>
> +static void test_LsaLookupAuthenticationPackage_correct_handle_wrong_name(void)
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> - LSA_STRING package_name;
> - ULONG auth_package;
> - package_name.Buffer = "this is probably not a valid authentication package name";

This triggers a compiler warning here:

lsa.c:90:25: error: assignment discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
     package_name.Buffer = "this is probably not a valid authentication package name";
> - package_name.Length = (USHORT)strlen(package_name.Buffer);

The explicit cast to USHORT should not be necessary here.
> -
> - LsaConnectUntrusted(&lsa_handle);
> - status = LsaLookupAuthenticationPackage(lsa_handle, &package_name, &auth_package);
> - LsaDeregisterLogonProcess(&lsa_handle);

Similar to above, the & is wrong here.
> -
> - todo_wine ok(status == STATUS_NO_SUCH_PACKAGE,
> -
>   "LsaLookupAuthenticationPackage(valid_handle, invalid, ...) returned 0x%x\n", status);
>
> +}
> -
>
> +static void test_LsaCallAuthenticationPackage_invalid_handle(void)
> +{
> - NTSTATUS status;
> - ULONG submit_buffer_length = 0;
> -
> - VOID *protocol_return_buffer = NULL;
> - ULONG *return_buffer_length = 0;
> - PNTSTATUS protocol_status = NULL;
> -
> - status = LsaCallAuthenticationPackage(
> -
>    NULL, 0xdeadbeef, NULL, submit_buffer_length,

You do not really need a separate variable just to pass 0 to a function.
> -
>    protocol_return_buffer, return_buffer_length, protocol_status

Those are supposed to be output pointers (&buffer, ...). Passing NULL is
probably also fine, but no need for separate variables then.
> -
>    );
> -
> - todo_wine ok(status == STATUS_INVALID_HANDLE || status == STATUS_INVALID_CONNECTION,
> -
>   "LsaCallAuthenticationPackage(NULL, ...) returned 0x%x\n", status);
>
> +}
> -
>
> +static void test_LsaCallAuthenticationPackage_invalid_package(void)
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> - ULONG submit_buffer_length = 0;
> -
> - VOID *protocol_return_buffer = NULL;
> - ULONG *return_buffer_length = 0;
> - PNTSTATUS protocol_status = NULL;
> -
> - LsaConnectUntrusted(&lsa_handle);
> - status = LsaCallAuthenticationPackage(
> -
>    lsa_handle, 0xdeadbeef, NULL, submit_buffer_length,
> -
>    protocol_return_buffer, return_buffer_length, protocol_status
> -
>    );
> - LsaDeregisterLogonProcess(&lsa_handle);
> -
> - todo_wine ok(status == STATUS_NO_SUCH_PACKAGE,
> -
>   "LsaCallAuthenticationPackage(valid_handle, invalid, ...) returned 0x%x\n", status);
>
> +}
> -
>
> +static void test_LsaCallAuthenticationPackage_null_submit_buffer(void)
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> - LSA_STRING package_name;
> - ULONG auth_package;
> - ULONG submit_buffer_length = 0;
> - VOID *protocol_return_buffer = NULL;
> - ULONG *return_buffer_length = 0;
> - PNTSTATUS protocol_status = NULL;
> - char name[] = MICROSOFT_KERBEROS_NAME_A;
> - package_name.Buffer = name;
> - package_name.Length = sizeof(MICROSOFT_KERBEROS_NAME_A) - 1;
> -
> - LsaConnectUntrusted(&lsa_handle);
> - LsaLookupAuthenticationPackage(lsa_handle, &package_name, &auth_package);
> - status = LsaCallAuthenticationPackage(
> -
>    lsa_handle, auth_package, NULL, submit_buffer_length,
> -
>    protocol_return_buffer, return_buffer_length, protocol_status
> -
>    );
> - LsaDeregisterLogonProcess(&lsa_handle);
> -
> - todo_wine ok(status == STATUS_INVALID_PARAMETER,
> -
>       "LsaCallAuthenticationPackage(...) returned 0x%x\n", status);
>
> +}
> -
>
> +static void test_LsaCallAuthenticationPackage_correct_submit_buffer(void)
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> - LSA_STRING package_name;
> - ULONG auth_package;
> - PKERB_RETRIEVE_TKT_REQUEST protocol_submit_buffer;
> - VOID     *protocol_return_buffer = NULL;
> - ULONG    *return_buffer_length = 0;
> - PNTSTATUS protocol_status = NULL;
> - char name[] = MICROSOFT_KERBEROS_NAME_A;
> - package_name.Buffer = name;
> - package_name.Length = sizeof(MICROSOFT_KERBEROS_NAME_A) - 1;
> -
> - LsaConnectUntrusted(&lsa_handle);
> - LsaLookupAuthenticationPackage(lsa_handle, &package_name, &auth_package);
> -
> - protocol_submit_buffer = LocalAlloc(LMEM_ZEROINIT, sizeof(KERB_RETRIEVE_TKT_REQUEST));
> - protocol_submit_buffer->MessageType = KerbRetrieveEncodedTicketMessage;

If you use memory allocations, please also free them at the end of the function.
Also, please note that it is usually preferred to use HeapAlloc(), unless the API
requires to use a very specific one.

In this case (fixed size struct) you could also consider allocating the memory
directly on the stack.
> -
> - status = LsaCallAuthenticationPackage(
> -
>    lsa_handle, auth_package, protocol_submit_buffer, sizeof(KERB_RETRIEVE_TKT_REQUEST),
> -
>    protocol_return_buffer, return_buffer_length, protocol_status
> -
>    );
> - LsaDeregisterLogonProcess(&lsa_handle);
> -
> - ok(status == STATUS_SUCCESS, "LsaCallAuthenticationPackage(...) returned 0x%x\n", status);
>
> +}
> -
>
> +START_TEST(lsa)
> +{
> - test_LsaConnectUntrusted_null_handle();
> - test_LsaConnectUntrusted_correct_handle();
> - test_LsaLookupAuthenticationPackage_null_handle();
> - test_LsaLookupAuthenticationPackage_correct_handle();
> - test_LsaLookupAuthenticationPackage_correct_handle_wrong_name();
> - test_LsaCallAuthenticationPackage_invalid_handle();
> - test_LsaCallAuthenticationPackage_invalid_package();
> - test_LsaCallAuthenticationPackage_null_submit_buffer();
> - test_LsaCallAuthenticationPackage_correct_submit_buffer();
>
> +}
> diff --git a/include/ntsecapi.h b/include/ntsecapi.h
> index 2bb3d31..e8a2af4 100644
> --- a/include/ntsecapi.h
> +++ b/include/ntsecapi.h
> @@ -387,6 +387,56 @@ NTSTATUS WINAPI LsaSetTrustedDomainInformation(LSA_HANDLE,PSID,TRUSTED_INFORMATI
>  NTSTATUS WINAPI LsaStorePrivateData(LSA_HANDLE,PLSA_UNICODE_STRING,PLSA_UNICODE_STRING);
>  NTSTATUS WINAPI LsaUnregisterPolicyChangeNotification(POLICY_NOTIFICATION_INFORMATION_CLASS,HANDLE);
>
> +#ifndef MICROSOFT_KERBEROS_NAME_A
> -
>
> +#define MICROSOFT_KERBEROS_NAME_A "Kerberos"
> +#define MICROSOFT_KERBEROS_NAME_W (const WCHAR []){'K','e','r','b','e','r','o','s',0}
> +#define MICROSOFT_KERBEROS_NAME MICROSOFT_KERBEROS_NAME_W
> +#endif
> -
>
> +typedef enum _KERB_PROTOCOL_MESSAGE_TYPE {
> - KerbDebugRequestMessage = 0,

You are using tabs here, but the file seems to use spaces.
> - KerbQueryTicketCacheMessage,
> - KerbChangeMachinePasswordMessage,
> - KerbVerifyPacMessage,
> - KerbRetrieveTicketMessage,
> - KerbUpdateAddressesMessage,
> - KerbPurgeTicketCacheMessage,
> - KerbChangePasswordMessage,
> - KerbRetrieveEncodedTicketMessage,
> - KerbDecryptDataMessage,
> - KerbAddBindingCacheEntryMessage,
> - KerbSetPasswordMessage,
> - KerbSetPasswordExMessage,
> - KerbVerifyCredentialsMessage,
> - KerbQueryTicketCacheExMessage,
> - KerbPurgeTicketCacheExMessage,
> - KerbRefreshSmartcardCredentialsMessage,
> - KerbAddExtraCredentialsMessage,
> - KerbQuerySupplementalCredentialsMessage,
> - KerbTransferCredentialsMessage,
> - KerbQueryTicketCacheEx2Message
>
> +} KERB_PROTOCOL_MESSAGE_TYPE,*PKERB_PROTOCOL_MESSAGE_TYPE;
> -
>
> +#ifndef __SECHANDLE_DEFINED__
> - typedef struct _SecHandle {
> -
>    ULONG_PTR dwLower;
> -
>    ULONG_PTR dwUpper;
> - } SecHandle,*PSecHandle;
> -
>
> +#define __SECHANDLE_DEFINED__
> +#endif
> -
>
> +typedef struct _KERB_RETRIEVE_TKT_REQUEST {
> - KERB_PROTOCOL_MESSAGE_TYPE MessageType;
> - LUID LogonId;
> - UNICODE_STRING TargetName;
> - ULONG TicketFlags;
> - ULONG CacheOptions;
> - LONG EncryptionType;
> - SecHandle CredentialsHandle;
>
> +} KERB_RETRIEVE_TKT_REQUEST,*PKERB_RETRIEVE_TKT_REQUEST;
> -
>
> #ifdef __cplusplus
>  } /* extern "C" */
>  #endif /* defined(__cplusplus) */
> -- 1.9.1

[PATCH] Some tests for lsa

[Julius Schwartzenberg]

Sebastian Lackner wrote:
> On 17.02.2017 00:29, Julius Schwartzenberg wrote:
> > dlls/secur32/tests/Makefile.in |   1 +
> >  dlls/secur32/tests/lsa.c       | 206 +++++++++++++++++++++++++++++++++++++++++
> >  include/ntsecapi.h             |  50 ++++++++++
> >  3 files changed, 257 insertions(+)
> >  create mode 100644 dlls/secur32/tests/lsa.c
>
> Thanks for providing the updated version. Unfortunately there are still various
> problems left, see the comments below.

Thanks a lot for the review!
> Patches should have a subject like "component: title". In this case something
> like "secur32/tests: Add some tests for lsa functions." might be appropriate.

Should I put that in the commit message? I think git will put it as the
subject as well then.
> It is preferred to add tests to existing files. You can still add your own
> copyright of course. Splitting in separate files is usually done when there
> are sufficient tests.

I don't care about copyright, but Bruno asked for that. I added the
files to this file because these functions are also defined in lsa.c and
this would be consistent with the other tests. Would an existing file be
more suitable?
> > +{
> > - NTSTATUS status;
> > - HANDLE lsa_handle;
> > -
>
> There are still some lines (see for example above) with trailing whitespace in your patch.

That's bad yeah, I should really take care of that!
> > - status = LsaConnectUntrusted(&lsa_handle);
> > -
> > - ok(status == STATUS_SUCCESS, "LsaConnectUntrusted(valid_handle) returned 0x%x\n", status);
>
> You should try to avoid leaks and always close handles at the end of each test - in
> this case with LsaDeregisterLogonProcess().

I missed this one yep.
> > +}
> > -
> >
> > +static void test_LsaLookupAuthenticationPackage_null_handle(void)
> > +{
> > - NTSTATUS status;
> > - LSA_STRING package_name;
> > - ULONG auth_package;
> > - char name[] = MICROSOFT_KERBEROS_NAME_A;
> > - package_name.Buffer = name;
> > - package_name.Length = sizeof(MICROSOFT_KERBEROS_NAME_A) - 1;
> > -
> > - status = LsaLookupAuthenticationPackage(NULL, &package_name, &auth_package);
> > -
> > - todo_wine ok(status == STATUS_INVALID_HANDLE || status == STATUS_INVALID_CONNECTION,
>
> Please always try to document where you observed which status value. Depending on what you
> consider more correct, you could mark the other value as broken() and a comment which Windows
> versions are affected.

The difference is between older and newer versions (<=6.0 and >=6.1). I
learned that from the testbot. It seems they made the messages more
precise in later versions. Should I just add a comment?
> > - LsaDeregisterLogonProcess(&lsa_handle);
>
> You should remove the & here. Also, you might want to check the result of each function,
> even if they are also tested separately.

This is something I still mix up from time to time. If there are any
recommendations on a good IDE which could point such things out, I'd be
glad to try it.
> > -
> > - ok(status == STATUS_SUCCESS,
> > -
> >  "LsaLookupAuthenticationPackage(valid_handle, \"Kerberos\", ...) returned 0x%x\n", status);
>
> You might also want to test the auth_package output parameter here.

Bruno suggested this also, but I hoped to add more tests for that in a
second patch. Maybe I should try to get at least a basic test going here
already though.

I intend to send a better patch soon. I am sending to wine-patches each
time, so that Marvin runs the tests, but correct me if I should send to
wine-devel first as long as it's still going through the review cycle.
Of course if others have remarks too, I'll be glad to incorporate any
feedback I get. Thanks!

Best regards,
Julius