Question about GetTokenInformation and memory leaks

[Dan Kegel]

I've never used the security apis, so I'm pretty unfamiliar with them.
Valgrinding chromium's sandbox_unittests.exe shows the leak

16 bytes in 1 blocks are definitely lost in loss record 123 of 728
   at RtlAllocateHeap (heap.c:1423)
   by RtlAllocateAndInitializeSid (sec.c:156)
   by NtQueryInformationToken (nt.c:379)
   by GetTokenInformation (security.c:676)
   by ATL::CAccessToken::GetInfoConvertATL::CSid,_TOKEN_PRIMARY_GROUP
(atlsecurity.h:754)
   by ATL::CAccessToken::GetPrimaryGroup (atlsecurity.inl:3623)
   by sandbox::RestrictedTokenTest_CustomInit_Test::TestBody
(restricted_token_unittest.cc:92)

The test in question is on line 92 of
http://src.chromium.org/viewvc/chrome/trunk/src/sandbox/src/restricted_token...

Can someone familiar with this stuff have a look?
I suspect it's a problem with the test, but
can't figure out what's going on.   I would
have thought the destructors for ATL::CAccessToken
and ATL::CSid would have freed everything.
(I think the code leaks token_handle, but fixing
that doesn't get rid of the reported leak.)
- Dan

[Rob Shearman]

2009/11/9 Dan Kegel :
> I've never used the security apis, so I'm pretty unfamiliar with them.
> Valgrinding chromium's sandbox_unittests.exe shows the leak
>
> 16 bytes in 1 blocks are definitely lost in loss record 123 of 728
>   at RtlAllocateHeap (heap.c:1423)
>   by RtlAllocateAndInitializeSid (sec.c:156)
>   by NtQueryInformationToken (nt.c:379)
>   by GetTokenInformation (security.c:676)
>   by ATL::CAccessToken::GetInfoConvertATL::CSid,_TOKEN_PRIMARY_GROUP
> (atlsecurity.h:754)
>   by ATL::CAccessToken::GetPrimaryGroup (atlsecurity.inl:3623)
>   by sandbox::RestrictedTokenTest_CustomInit_Test::TestBody
> (restricted_token_unittest.cc:92)
>
> The test in question is on line 92 of
> http://src.chromium.org/viewvc/chrome/trunk/src/sandbox/src/restricted_token...
>
> Can someone familiar with this stuff have a look?
> I suspect it's a problem with the test, but
> can't figure out what's going on.   I would
> have thought the destructors for ATL::CAccessToken
> and ATL::CSid would have freed everything.
> (I think the code leaks token_handle, but fixing
> that doesn't get rid of the reported leak.)

Hi Dan,

The bug is in the Wine code. Try the attached patch, which should fix
it as a side-effect.

Thanks,

[Dan Kegel]

On Mon, Nov 9, 2009 at 2:12 PM, Rob Shearman  wrote:
> 2009/11/9 Dan Kegel :
> > 16 bytes in 1 blocks are definitely lost in loss record 123 of 728
> >   at RtlAllocateHeap (heap.c:1423)
> >   by RtlAllocateAndInitializeSid (sec.c:156)
> >   by NtQueryInformationToken (nt.c:379)
> >   by GetTokenInformation (security.c:676)
>
> The bug is in the Wine code. Try the attached patch, which should fix
> it as a side-effect.

Works great, thanks.  Is that submitted / going to be submitted soon?
- Dan

[Rob Shearman]

2009/11/11 Dan Kegel :
> On Mon, Nov 9, 2009 at 2:12 PM, Rob Shearman  wrote:
> > 2009/11/9 Dan Kegel :
> > > 16 bytes in 1 blocks are definitely lost in loss record 123 of 728
> > >   at RtlAllocateHeap (heap.c:1423)
> > >   by RtlAllocateAndInitializeSid (sec.c:156)
> > >   by NtQueryInformationToken (nt.c:379)
> > >   by GetTokenInformation (security.c:676)
> >
> > The bug is in the Wine code. Try the attached patch, which should fix
> > it as a side-effect.
>
> Works great, thanks.  Is that submitted / going to be submitted soon?

Given the possibility of these changes breaking installers I plan to
submit it after the next release of Wine.

[Reece Dunn]

2009/11/9 Dan Kegel :
> I've never used the security apis, so I'm pretty unfamiliar with them.
> Valgrinding chromium's sandbox_unittests.exe shows the leak
>
> (I think the code leaks token_handle, but fixing
> that doesn't get rid of the reported leak.)

token_handle does not leak -- access_token is managing it via
`access_token.Attach(token_handle);` (when OpenProcessToken succeeds
and token_handle is not NULL).