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

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().

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/