Avoid handle leak (src/bin/pg_ctl/pg_ctl.c)
Hi.
Per Coverity.
The function *CreateRestrictedProcess* is responsible to create a
restricted token
Coverity complains that the handle origToken can be leaked.
In case of failure of the functions *AllocateAndInitializeSid* or
*GetPrivilegesToDelete*
the handle origToken must be released.
Trivial patch attached.
best regards,
Ranier Vilela
Attachments:
avoid-leak-handle-pg_ctl.patchapplication/octet-stream; name=avoid-leak-handle-pg_ctl.patchDownload
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 8a405ff122..04cbb6b28c 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -1811,14 +1811,19 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
{
write_stderr(_("%s: could not allocate SIDs: error code %lu\n"),
progname, (unsigned long) GetLastError());
+ CloseHandle(origToken);
return 0;
}
/* Get list of privileges to remove */
delPrivs = GetPrivilegesToDelete(origToken);
if (delPrivs == NULL)
+ {
+ CloseHandle(origToken);
+
/* Error message already printed */
return 0;
+ }
b = CreateRestrictedToken(origToken,
0,
On Thu, Oct 23, 2025 at 09:51:14PM -0300, Ranier Vilela wrote:
The function *CreateRestrictedProcess* is responsible to create a
restricted token
Coverity complains that the handle origToken can be leaked.In case of failure of the functions *AllocateAndInitializeSid* or
*GetPrivilegesToDelete*
the handle origToken must be released.
pg_ctl exits quickly when a failure of CreateRestrictedProcess()
happens, hence why does it matter to close these handles as an exit()
should do the job as well?
--
Michael
Em sex., 24 de out. de 2025 às 02:57, Michael Paquier <michael@paquier.xyz>
escreveu:
On Thu, Oct 23, 2025 at 09:51:14PM -0300, Ranier Vilela wrote:
The function *CreateRestrictedProcess* is responsible to create a
restricted token
Coverity complains that the handle origToken can be leaked.In case of failure of the functions *AllocateAndInitializeSid* or
*GetPrivilegesToDelete*
the handle origToken must be released.pg_ctl exits quickly when a failure of CreateRestrictedProcess()
happens, hence why does it matter to close these handles as an exit()
should do the job as well?
Handles are a scarce Windows resource.
The work of freeing these resources is not done by exit(), but by Windows
itself, when possible.
If applications are not good citizens, these resources will eventually run
out.
best regards,
Ranier Vilela
On 2025-Oct-24, Ranier Vilela wrote:
Handles are a scarce Windows resource.
The work of freeing these resources is not done by exit(), but by
Windows itself, when possible.
If applications are not good citizens, these resources will eventually
run out.
Hmm? That makes no sense. Do you have references to documentation that
says the system works the way you claim?
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Pensar que el espectro que vemos es ilusorio no lo despoja de espanto,
sólo le suma el nuevo terror de la locura" (Perelandra, C.S. Lewis)
Em sex., 24 de out. de 2025 às 09:24, Álvaro Herrera <alvherre@kurilemu.de>
escreveu:
On 2025-Oct-24, Ranier Vilela wrote:
Handles are a scarce Windows resource.
The work of freeing these resources is not done by exit(), but by
Windows itself, when possible.
If applications are not good citizens, these resources will eventually
run out.Hmm? That makes no sense. Do you have references to documentation that
says the system works the way you claim?
There are a bunch of messages on the web.
Some are:
"insufficient system resources exist to complete the requested service"
resource-exhaustion-detected-windows-11
<https://learn.microsoft.com/en-us/answers/questions/4019236/resource-exhaustion-detected-windows-11>
fatal-error-out-of-system-resources
<https://learn.microsoft.com/en-us/answers/questions/2683708/fatal-error-out-of-system-resources>
out-of-memory-error-windows/
<https://appuals.com/out-of-memory-error-windows/>
best regards,
Ranier Vilela
On 10/24/2025 7:40 AM, Ranier Vilela wrote:
Em sex., 24 de out. de 2025 às 09:24, Álvaro Herrera
<alvherre@kurilemu.de <mailto:alvherre@kurilemu.de>> escreveu:On 2025-Oct-24, Ranier Vilela wrote:
Handles are a scarce Windows resource.
The work of freeing these resources is not done by exit(), but by
Windows itself, when possible.
If applications are not good citizens, these resources willeventually
run out.
Hmm? That makes no sense. Do you have references to documentation that
says the system works the way you claim?There are a bunch of messages on the web.
Some are:
"insufficient system resources exist to complete the requested service"resource-exhaustion-detected-windows-11 <https://learn.microsoft.com/en-
us/answers/questions/4019236/resource-exhaustion-detected-windows-11>fatal-error-out-of-system-resources <https://learn.microsoft.com/en-us/
answers/questions/2683708/fatal-error-out-of-system-resources>out-of-memory-error-windows/ <https://appuals.com/out-of-memory-error-
windows/>best regards,
Ranier Vilela
Currently handles have ~10000 hard limit for a process. Your processes
handle table is allocated by the kernel in the non-paged pool of kernel
memory. The reason for the limit to handles is obvious, you can't have
something growing out of control in kernel memory, especially non-paged
pool memory. So, the bad part of a handle memory leak comes from
creating to many handles to the point that you fill up the process
handle table and can't create anymore. That can only happen while your
process is running. As soon as you call exit() the OS will get rid of
your processes handle table in the kernel and eradicate your address
space. The kernel object that your handle may have been "pointing"
(it's really just an index into your handle table) at may still exist if
it has been shared with another process--but that is a different handle
in a different handle table.
Currently this code is only called when running as a service. It will
quickly exit in the leaking path and the exit process does indeed
cleanup the handles. Handle leaks in userland only exist during the
life of the process. I can not imagine how this code would consume 10000
handles before exiting and the OS cleaning up.
Having said that, there are other things in this code that could use
cleaning up and I am not opposed to closing the handle in the right
spots being added during that cleanup. The PowerUser is no longer any
different than a normal user on any version of Windows we support. In
/messages/by-id/e4025275-0f97-4a3e-b107-a85e60ccf0f7@gmail.com
the patch I attached has a static HANDLE create_restricted_token(void)
function that does call CloseHandle(origToken) in the correct places, as
well as removing the PowerUser specific code. The removal of the
PowerUser and Closing the handle might be of more interest as a patch?
But, In the words of Raymond Chen
(https://devblogs.microsoft.com/oldnewthing/20120105-00/?p=8683), "All
this anal-rententive memory management is pointless. The process is
exiting. All that memory will be freed when the address space is
destroyed. Stop wasting time and just exit already."
BG
On 24 Oct 2025, at 14:40, Ranier Vilela <ranier.vf@gmail.com> wrote:
Em sex., 24 de out. de 2025 às 09:24, Álvaro Herrera <alvherre@kurilemu.de <mailto:alvherre@kurilemu.de>> escreveu:
Hmm? That makes no sense. Do you have references to documentation that
says the system works the way you claim?There are a bunch of messages on the web.
Some are:
"insufficient system resources exist to complete the requested service"
None of the these links back up the claim that exit() wont release resources
under Windows. The "Terminating a Process" article on MSDN does however have
this to say about the subject:
Terminating a process has the following results:
* Any remaining threads in the process are marked for termination.
* Any resources allocated by the process are freed.
* All kernel objects are closed.
* The process code is removed from memory.
* The process exit code is set.
* The process object is signaled.
--
Daniel Gustafsson