Re: GetTokenInformation() and FreeSid() at port/exec.c

Andrew Chernow wrote:
> TAKATSUKA Haruka wrote:
>> Hi.
>>
>> We found the unbalance of xxAlloc and xxFree at AddUserToDacl() in
>> src/port/exec.c (of current HEAD code).
>>
>> psidUser is a pointer of the element of a TOKEN_USER structure
>> allocated by  HeapAlloc().  The FreeSid() frees a SID allocated by
>> AllocateAndInitializeSid().  I think that it is correct to use
>> HeapFree(GetProcessHeap(), 0, pTokenUser).
>>
>> At present, a specific error, crash or trouble seems not to have
>> happened.
>>
>>
>> src/port/exec.c:748     AddUserToDacl()
>> src/port/exec.c:841      GetUserSid()
>>         pTokenUser = (PTOKEN_USER) HeapAlloc(GetProcessHeap(),
>> HEAP_ZERO_MEMORY, dwLength);
>>
>> src/port/exec.c:807      AddUserToDacl()
>>     FreeSid(psidUser);
> 
> I quickly poked around and found what I believe to be two memory issues.
> 
> 1. GetUserSid() uses HeapAlloc() to allocate a TOKEN_USER, but never
> calls HeapFree() if the function succeeds.  Instead, it pulls out the
> token's SID and returns it.  This is a memory leak.
> 
> 2. The SID returned by GetUserSid() is incorrectly being passed to
> FreeSid() within AddUserToDacl()'s cleanup section.  This memory belongs
> to the TOKEN_USER allocated by HeapAlloc() in GetUserSid(), it cannot be
> passed to FreeSid.
> 
> Quick question, Why HeapAlloc and LocalAlloc.  Why not use malloc?
> 
> One solution would be to return a copy of the SID from GetUserSid and
> HeapFree the TOKEN_USER.
> 
> Replace GetUserSid() line 869
> 
> *ppSidUser = pTokenUser->User.Sid;
> return TRUE;
> 
> With the below (error checking excluded)
> 
> DWORD len = GetLengthSid(pTokenUser->User.Sid)
> *ppSidUser = (PSID) HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len);
> CopySid(len, *ppSidUser, pTokenUser->User.Sid);
> 
> // SID is copied, free TOKEN_USER
> HeapFree(GetProcessHeap(), 0, pTokenUser);
> return TRUE;
> 
> Also, AddUserToDacl() line 807
> 
> FreeSid(psidUser) should be HeapFree(GetProcessHeap(), 0, psidUser)
> 
> in order to work with my suggested changes in GetUserSid().

How about something like this? I switched to using LocalAlloc() in all
places to be consistent, instead of mixing heap and local. (Though per
doc, LocalAlloc is actually a wrapper for HeapAlloc in win32).

-- 
 Magnus Hagander
 Self: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/