Re: GetTokenInformation() and FreeSid() at port/exec.c - Mailing list pgsql-bugs
From | Magnus Hagander |
---|---|
Subject | Re: GetTokenInformation() and FreeSid() at port/exec.c |
Date | |
Msg-id | 4A40ED79.7080409@hagander.net Whole thread Raw |
In response to | Re: GetTokenInformation() and FreeSid() at port/exec.c (Andrew Chernow <ac@esilo.com>) |
Responses |
Re: GetTokenInformation() and FreeSid() at port/exec.c
|
List | pgsql-bugs |
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; }
pgsql-bugs by date: