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/
*** a/src/port/exec.c
--- b/src/port/exec.c
***************
*** 804,810 **** AddUserToDacl(HANDLE hProcess)
cleanup:
if (psidUser)
! FreeSid(psidUser);
if (pacl)
LocalFree((HLOCAL) pacl);
--- 804,810 ----
cleanup:
if (psidUser)
! LocalFree((HLOCAL) psidUser);
if (pacl)
LocalFree((HLOCAL) pacl);
***************
*** 822,827 **** cleanup:
--- 822,830 ----
* GetUserSid*PSID *ppSidUser, HANDLE hToken)
*
* Get the SID for the current user
+ *
+ * The caller of this function is responsible for calling LocalFree() on the
+ * returned SID area.
*/
static BOOL
GetUserSid(PSID *ppSidUser, HANDLE hToken)
***************
*** 838,844 **** GetUserSid(PSID *ppSidUser, HANDLE hToken)
{
if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
{
! pTokenUser = (PTOKEN_USER) HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength);
if (pTokenUser == NULL)
{
--- 841,847 ----
{
if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
{
! pTokenUser = (PTOKEN_USER) LocalAlloc(LPTR, dwLength);
if (pTokenUser == NULL)
{
***************
*** 859,872 **** GetUserSid(PSID *ppSidUser, HANDLE hToken)
dwLength,
&dwLength))
{
! HeapFree(GetProcessHeap(), 0, pTokenUser);
pTokenUser = NULL;
log_error("could not get token information: %lu", GetLastError());
return FALSE;
}
! *ppSidUser = pTokenUser->User.Sid;
return TRUE;
}
--- 862,895 ----
dwLength,
&dwLength))
{
! LocalFree(pTokenUser);
pTokenUser = NULL;
log_error("could not get token information: %lu", GetLastError());
return FALSE;
}
! /*
! * Need to copy the data to a separate buffer, so we can free our buffer
! * and let the caller free only the sid part.
! */
! dwLength = GetLengthSid(pTokenUser->User.Sid);
! *ppSidUser = (PSID) LocalAlloc(LPTR, dwLength);
! if (!*ppSidUser)
! {
! LocalFree(pTokenUser);
! log_error("could not allocate %lu bytes of memory", dwLength);
! return FALSE;
! }
! if (!CopySid(dwLength, *ppSidUser, pTokenUser->User.Sid))
! {
! LocalFree(*ppSidUser);
! LocalFree(pTokenUser);
! log_error("could not copy SID: %lu", GetLastError());
! return FALSE;
! }
!
! LocalFree(pTokenUser);
return TRUE;
}
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
