[Bug 20317] New: Uninitialised memory reference in SetEntriesInAclW()

http://bugs.winehq.org/show_bug.cgi?id=20317

Summary: Uninitialised memory reference in SetEntriesInAclW()
           Product: Wine
           Version: 1.1.31
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Keywords: download, patch, source
          Severity: normal
          Priority: P2
         Component: advapi32
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]

Once you are past bug 20303 and bug 20315, the commands

cd dlls/advapi32/tests
/usr/local/valgrind-10896/bin/valgrind --trace-children=yes --track-origins=yes
 --workaround-gcc296-bugs=yes ~/wine-git/wine advapi32_test.exe.so security.c

produce the valgrind warning

Conditional jump or move depends on uninitialised value(s)
   at RtlAllocateHeap (heap.c:1373)
   by HeapAlloc (heap.c:276)
   by GlobalAlloc (heap.c:361)
   by LocalAlloc (heap.c:961)
   by SetEntriesInAclW (security.c:3568)
   by test_SetEntriesInAcl (security.c:2583)
 Uninitialised value was created by a client request
   at mark_block_uninitialized (heap.c:187)
   by RtlAllocateHeap (heap.c:1429)
   by SetEntriesInAclW (security.c:3471)
   by test_SetEntriesInAcl (security.c:2583)

(so the amount of memory being allocated is undefined!)
It seems the ppsid memory block is not fully initialized,
since the change

--- a/dlls/advapi32/security.c
+++ b/dlls/advapi32/security.c
@@ -3468,7 +3468,7 @@ DWORD WINAPI SetEntriesInAclW( ULONG count,
PEXPLICIT_ACCESSW pEntries,
         return ERROR_SUCCESS;

/* allocate array of maximum sized sids allowed */
-    ppsid = HeapAlloc(GetProcessHeap(), 0, count * (sizeof(SID *) +
FIELD_OFFSET(SID, SubAuthority[SID_MAX_SUB_AUTHORITIES])));
+    ppsid = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, count * (sizeof(SID
*) + FIELD_OFFSET(SID, SubAuthority[SID_MAX_SUB_AUTHORITIES])));

makes the warning go away.

[Bug 20317] Uninitialised memory reference in SetEntriesInAclW()

http://bugs.winehq.org/show_bug.cgi?id=20317

--- Comment #1 from Dan Kegel   2009-10-10 22:22:23 ---
Created an attachment (id=24023)
 --> (http://bugs.winehq.org/attachment.cgi?id=24023)
Quick patch to work around missing initialization

[Bug 20317] Uninitialised memory reference in SetEntriesInAclW()

http://bugs.winehq.org/show_bug.cgi?id=20317

Dan Kegel  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #2 from Dan Kegel   2009-10-20 03:35:17 ---
Still there today. cc'ing Rob, who wrote this bit about two years ago (not sure
he's still interested, though.)

(BTW, you also need a simple patch from John Reiser to get current valgrind
to not complain on every heap allocation, see 
https://bugs.kde.org/show_bug.cgi?id=205541#c1 )

[Bug 20317] Uninitialised memory reference in SetEntriesInAclW()

http://bugs.winehq.org/show_bug.cgi?id=20317

--- Comment #3 from Rob Shearman   2009-10-21 09:03:15 ---
I'm guessing this is Valgrind warning for the same piece of code that I
commented on when the patch was sent and not fixed:
subject    Re: advapi32: SetEntriesInAclW() should accept account name
"CURRENT_USER".
mailed-by    gmail.com

hide details 18 Aug

2009/8/16 Rein Klazes :
> @@ -3510,7 +3511,8 @@ DWORD WINAPI SetEntriesInAclW( ULONG count, PEXPLICIT_ACCESSW pEntries,
>             DWORD sid_size = FIELD_OFFSET(SID, SubAuthority[SID_MAX_SUB_AUTHORITIES]);
>             DWORD domain_size = MAX_COMPUTERNAME_LENGTH + 1;
>             SID_NAME_USE use;
> -
>        if (!LookupAccountNameW(NULL, pEntries[i].Trustee.ptstrName, ppsid[i], &sid_size, NULL, &domain_size, &use))
> -
>        if ( strcmpW( pEntries[i].Trustee.ptstrName, CURRENT_USER ) &&

What will the memory pointed to by ppsid[i] be set to in this case?
Hint: whatever the memory was last used for, not the SID of the
current user which is what is desired.
> -
>                !LookupAccountNameW(NULL, pEntries[i].Trustee.ptstrName, ppsid[i], &sid_size, NULL, &domain_size, &use))
>       {
>           WARN("bad user name %s for trustee %d\n", debugstr_w(pEntries[i].Trustee.ptstrName), i);
>           ret = ERROR_INVALID_PARAMETER;

"git revert b46e2ef9b1d1349d4caf7f7a31ea1b6c348875ca" should also fix the issue
if the original author doesn't want to fix the code.

[Bug 20317] Uninitialised memory reference in SetEntriesInAclW()

http://bugs.winehq.org/show_bug.cgi?id=20317

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #4 from Alexandre Julliard   2009-10-22 10:41:18 ---
Should be fixed by 104a0f5439c941c5a052de28c40fd6613cdf99c1.

[Bug 20317] Uninitialised memory reference in SetEntriesInAclW()

http://bugs.winehq.org/show_bug.cgi?id=20317

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Alexandre Julliard   2009-10-23 13:19:33 ---
Closing bugs fixed in 1.1.32.