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

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);

______________________________________________________________________
TAKATSUKA Haruka harukat(at)sraoss(dot)co(dot)jp
SRA OSS, Inc http://www.sraoss.co.jp

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/

>> At present, a specific error, crash or trouble seems not to have
>> happened.

The reason its not crashing is that most, if not all, windows allocation
functions know which addresses belong to them. FreeSid is actually
documented as returning NULL on success. On failure it returns the
address you tried to free.

Although the FreeSid call causes no harm because its defensive, there is
still the issue of leaking the TOKEN_USER structure.

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

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/

>
> DWORD len = GetLengthSid(pTokenUser->User.Sid)
> *ppSidUser = (PSID) HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len);
> CopySid(len, *ppSidUser, pTokenUser->User.Sid);
>

I attached a patch for this. Although, I did not use CopySid. Instead,
I changed GetUserSid to GetTokenUser. AddUserToDacl() is the only
function making use of GetUserSid(), so this change won't break
anything. The benefit to this approach over my first suggestion is that
it avoids an unneeded HeapAlloc(sid), CopySid(sid) ... and its cleaner.

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

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

Our patches crossed. Although, in my patch I left the allocation scheme
alone since I wasn't sure if someone wanted that way. I'd suggest
malloc and free if your going to change it. The only time I use an MS
allocater is when a win32 api function specifically states it must be used.

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

Andrew Chernow wrote:
>
>>
>> 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).
>
> Our patches crossed. Although, in my patch I left the allocation scheme
> alone since I wasn't sure if someone wanted that way. I'd suggest
> malloc and free if your going to change it. The only time I use an MS
> allocater is when a win32 api function specifically states it must be used.

Attached is a mix of our two patches. How does that look to you?

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

>
> Attached is a mix of our two patches. How does that look to you?
>

looks good.

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

Andrew Chernow wrote:
>>
>> Attached is a mix of our two patches. How does that look to you?
>>
>
> looks good.

The next obvious question is, is this something we care to backpatch (at
this point, 8.4 is considered backpatch from that perspective), or
should we hold for 8.5?

The only issue now is a small leak of memory in a function that is only
called a fixed (and very small) number of times per process. Given this,
I'm inclined to say we should put it on hold for 8.5. Thoughts?

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

> The only issue now is a small leak of memory in a function that is only
> called a fixed (and very small) number of times per process. Given this,
> I'm inclined to say we should put it on hold for 8.5. Thoughts?
>
>

Doesn't sound urgent to me. If it were my decision, I'd punt it to 8.5.

Hard to keep that win32 acl stuff leak free. There is always a cleanup
goto because you need 6 billion objects to answer any question :o

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

Andrew Chernow wrote:
>
>> The only issue now is a small leak of memory in a function that is only
>> called a fixed (and very small) number of times per process. Given this,
>> I'm inclined to say we should put it on hold for 8.5. Thoughts?
>>
>>
>
> Doesn't sound urgent to me. If it were my decision, I'd punt it to 8.5.

Ok. Agreed then - added to the commitfest page.

> Hard to keep that win32 acl stuff leak free. There is always a cleanup
> goto because you need 6 billion objects to answer any question :o

Yeah, true.

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

On Wed, Jun 24, 2009 at 16:01, Magnus Hagander<magnus(at)hagander(dot)net> wrote:
> Andrew Chernow wrote:
>>
>>> The only issue now is a small leak of memory in a function that is only
>>> called a fixed (and very small) number of times per process. Given this,
>>> I'm inclined to say we should put it on hold for 8.5. Thoughts?
>>>
>>>
>>
>> Doesn't sound urgent to me.  If it were my decision, I'd punt it to 8.5.
>
> Ok. Agreed then - added to the commitfest page.

Applied to HEAD.

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