Horton
Menu

Thread

  1. GetTokenInformation() and FreeSid() at port/exec.c

    TAKATSUKA Haruka <harukat@sraoss.co.jp> — 2009-06-23T01:36:39Z

    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@sraoss.co.jp
 SRA OSS, Inc  http://www.sraoss.co.jp

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

    Andrew Chernow <ac@esilo.com> — 2009-06-23T03:55:36Z

    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/

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

    Andrew Chernow <ac@esilo.com> — 2009-06-23T13:14:25Z

    >> 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/

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

    Magnus Hagander <magnus@hagander.net> — 2009-06-23T14:58:01Z

    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/

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

    Andrew Chernow <ac@esilo.com> — 2009-06-23T15:01:42Z

    > 
> 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/

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

    Andrew Chernow <ac@esilo.com> — 2009-06-23T15:07:06Z

    > 
> 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/

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

    Magnus Hagander <magnus@hagander.net> — 2009-06-24T12:45:54Z

    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/

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

    Andrew Chernow <ac@esilo.com> — 2009-06-24T13:48:22Z

    > 
> 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/

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

    Magnus Hagander <magnus@hagander.net> — 2009-06-24T13:50:50Z

    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/

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

    Andrew Chernow <ac@esilo.com> — 2009-06-24T13:57:23Z

    > 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/

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

    Magnus Hagander <magnus@hagander.net> — 2009-06-24T14:01:50Z

    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/

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

    Magnus Hagander <magnus@hagander.net> — 2009-07-27T08:46:34Z

    On Wed, Jun 24, 2009 at 16:01, Magnus Hagander<magnus@hagander.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/