[PATCH 1/2] advapi32/tests: Fix a couple of leaks (Valgrind).

[Matteo Bruni]

Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com>
---
I don't know that we care a lot about the leak fixed in the first
hunk. If we go with it, the LocalFree() ideally should be removed when
we'll get rid of the todo_wine.

For bug 43310.

 dlls/advapi32/tests/security.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
index 51dcf90851..769328be81 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -6570,6 +6570,7 @@ static void test_system_security_access(void)
     /* privilege is checked on access */
     err = GetSecurityInfo( hkey, SE_REGISTRY_KEY, SACL_SECURITY_INFORMATION, NULL, NULL, NULL, &sacl, &sd );
     todo_wine ok( err == ERROR_PRIVILEGE_NOT_HELD, "got %u\n", err );
+    LocalFree( sd );
 
     priv.PrivilegeCount = 1;
     priv.Privileges[0].Luid = luid;
@@ -7082,6 +7083,7 @@ static void test_token_security_descriptor(void)
     CloseHandle(info.hThread);
 
     LocalFree(acl_child);
+    HeapFree(GetProcessHeap(), 0, sd2);
     LocalFree(psid);
 
     CloseHandle(token3);
-- 
2.13.6

[PATCH 2/2] advapi32/tests: Fully initialize ACLs (Valgrind).

[Matteo Bruni]

Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com>
---
I agree with Sebastian (bug 38671 comment 1) in that it seems
harmless. Let's make Valgrind happy anyway...

For bugs 38671, 43308, 43309.

 dlls/advapi32/tests/security.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
index 769328be81..d5e898ca8a 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -3559,7 +3559,7 @@ static void test_CreateDirectoryA(void)
     sa.bInheritHandle = TRUE;
     InitializeSecurityDescriptor(pSD, SECURITY_DESCRIPTOR_REVISION);
     pCreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, admin_sid, &sid_size);
-    pDacl = HeapAlloc(GetProcessHeap(), 0, 100);
+    pDacl = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 100);
     bret = InitializeAcl(pDacl, 100, ACL_REVISION);
     ok(bret, "Failed to initialize ACL.\n");
     bret = pAddAccessAllowedAceEx(pDacl, ACL_REVISION, OBJECT_INHERIT_ACE|CONTAINER_INHERIT_ACE,
@@ -6306,6 +6306,7 @@ static void test_AddMandatoryAce(void)
     HeapFree(GetProcessHeap(), 0, sd2);
     CloseHandle(handle);
 
+    memset(buffer_acl, 0, sizeof(buffer_acl));
     ret = InitializeAcl(acl, 256, ACL_REVISION);
     ok(ret, "InitializeAcl failed with %u\n", GetLastError());
 
@@ -6793,6 +6794,7 @@ static void test_maximum_allowed(void)
 
     ret = InitializeSecurityDescriptor(sd, SECURITY_DESCRIPTOR_REVISION);
     ok(ret, "InitializeSecurityDescriptor failed with %u\n", GetLastError());
+    memset(buffer_acl, 0, sizeof(buffer_acl));
     ret = InitializeAcl(acl, 256, ACL_REVISION);
     ok(ret, "InitializeAcl failed with %u\n", GetLastError());
     ret = SetSecurityDescriptorDacl(sd, TRUE, acl, FALSE);
@@ -6922,6 +6924,7 @@ static void test_token_security_descriptor(void)
     ret = InitializeSecurityDescriptor(sd, SECURITY_DESCRIPTOR_REVISION);
     ok(ret, "InitializeSecurityDescriptor failed with error %u\n", GetLastError());
 
+    memset(buffer_acl, 0, sizeof(buffer_acl));
     ret = InitializeAcl(acl, 256, ACL_REVISION);
     ok(ret, "InitializeAcl failed with error %u\n", GetLastError());
 
-- 
2.13.6

[Thomas Faber]

On 2018-01-10 22:59, Matteo Bruni wrote:
> @@ -6570,6 +6570,7 @@ static void test_system_security_access(void)
>       /* privilege is checked on access */
>       err = GetSecurityInfo( hkey, SE_REGISTRY_KEY, SACL_SECURITY_INFORMATION, NULL, NULL, NULL, &sacl, &sd );
>       todo_wine ok( err == ERROR_PRIVILEGE_NOT_HELD, "got %u\n", err );
> +    LocalFree( sd );

It may be better to only do this if (!err). Otherwise you might
introduce heap corruption on Windows, since sd is uninitialized.



Best,
Thomas

[Matteo Bruni]

2018-01-10 23:22 GMT+01:00 Thomas Faber <thomas.faber(a)reactos.org>:
> On 2018-01-10 22:59, Matteo Bruni wrote:
> > @@ -6570,6 +6570,7 @@ static void test_system_security_access(void)
> >       /* privilege is checked on access */
> >       err = GetSecurityInfo( hkey, SE_REGISTRY_KEY,
> > SACL_SECURITY_INFORMATION, NULL, NULL, NULL, &sacl, &sd );
> >       todo_wine ok( err == ERROR_PRIVILEGE_NOT_HELD, "got %u\n", err );
> > +    LocalFree( sd );
>
> It may be better to only do this if (!err). Otherwise you might
> introduce heap corruption on Windows, since sd is uninitialized.

Eh, good point. I'll resend with that change.

[PATCH v2] advapi32/tests: Fix a couple of leaks (Valgrind).

[Matteo Bruni]

Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com>
---
v2: Don't call LocalFree() with an uninitialized argument.

Alternatively we could just ignore that Wine-only leak.

 dlls/advapi32/tests/security.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
index 51dcf90851..791aef1879 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -6570,6 +6570,8 @@ static void test_system_security_access(void)
     /* privilege is checked on access */
     err = GetSecurityInfo( hkey, SE_REGISTRY_KEY, SACL_SECURITY_INFORMATION, NULL, NULL, NULL, &sacl, &sd );
     todo_wine ok( err == ERROR_PRIVILEGE_NOT_HELD, "got %u\n", err );
+    if (err == ERROR_SUCCESS)
+        LocalFree( sd );
 
     priv.PrivilegeCount = 1;
     priv.Privileges[0].Luid = luid;
@@ -7082,6 +7084,7 @@ static void test_token_security_descriptor(void)
     CloseHandle(info.hThread);
 
     LocalFree(acl_child);
+    HeapFree(GetProcessHeap(), 0, sd2);
     LocalFree(psid);
 
     CloseHandle(token3);
-- 
2.13.6