Re: [v1 3/3] kernel32/tests: pipe: Added test for pipe security attributes

[Jacek Caban]

Hi Jonathan,

As I mentioned in the other mail, it would be interesting to add some
more tests. As for those tests, I have a few comments bellow.

On 12.09.2017 10:01, Jonathan Doron wrote:
> Signed-off-by: Jonathan Doron <jond(a)wizery.com>
> ---
>  dlls/kernel32/tests/pipe.c | 241 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 241 insertions(+)
>
> diff --git a/dlls/kernel32/tests/pipe.c b/dlls/kernel32/tests/pipe.c
> index 0a6edd6..9b90ce9 100644
> --- a/dlls/kernel32/tests/pipe.c
> +++ b/dlls/kernel32/tests/pipe.c
> @@ -28,6 +28,7 @@
>  #include "winternl.h"
>  #include "winioctl.h"
>  #include "wine/test.h"
> +#include "sddl.h"
>
> #define PIPENAME "\\\\.\\PiPe\\tests_pipe.c"
>
> @@ -3018,6 +3019,245 @@ static void test_overlapped_transport(BOOL msg_mode, BOOL msg_read_mode)
>      CloseHandle(client);
>  }
>
> +static PSECURITY_DESCRIPTOR GetObjectSecDesc(HANDLE Object)
> +{
> +    ULONG RequiredLength;
> +    PSECURITY_DESCRIPTOR SecDesc = NULL;
> +
> +    RequiredLength = 0;
> +    NtQuerySecurityObject(Object,
> +                          GROUP_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION,
> +                          NULL, 0, &RequiredLength);    // Expected c0000023
> +    if (!RequiredLength)
> +        goto cleanup;
> +
> +    SecDesc = LocalAlloc(LMEM_FIXED, RequiredLength);

Please use HeapAlloc().
> +    if (!SecDesc)
> +        goto cleanup;
> +    memset(SecDesc, 0, RequiredLength);
> +
> +    if (NtQuerySecurityObject(Object,
> +                              GROUP_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION,
> +                              SecDesc, RequiredLength, &RequiredLength)) {
> +        LocalFree(SecDesc);
> +        SecDesc = NULL;
> +        goto cleanup;
> +    }
> +
> +cleanup:
> +    return SecDesc;
> +}

Silent failures are not really good for tests. Please test
NtQuerySecurityObject result with ok(). Same for other similar cases.
> +static ULONG WINAPI ServerPipeThread(PVOID Param)
> +{
> +    ULONG RetVal;
> +
> +    RetVal = ConnectNamedPipe((HANDLE)Param, NULL) ? ERROR_SUCCESS : GetLastError();
> +    if ((RetVal == ERROR_PIPE_CONNECTED) || (RetVal == ERROR_SUCCESS))
> +        RetVal = ERROR_SUCCESS;
> +
> +    ExitThread(RetVal);
> +}

This is not really needed. You may just connect to file after it's
created, skipping new thread and ConnectNamedPipe() call.

Also there are styling issues. Please don't use C++ comments (//) in
Wine and try to match style of existing code (eg. function names,
variable names).

Thanks,
Jacek