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/