Avoid use deprecated Windows Memory API
Hi.
According to:
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-localalloc
"Note The local functions have greater overhead and provide fewer features
than other memory management functions. New applications should use the
heap functions unless documentation states that a local function should be
used. For more information, see Global and Local Functions."
LocalAlloc is deprecated.
So use HeapAlloc instead, once LocalAlloc is an overhead wrapper to
HeapAlloc.
Attached a patch.
regards,
Ranier Vilela
Attachments:
use-heapalloc-instead-deprecated-localalloc.patchapplication/octet-stream; name=use-heapalloc-instead-deprecated-localalloc.patchDownload
diff --git a/src/common/exec.c b/src/common/exec.c
index 22f04aafbe..fe27403e5b 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -561,13 +561,23 @@ AddUserToTokenDacl(HANDLE hToken)
TOKEN_DEFAULT_DACL *ptdd = NULL;
TOKEN_INFORMATION_CLASS tic = TokenDefaultDacl;
BOOL ret = FALSE;
+ HANDLE hDefaultProcessHeap;
+
+ hDefaultProcessHeap = GetProcessHeap();
+ if (hDefaultProcessHeap == NULL)
+ {
+ log_error(errcode(ERRCODE_SYSTEM_ERROR),
+ "could not get default process heap: error code %lu",
+ GetLastError());
+ return FALSE;
+ }
/* Figure out the buffer size for the DACL info */
if (!GetTokenInformation(hToken, tic, (LPVOID) NULL, dwTokenInfoLength, &dwSize))
{
if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
{
- ptdd = (TOKEN_DEFAULT_DACL *) LocalAlloc(LPTR, dwSize);
+ ptdd = (TOKEN_DEFAULT_DACL *) HeapAlloc(hDefaultProcessHeap, 0, dwSize);
if (ptdd == NULL)
{
log_error(errcode(ERRCODE_OUT_OF_MEMORY),
@@ -612,7 +622,7 @@ AddUserToTokenDacl(HANDLE hToken)
GetLengthSid(pTokenUser->User.Sid) - sizeof(DWORD);
/* Allocate the ACL buffer & initialize it */
- pacl = (PACL) LocalAlloc(LPTR, dwNewAclSize);
+ pacl = (PACL) HeapAlloc(hDefaultProcessHeap, 0, dwNewAclSize);
if (pacl == NULL)
{
log_error(errcode(ERRCODE_OUT_OF_MEMORY),
@@ -669,13 +679,13 @@ AddUserToTokenDacl(HANDLE hToken)
cleanup:
if (pTokenUser)
- LocalFree((HLOCAL) pTokenUser);
+ HeapFree(hDefaultProcessHeap, 0, pTokenUser);
if (pacl)
- LocalFree((HLOCAL) pacl);
+ HeapFree(hDefaultProcessHeap, 0, pacl);
if (ptdd)
- LocalFree((HLOCAL) ptdd);
+ HeapFree(hDefaultProcessHeap, 0, ptdd);
return ret;
}
@@ -685,16 +695,26 @@ cleanup:
*
* Get the users token information from a process token.
*
- * The caller of this function is responsible for calling LocalFree() on the
+ * The caller of this function is responsible for calling HeapFree() on the
* returned TOKEN_USER memory.
*/
static BOOL
GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
{
DWORD dwLength;
+ HANDLE hDefaultProcessHeap;
*ppTokenUser = NULL;
+ hDefaultProcessHeap = GetProcessHeap();
+ if (hDefaultProcessHeap == NULL)
+ {
+ log_error(errcode(ERRCODE_SYSTEM_ERROR),
+ "could not get default process heap: error code %lu",
+ GetLastError());
+ return FALSE;
+ }
+
if (!GetTokenInformation(hToken,
TokenUser,
NULL,
@@ -703,7 +723,7 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
{
if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
{
- *ppTokenUser = (PTOKEN_USER) LocalAlloc(LPTR, dwLength);
+ *ppTokenUser = (PTOKEN_USER) HeapAlloc(hDefaultProcessHeap, 0, dwLength);
if (*ppTokenUser == NULL)
{
@@ -727,7 +747,7 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
dwLength,
&dwLength))
{
- LocalFree(*ppTokenUser);
+ HeapFree(hDefaultProcessHeap, 0, *ppTokenUser);
*ppTokenUser = NULL;
log_error(errcode(ERRCODE_SYSTEM_ERROR),
@@ -736,7 +756,7 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
return FALSE;
}
- /* Memory in *ppTokenUser is LocalFree():d by the caller */
+ /* Memory in *ppTokenUser is HeapFree():d by the caller */
return TRUE;
}
On 15 Sep 2022, at 01:19, Ranier Vilela <ranier.vf@gmail.com> wrote:
LocalAlloc is deprecated.
So use HeapAlloc instead, once LocalAlloc is an overhead wrapper to HeapAlloc.Attached a patch.
Don't forget that patches which aim to reduce overhead are best when
accompanied with benchmarks which show the effect of the reduction.
- pacl = (PACL) LocalAlloc(LPTR, dwNewAclSize);
+ pacl = (PACL) HeapAlloc(hDefaultProcessHeap, 0, dwNewAclSize);These calls are not equal, the LocalAlloc calls zeroes out the allocated memory
but the HeapAlloc does not unless the HEAP_ZERO_MEMORY flag is passed. I
haven't read the code enough to know if that matters, but it seems relevant to
at least discuss.
--
Daniel Gustafsson https://vmware.com/
Em qui., 15 de set. de 2022 às 05:35, Daniel Gustafsson <daniel@yesql.se>
escreveu:
On 15 Sep 2022, at 01:19, Ranier Vilela <ranier.vf@gmail.com> wrote:
LocalAlloc is deprecated.
So use HeapAlloc instead, once LocalAlloc is an overhead wrapper toHeapAlloc.
Attached a patch.
Don't forget that patches which aim to reduce overhead are best when
accompanied with benchmarks which show the effect of the reduction.
I'm trusting the API producer.
- pacl = (PACL) LocalAlloc(LPTR, dwNewAclSize); + pacl = (PACL) HeapAlloc(hDefaultProcessHeap, 0, dwNewAclSize);These calls are not equal, the LocalAlloc calls zeroes out the allocated
memory
but the HeapAlloc does not unless the HEAP_ZERO_MEMORY flag is passed. I
haven't read the code enough to know if that matters, but it seems
relevant to
at least discuss.
Yeah, I missed that.
But works fine and passes all tests.
If really ok, yet another improvement by avoiding useless padding.
CF entry created.
https://commitfest.postgresql.org/40/3893/
regards,
Ranier Vilela
On 2022-Sep-14, Ranier Vilela wrote:
According to:
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-localalloc
LocalAlloc is deprecated.
So use HeapAlloc instead, once LocalAlloc is an overhead wrapper to
HeapAlloc.
These functions you are patching are not in performance-sensitive code,
so I doubt this makes any difference performance wise. I doubt
Microsoft will ever remove these deprecated functions, given its history
of backwards compatibility, so from that perspective this change does
not achieve anything either.
If you were proposing to change how palloc() allocates memory, that
would be quite different and perhaps useful, as long as a benchmark
accompanies the patch.
.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Hi Ranier,
use HeapAlloc instead, once LocalAlloc is an overhead wrapper to HeapAlloc.
Thanks for the patch.
Although MSDN doesn't explicitly say that LocalAlloc is _depricated_
+1 for replacing it. I agree with Alvaro that it is unlikely to be
ever removed, but this is a trivial change, so let's keep the code a
bit cleaner. Also I agree that no benchmarking is required for this
patch since the code is not performance-sensitive.
These calls are not equal, the LocalAlloc calls zeroes out the allocated memory
I took a look. The memory is initialized below by InitializeAcl() /
GetTokenInformation() [1]https://docs.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-initializeacl[2]https://docs.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-gettokeninformation so it seems we are fine here.
The patch didn't have a proper commit message and had some issues with
the formating. I fixed this. The new code checks the return value of
GetProcessHeap() which is unlikely to fail, so I figured unlikely() is
appropriate:
+ hDefaultProcessHeap = GetProcessHeap();
+ if (unlikely(hDefaultProcessHeap == NULL))The corrected patch is attached.
[1]: https://docs.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-initializeacl
[2]: https://docs.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-gettokeninformation
--
Best regards,
Aleksander Alekseev
Attachments:
v2-0001-Replace-LocalAlloc-LocalFree-with-HeapAlloc-HeapF.patchapplication/octet-stream; name=v2-0001-Replace-LocalAlloc-LocalFree-with-HeapAlloc-HeapF.patchDownload
From 42730f3cdf643d0faf7293435e608f24340537a5 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Thu, 15 Sep 2022 15:35:50 +0300
Subject: [PATCH v2] Replace LocalAlloc/LocalFree with HeapAlloc/HeapFree
According to MSDN, new applications should use the Heap* functions unless
documentation states that a Local* function should be used. See:
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-localalloc
Although the use of Local* WinAPIs family doesn't seem to cause any particular
problems at present, it's trivial to replace them with Heap* family, so simply
do this.
Author: Ranier Vilela <ranier.vf@gmail.com>
Reviewed-by: Aleksander Alekseev <aleksander@timescale.com>
Discussion: https://postgr.es/m/CAEudQAo3etqZWk5tht-2TCgo6JLGAFonn1pbaWY2ioaNXiBNNA%40mail.gmail.com
---
src/common/exec.c | 38 +++++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/src/common/exec.c b/src/common/exec.c
index 22f04aafbe..967c70a81c 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -561,13 +561,23 @@ AddUserToTokenDacl(HANDLE hToken)
TOKEN_DEFAULT_DACL *ptdd = NULL;
TOKEN_INFORMATION_CLASS tic = TokenDefaultDacl;
BOOL ret = FALSE;
+ HANDLE hDefaultProcessHeap;
+
+ hDefaultProcessHeap = GetProcessHeap();
+ if (unlikely(hDefaultProcessHeap == NULL))
+ {
+ log_error(errcode(ERRCODE_SYSTEM_ERROR),
+ "could not get default process heap: error code %lu",
+ GetLastError());
+ return FALSE;
+ }
/* Figure out the buffer size for the DACL info */
if (!GetTokenInformation(hToken, tic, (LPVOID) NULL, dwTokenInfoLength, &dwSize))
{
if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
{
- ptdd = (TOKEN_DEFAULT_DACL *) LocalAlloc(LPTR, dwSize);
+ ptdd = (TOKEN_DEFAULT_DACL *) HeapAlloc(hDefaultProcessHeap, 0, dwSize);
if (ptdd == NULL)
{
log_error(errcode(ERRCODE_OUT_OF_MEMORY),
@@ -612,7 +622,7 @@ AddUserToTokenDacl(HANDLE hToken)
GetLengthSid(pTokenUser->User.Sid) - sizeof(DWORD);
/* Allocate the ACL buffer & initialize it */
- pacl = (PACL) LocalAlloc(LPTR, dwNewAclSize);
+ pacl = (PACL) HeapAlloc(hDefaultProcessHeap, 0, dwNewAclSize);
if (pacl == NULL)
{
log_error(errcode(ERRCODE_OUT_OF_MEMORY),
@@ -669,13 +679,13 @@ AddUserToTokenDacl(HANDLE hToken)
cleanup:
if (pTokenUser)
- LocalFree((HLOCAL) pTokenUser);
+ HeapFree(hDefaultProcessHeap, 0, pTokenUser);
if (pacl)
- LocalFree((HLOCAL) pacl);
+ HeapFree(hDefaultProcessHeap, 0, pacl);
if (ptdd)
- LocalFree((HLOCAL) ptdd);
+ HeapFree(hDefaultProcessHeap, 0, ptdd);
return ret;
}
@@ -685,16 +695,26 @@ cleanup:
*
* Get the users token information from a process token.
*
- * The caller of this function is responsible for calling LocalFree() on the
+ * The caller of this function is responsible for calling HeapFree() on the
* returned TOKEN_USER memory.
*/
static BOOL
GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
{
DWORD dwLength;
+ HANDLE hDefaultProcessHeap;
*ppTokenUser = NULL;
+ hDefaultProcessHeap = GetProcessHeap();
+ if (unlikely(hDefaultProcessHeap == NULL))
+ {
+ log_error(errcode(ERRCODE_SYSTEM_ERROR),
+ "could not get default process heap: error code %lu",
+ GetLastError());
+ return FALSE;
+ }
+
if (!GetTokenInformation(hToken,
TokenUser,
NULL,
@@ -703,7 +723,7 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
{
if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
{
- *ppTokenUser = (PTOKEN_USER) LocalAlloc(LPTR, dwLength);
+ *ppTokenUser = (PTOKEN_USER) HeapAlloc(hDefaultProcessHeap, 0, dwLength);
if (*ppTokenUser == NULL)
{
@@ -727,7 +747,7 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
dwLength,
&dwLength))
{
- LocalFree(*ppTokenUser);
+ HeapFree(hDefaultProcessHeap, 0, *ppTokenUser);
*ppTokenUser = NULL;
log_error(errcode(ERRCODE_SYSTEM_ERROR),
@@ -736,7 +756,7 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
return FALSE;
}
- /* Memory in *ppTokenUser is LocalFree():d by the caller */
+ /* Memory in *ppTokenUser is HeapFree():d by the caller */
return TRUE;
}
--
2.37.2
Em qui., 15 de set. de 2022 às 09:58, Aleksander Alekseev <
aleksander@timescale.com> escreveu:
Hi Ranier,
use HeapAlloc instead, once LocalAlloc is an overhead wrapper to
HeapAlloc.
Thanks for the patch.
Although MSDN doesn't explicitly say that LocalAlloc is _depricated_
+1 for replacing it.
The MSDN says:
" New applications should use the heap functions
<https://docs.microsoft.com/en-us/windows/desktop/Memory/heap-functions>".
IMO Postgres 16, fits here.
I agree with Alvaro that it is unlikely to be
ever removed, but this is a trivial change, so let's keep the code a
bit cleaner. Also I agree that no benchmarking is required for this
patch since the code is not performance-sensitive.
In no time, the patch claims performance.
So much so that the open CF is in refactoring.
These calls are not equal, the LocalAlloc calls zeroes out the allocated
memory
I took a look. The memory is initialized below by InitializeAcl() /
GetTokenInformation() [1][2] so it seems we are fine here.The patch didn't have a proper commit message and had some issues with
the formating. I fixed this. The new code checks the return value of
GetProcessHeap() which is unlikely to fail, so I figured unlikely() is
appropriate:+ hDefaultProcessHeap = GetProcessHeap(); + if (unlikely(hDefaultProcessHeap == NULL))The corrected patch is attached.
Thanks for the review and fixes.
regards,
Ranier Vilela
Em qui., 15 de set. de 2022 às 09:50, Alvaro Herrera <
alvherre@alvh.no-ip.org> escreveu:
On 2022-Sep-14, Ranier Vilela wrote:
According to:
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-localalloc
LocalAlloc is deprecated.
So use HeapAlloc instead, once LocalAlloc is an overhead wrapper to
HeapAlloc.These functions you are patching are not in performance-sensitive code,
so I doubt this makes any difference performance wise. I doubt
Microsoft will ever remove these deprecated functions, given its history
of backwards compatibility, so from that perspective this change does
not achieve anything either.
If users don't adapt to the new API, the old one will never really expire.
If you were proposing to change how palloc() allocates memory, that
would be quite different and perhaps useful, as long as a benchmark
accompanies the patch.
This is irrelevant to the discussion.
Neither the patch nor the thread deals with palloc.
regards,
Ranier Vilela
On 2022-Sep-15, Aleksander Alekseev wrote:
I agree with Alvaro that it is unlikely to be ever removed, but this
is a trivial change, so let's keep the code a bit cleaner.
In what way is this code cleaner? I argue it is the opposite.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Nunca confiaré en un traidor. Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)
On 2022-Sep-15, Ranier Vilela wrote:
Em qui., 15 de set. de 2022 às 09:50, Alvaro Herrera <
alvherre@alvh.no-ip.org> escreveu:
These functions you are patching are not in performance-sensitive code,
so I doubt this makes any difference performance wise. I doubt
Microsoft will ever remove these deprecated functions, given its history
of backwards compatibility, so from that perspective this change does
not achieve anything either.If users don't adapt to the new API, the old one will never really expire.
If you are claiming that Microsoft will remove the old API because
Postgres stopped using it, ... sorry, no.
Neither the patch nor the thread deals with palloc.
I know. Which is why I think the patch is useless.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Hi Alvaro,
In what way is this code cleaner? I argue it is the opposite.
Well, I guess it depends on the perspective. There are a bit more
lines of code for sure. So if "less code is better" is the criteria,
then no, the new code is not cleaner. If the criteria is to use an API
according to the spec provided by the vendor, to me personally the new
code looks cleaner.
This being said I don't have a strong opinion on whether this patch
should be accepted or not. I completely agree that MS will actually
keep LocalAlloc() indefinitely for backward compatibility with
existing applications, as the company always did.
--
Best regards,
Aleksander Alekseev
Em qui., 15 de set. de 2022 às 10:13, Alvaro Herrera <
alvherre@alvh.no-ip.org> escreveu:
On 2022-Sep-15, Ranier Vilela wrote:
Em qui., 15 de set. de 2022 às 09:50, Alvaro Herrera <
alvherre@alvh.no-ip.org> escreveu:These functions you are patching are not in performance-sensitive code,
so I doubt this makes any difference performance wise. I doubt
Microsoft will ever remove these deprecated functions, given itshistory
of backwards compatibility, so from that perspective this change does
not achieve anything either.If users don't adapt to the new API, the old one will never really
expire.
If you are claiming that Microsoft will remove the old API because
Postgres stopped using it, ... sorry, no.
Certainly not.
But Postgres should be an example.
By removing the old API, Postgres encourages others to do so.
So who knows, one day, the OS might finally get rid of this useless burden.
Neither the patch nor the thread deals with palloc.
I know. Which is why I think the patch is useless.
Other hackers think differently, thankfully.
regards,
Ranier Vilela
On 15 Sep 2022, at 15:13, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-Sep-15, Ranier Vilela wrote:
Em qui., 15 de set. de 2022 às 09:50, Alvaro Herrera <
alvherre@alvh.no-ip.org> escreveu:These functions you are patching are not in performance-sensitive code,
so I doubt this makes any difference performance wise. I doubt
Microsoft will ever remove these deprecated functions, given its history
of backwards compatibility, so from that perspective this change does
not achieve anything either.If users don't adapt to the new API, the old one will never really expire.
If you are claiming that Microsoft will remove the old API because
Postgres stopped using it, ... sorry, no.
Also, worth noting is that these functions aren't actually deprecated. The
note in the docs state:
The local functions have greater overhead and provide fewer features
than other memory management functions. New applications should use
the heap functions unless documentation states that a local function
should be used.
And following the bouncing ball into the documentation they refer to [0]https://docs.microsoft.com/en-us/windows/win32/memory/global-and-local-functions I read
this:
As a result, the global and local families of functions are equivalent
and choosing between them is a matter of personal preference.
--
Daniel Gustafsson https://vmware.com/
[0]: https://docs.microsoft.com/en-us/windows/win32/memory/global-and-local-functions
Em qui., 15 de set. de 2022 às 10:30, Daniel Gustafsson <daniel@yesql.se>
escreveu:
On 15 Sep 2022, at 15:13, Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:
On 2022-Sep-15, Ranier Vilela wrote:
Em qui., 15 de set. de 2022 às 09:50, Alvaro Herrera <
alvherre@alvh.no-ip.org> escreveu:These functions you are patching are not in performance-sensitive code,
so I doubt this makes any difference performance wise. I doubt
Microsoft will ever remove these deprecated functions, given itshistory
of backwards compatibility, so from that perspective this change does
not achieve anything either.If users don't adapt to the new API, the old one will never really
expire.
If you are claiming that Microsoft will remove the old API because
Postgres stopped using it, ... sorry, no.Also, worth noting is that these functions aren't actually deprecated. The
note in the docs state:
Daniel, I agree that MSDN could be better written.
See:
"Remarks
Windows memory management does not provide a separate local heap and global
heap. Therefore, the *LocalAlloc* and GlobalAlloc
<https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-globalalloc>
functions are essentially the same.
The movable-memory flags *LHND*, *LMEM_MOVABLE*, and *NONZEROLHND* add
unnecessary overhead and require locking to be used safely. They should be
avoided unless documentation specifically states that they should be used.
New applications should use the heap functions
<https://docs.microsoft.com/en-us/windows/desktop/Memory/heap-functions>"
Again, MSDN claims to use a new API.
And following the bouncing ball into the documentation they refer to [0] I
read
this:As a result, the global and local families of functions are
equivalent
and choosing between them is a matter of personal preference.
IMO, This part has nothing to do with it.
regards,
Ranier Vilela
Hi,
Again, MSDN claims to use a new API.
TWIMC the patch rotted a bit and now needs to be updated [1]http://cfbot.cputube.org/.
I changed its status to "Waiting on Author" [2]https://commitfest.postgresql.org/42/3893/.
[1]: http://cfbot.cputube.org/
[2]: https://commitfest.postgresql.org/42/3893/
--
Best regards,
Aleksander Alekseev
Em sex., 17 de mar. de 2023 às 10:58, Aleksander Alekseev <
aleksander@timescale.com> escreveu:
Hi,
Again, MSDN claims to use a new API.
TWIMC the patch rotted a bit and now needs to be updated [1].
I changed its status to "Waiting on Author" [2].
Rebased to latest Head.
best regards,
Ranier Vilela
Attachments:
v2-use-heapalloc-instead-deprecated-localalloc.patchapplication/octet-stream; name=v2-use-heapalloc-instead-deprecated-localalloc.patchDownload
diff --git a/src/common/exec.c b/src/common/exec.c
index 93b2faf2c4..ff59e0bf66 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -561,13 +561,23 @@ AddUserToTokenDacl(HANDLE hToken)
TOKEN_DEFAULT_DACL *ptdd = NULL;
TOKEN_INFORMATION_CLASS tic = TokenDefaultDacl;
BOOL ret = FALSE;
+ HANDLE hDefaultProcessHeap;
+
+ hDefaultProcessHeap = GetProcessHeap();
+ if (hDefaultProcessHeap == NULL)
+ {
+ log_error(errcode(ERRCODE_SYSTEM_ERROR),
+ "could not get default process heap: error code %lu",
+ GetLastError());
+ return FALSE;
+ }
/* Figure out the buffer size for the DACL info */
if (!GetTokenInformation(hToken, tic, (LPVOID) NULL, dwTokenInfoLength, &dwSize))
{
if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
{
- ptdd = (TOKEN_DEFAULT_DACL *) LocalAlloc(LPTR, dwSize);
+ ptdd = (TOKEN_DEFAULT_DACL *) HeapAlloc(hDefaultProcessHeap, 0, dwSize);
if (ptdd == NULL)
{
log_error(errcode(ERRCODE_OUT_OF_MEMORY),
@@ -612,7 +622,7 @@ AddUserToTokenDacl(HANDLE hToken)
GetLengthSid(pTokenUser->User.Sid) - sizeof(DWORD);
/* Allocate the ACL buffer & initialize it */
- pacl = (PACL) LocalAlloc(LPTR, dwNewAclSize);
+ pacl = (PACL) HeapAlloc(hDefaultProcessHeap, 0, dwNewAclSize);
if (pacl == NULL)
{
log_error(errcode(ERRCODE_OUT_OF_MEMORY),
@@ -669,13 +679,13 @@ AddUserToTokenDacl(HANDLE hToken)
cleanup:
if (pTokenUser)
- LocalFree((HLOCAL) pTokenUser);
+ HeapFree(hDefaultProcessHeap, 0, pTokenUser);
if (pacl)
- LocalFree((HLOCAL) pacl);
+ HeapFree(hDefaultProcessHeap, 0, pacl);
if (ptdd)
- LocalFree((HLOCAL) ptdd);
+ HeapFree(hDefaultProcessHeap, 0, ptdd);
return ret;
}
@@ -685,16 +695,26 @@ cleanup:
*
* Get the users token information from a process token.
*
- * The caller of this function is responsible for calling LocalFree() on the
+ * The caller of this function is responsible for calling HeapFree() on the
* returned TOKEN_USER memory.
*/
static BOOL
GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
{
DWORD dwLength;
+ HANDLE hDefaultProcessHeap;
*ppTokenUser = NULL;
+ hDefaultProcessHeap = GetProcessHeap();
+ if (hDefaultProcessHeap == NULL)
+ {
+ log_error(errcode(ERRCODE_SYSTEM_ERROR),
+ "could not get default process heap: error code %lu",
+ GetLastError());
+ return FALSE;
+ }
+
if (!GetTokenInformation(hToken,
TokenUser,
NULL,
@@ -703,7 +723,7 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
{
if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
{
- *ppTokenUser = (PTOKEN_USER) LocalAlloc(LPTR, dwLength);
+ *ppTokenUser = (PTOKEN_USER) HeapAlloc(hDefaultProcessHeap, 0, dwLength);
if (*ppTokenUser == NULL)
{
@@ -727,7 +747,7 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
dwLength,
&dwLength))
{
- LocalFree(*ppTokenUser);
+ HeapFree(hDefaultProcessHeap, 0, *ppTokenUser);
*ppTokenUser = NULL;
log_error(errcode(ERRCODE_SYSTEM_ERROR),
@@ -736,7 +756,7 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
return FALSE;
}
- /* Memory in *ppTokenUser is LocalFree():d by the caller */
+ /* Memory in *ppTokenUser is HeapFree():d by the caller */
return TRUE;
}
On Fri, Mar 17, 2023 at 12:19:56PM -0300, Ranier Vilela wrote:
Rebased to latest Head.
I was looking at this thread, and echoing Daniel's and Alvaro's
arguments, I don't quite see why this patch is something we need. I
would recommend to mark it as rejected and move on.
--
Michael
On 19 Mar 2023, at 23:41, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Mar 17, 2023 at 12:19:56PM -0300, Ranier Vilela wrote:
Rebased to latest Head.
I was looking at this thread, and echoing Daniel's and Alvaro's
arguments, I don't quite see why this patch is something we need. I
would recommend to mark it as rejected and move on.
Unless the claimed performance improvement is measured and provides a speedup,
and the loss of zeroing memory is guaranteed safe, there doesn't seem to be
much value provided.
--
Daniel Gustafsson
Em qua., 22 de mar. de 2023 às 07:01, Daniel Gustafsson <daniel@yesql.se>
escreveu:
On 19 Mar 2023, at 23:41, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Mar 17, 2023 at 12:19:56PM -0300, Ranier Vilela wrote:
Rebased to latest Head.
I was looking at this thread, and echoing Daniel's and Alvaro's
arguments, I don't quite see why this patch is something we need. I
would recommend to mark it as rejected and move on.Unless the claimed performance improvement is measured and provides a
speedup,
and the loss of zeroing memory is guaranteed safe, there doesn't seem to be
much value provided.
At no time was it suggested that there would be performance gains.
The patch proposes to adjust the API that the manufacturer asks you to use.
However, I see no point in discussing a defunct patch.
regards,
Ranier Vilela