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
>
> 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