[PATCH] Windows port, fix some resources leaks
Hi, greetings everyone.
Continuing the process of improving windows port, I'm trying to fix some
leaks.
best regards,
Ranier Vilela
Attachments:
win_resource_leak.patchapplication/octet-stream; name=win_resource_leak.patchDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 511f891393..cf45cb882f 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1393,6 +1393,12 @@ pg_SSPI_recvauth(Port *port)
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("expected SSPI response, got message type %d",
mtype)));
+ if (sspictx != NULL)
+ {
+ DeleteSecurityContext(sspictx);
+ free(sspictx);
+ }
+ FreeCredentialsHandle(&sspicred);
return STATUS_ERROR;
}
@@ -1402,6 +1408,12 @@ pg_SSPI_recvauth(Port *port)
{
/* EOF - pq_getmessage already logged error */
pfree(buf.data);
+ if (sspictx != NULL)
+ {
+ DeleteSecurityContext(sspictx);
+ free(sspictx);
+ }
+ FreeCredentialsHandle(&sspicred);
return STATUS_ERROR;
}
@@ -2517,6 +2529,7 @@ InitializeLDAPConnection(Port *port, LDAP **ldap)
(errmsg("could not load function _ldap_start_tls_sA in wldap32.dll"),
errdetail("LDAP over SSL is not supported on this platform.")));
ldap_unbind(*ldap);
+ FreeLibrary(ldaphandle);
return STATUS_ERROR;
}
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index 30b07303ff..e8911a9999 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -576,6 +576,7 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
*/
elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
address, ShmemProtectiveRegion);
+ VirtualFreeEx(hChild, address, 0, MEM_RELEASE);
return false;
}
@@ -592,6 +593,7 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
{
elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
address, UsedShmemSegAddr);
+ VirtualFreeEx(hChild, address, 0, MEM_RELEASE);
return false;
}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 7a92dac525..8c10bbbabc 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4719,6 +4719,8 @@ retry:
if (cmdLine[sizeof(cmdLine) - 2] != '\0')
{
elog(LOG, "subprocess command line too long");
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1;
}
@@ -4735,6 +4737,8 @@ retry:
{
elog(LOG, "CreateProcess call failed: %m (error code %lu)",
GetLastError());
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1;
}
@@ -4750,6 +4754,8 @@ retry:
GetLastError())));
CloseHandle(pi.hProcess);
CloseHandle(pi.hThread);
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1; /* log made by save_backend_variables */
}
@@ -4839,6 +4845,7 @@ retry:
/* Don't close pi.hProcess here - the wait thread needs access to it */
CloseHandle(pi.hThread);
+ free(childinfo);
return pi.dwProcessId;
}
diff --git a/src/backend/regex/rege_dfa.c b/src/backend/regex/rege_dfa.c
index 5695e158a5..f62770ba37 100644
--- a/src/backend/regex/rege_dfa.c
+++ b/src/backend/regex/rege_dfa.c
@@ -530,6 +530,8 @@ freedfa(struct dfa *d)
if (d->mallocarea != NULL)
FREE(d->mallocarea);
+
+ FREE(d);
}
/*
diff --git a/src/backend/regex/regexec.c b/src/backend/regex/regexec.c
index f7eaa76b02..cba5ecafe7 100644
--- a/src/backend/regex/regexec.c
+++ b/src/backend/regex/regexec.c
@@ -389,7 +389,11 @@ find(struct vars *v,
/* first, a shot with the search RE */
s = newdfa(v, &v->g->search, cm, &v->dfa1);
assert(!(ISERR() && s != NULL));
- NOERR();
+ if (ISERR())
+ {
+ freedfa(s);
+ return v->err;
+ }
MDEBUG(("\nsearch at %ld\n", LOFF(v->start)));
cold = NULL;
close = shortest(v, s, v->search_start, v->search_start, v->stop,
@@ -473,7 +477,11 @@ cfind(struct vars *v,
int ret;
s = newdfa(v, &v->g->search, cm, &v->dfa1);
- NOERR();
+ if (ISERR())
+ {
+ freedfa(s);
+ return v->err;
+ }
d = newdfa(v, cnfa, cm, &v->dfa2);
if (ISERR())
{
diff --git a/src/common/logging.c b/src/common/logging.c
index c78ae793b8..982888eddb 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -78,11 +78,20 @@ pg_logging_init(const char *argv0)
value = e + 1;
if (strcmp(name, "error") == 0)
+ {
+ free(sgr_error);
sgr_error = strdup(value);
+ }
if (strcmp(name, "warning") == 0)
+ {
+ free(sgr_warning);
sgr_warning = strdup(value);
+ }
if (strcmp(name, "locus") == 0)
+ {
+ free(sgr_locus);
sgr_locus = strdup(value);
+ }
}
}
diff --git a/src/common/restricted_token.c b/src/common/restricted_token.c
index 74ba7192a1..15ba86d1a8 100644
--- a/src/common/restricted_token.c
+++ b/src/common/restricted_token.c
@@ -76,6 +76,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ALL_ACCESS, &origToken))
{
pg_log_error("could not open process token: error code %lu", GetLastError());
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -89,6 +90,8 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
0, &dropSids[1].Sid))
{
pg_log_error("could not allocate SIDs: error code %lu", GetLastError());
+ CloseHandle(origToken);
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -187,6 +190,7 @@ get_restricted_token(void)
}
exit(x);
}
+ free(cmdline);
}
#endif
}
On Sun, Jan 19, 2020 at 9:49 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
Continuing the process of improving windows port, I'm trying to fix some
leaks.
Some of the code this patch touches is not windows port only, so the
subject might be misleading reviewers.
It will be easier to review if you break this patch into smaller and
independent committable patches, as one per file.
Regards,
Juan José Santamaría Flecha
Em ter., 21 de jan. de 2020 às 06:18, Juan José Santamaría Flecha <
juanjo.santamaria@gmail.com> escreveu:
Some of the code this patch touches is not windows port only, so the
subject might be misleading reviewers.
True. Some leaks occurs at other platforms.
It will be easier to review if you break this patch into smaller and
independent committable patches, as one per file.
Done.
I separated the patch, one per file, to facilitate the review according to
your suggestion.
It looked like this:
1. /src/backend/postmaster/postmaster.c
In case of failure, it was necessary to deallocate the param pointer and
release the handle properly.
2. /src/backend/port/win32_shmem.c
In case of failure, the reserved memory can be released immediately, within
the function.
3. /src/common/restricted_token.c
If it is not possible to open the token, better release the dll, we may be
the only one to use it.
If it is not possible to allocate the SID, it was necessary to release the
handle and release the DLL properly.
The cmdline variable has yet to be released.
4. src / backend / regex / rege_dfa.c
The free_dfa function must free the entire structure, including itself.
5. src / backend / regex / regexec.c
The use of the NOERR () macro, hides the return, which causes the failure
to free the memory properly.
6. src / common / logging.c
The strdup function destroys the reference to the old pointer, in case of a
loop, it is necessary to release it beforehand.
The free function with variable NULL, has no effect and can be called
without problems.
7. /src/backend/libpq/auth.c
In case of failure, it was necessary to release the handlers properly.
regards,
Ranier Vilela
Attachments:
postmaster_resource_leak.patchapplication/octet-stream; name=postmaster_resource_leak.patchDownload
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 7a92dac525..8c10bbbabc 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4719,6 +4719,8 @@ retry:
if (cmdLine[sizeof(cmdLine) - 2] != '\0')
{
elog(LOG, "subprocess command line too long");
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1;
}
@@ -4735,6 +4737,8 @@ retry:
{
elog(LOG, "CreateProcess call failed: %m (error code %lu)",
GetLastError());
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1;
}
@@ -4750,6 +4754,8 @@ retry:
GetLastError())));
CloseHandle(pi.hProcess);
CloseHandle(pi.hThread);
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1; /* log made by save_backend_variables */
}
@@ -4839,6 +4845,7 @@ retry:
/* Don't close pi.hProcess here - the wait thread needs access to it */
CloseHandle(pi.hThread);
+ free(childinfo);
return pi.dwProcessId;
}win32_shmem_resource_leak.patchapplication/octet-stream; name=win32_shmem_resource_leak.patchDownload
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index 30b07303ff..e8911a9999 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -576,6 +576,7 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
*/
elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
address, ShmemProtectiveRegion);
+ VirtualFreeEx(hChild, address, 0, MEM_RELEASE);
return false;
}
@@ -592,6 +593,7 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
{
elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
address, UsedShmemSegAddr);
+ VirtualFreeEx(hChild, address, 0, MEM_RELEASE);
return false;
}restricted_token_resource_leak.patchapplication/octet-stream; name=restricted_token_resource_leak.patchDownload
diff --git a/src/common/restricted_token.c b/src/common/restricted_token.c
index 74ba7192a1..15ba86d1a8 100644
--- a/src/common/restricted_token.c
+++ b/src/common/restricted_token.c
@@ -76,6 +76,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ALL_ACCESS, &origToken))
{
pg_log_error("could not open process token: error code %lu", GetLastError());
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -89,6 +90,8 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
0, &dropSids[1].Sid))
{
pg_log_error("could not allocate SIDs: error code %lu", GetLastError());
+ CloseHandle(origToken);
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -187,6 +190,7 @@ get_restricted_token(void)
}
exit(x);
}
+ free(cmdline);
}
#endif
}regex_resource_leak.patchapplication/octet-stream; name=regex_resource_leak.patchDownload
diff --git a/src/backend/regex/rege_dfa.c b/src/backend/regex/rege_dfa.c
index 5695e158a5..f62770ba37 100644
--- a/src/backend/regex/rege_dfa.c
+++ b/src/backend/regex/rege_dfa.c
@@ -530,6 +530,8 @@ freedfa(struct dfa *d)
if (d->mallocarea != NULL)
FREE(d->mallocarea);
+
+ FREE(d);
}
/*
diff --git a/src/backend/regex/regexec.c b/src/backend/regex/regexec.c
index f7eaa76b02..cba5ecafe7 100644
--- a/src/backend/regex/regexec.c
+++ b/src/backend/regex/regexec.c
@@ -389,7 +389,11 @@ find(struct vars *v,
/* first, a shot with the search RE */
s = newdfa(v, &v->g->search, cm, &v->dfa1);
assert(!(ISERR() && s != NULL));
- NOERR();
+ if (ISERR())
+ {
+ freedfa(s);
+ return v->err;
+ }
MDEBUG(("\nsearch at %ld\n", LOFF(v->start)));
cold = NULL;
close = shortest(v, s, v->search_start, v->search_start, v->stop,
@@ -473,7 +477,11 @@ cfind(struct vars *v,
int ret;
s = newdfa(v, &v->g->search, cm, &v->dfa1);
- NOERR();
+ if (ISERR())
+ {
+ freedfa(s);
+ return v->err;
+ }
d = newdfa(v, cnfa, cm, &v->dfa2);
if (ISERR())
{logging_resource_leak.patchapplication/octet-stream; name=logging_resource_leak.patchDownload
diff --git a/src/common/logging.c b/src/common/logging.c
index c78ae793b8..982888eddb 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -78,11 +78,20 @@ pg_logging_init(const char *argv0)
value = e + 1;
if (strcmp(name, "error") == 0)
+ {
+ free(sgr_error);
sgr_error = strdup(value);
+ }
if (strcmp(name, "warning") == 0)
+ {
+ free(sgr_warning);
sgr_warning = strdup(value);
+ }
if (strcmp(name, "locus") == 0)
+ {
+ free(sgr_locus);
sgr_locus = strdup(value);
+ }
}
}auth_resource_leak.patchapplication/octet-stream; name=auth_resource_leak.patchDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 511f891393..cf45cb882f 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1393,6 +1393,12 @@ pg_SSPI_recvauth(Port *port)
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("expected SSPI response, got message type %d",
mtype)));
+ if (sspictx != NULL)
+ {
+ DeleteSecurityContext(sspictx);
+ free(sspictx);
+ }
+ FreeCredentialsHandle(&sspicred);
return STATUS_ERROR;
}
@@ -1402,6 +1408,12 @@ pg_SSPI_recvauth(Port *port)
{
/* EOF - pq_getmessage already logged error */
pfree(buf.data);
+ if (sspictx != NULL)
+ {
+ DeleteSecurityContext(sspictx);
+ free(sspictx);
+ }
+ FreeCredentialsHandle(&sspicred);
return STATUS_ERROR;
}
@@ -2517,6 +2529,7 @@ InitializeLDAPConnection(Port *port, LDAP **ldap)
(errmsg("could not load function _ldap_start_tls_sA in wldap32.dll"),
errdetail("LDAP over SSL is not supported on this platform.")));
ldap_unbind(*ldap);
+ FreeLibrary(ldaphandle);
return STATUS_ERROR;
}
On Tue, Jan 21, 2020 at 11:01:07AM -0300, Ranier Vilela wrote:
Done.
I would recommend that you also run all the regression tests present
in the source before sending a patch. If you don't know how to do
that, there is some documentation on the matter:
https://www.postgresql.org/docs/current/regress-run.html
If you send any patches, it is always important to make sure that
nothing you change breaks the existing coverage (on top of reading the
surrounding code and understanding its context, of course)
Hint: this crashes at initdb time.
--
Michael
Hi,
After review the patches and build all and run regress checks for each
patch, those are the ones that don't break.
Not all leaks detected by Coverity are fixed.
regards,
Ranier Vilela
Attachments:
auth_leak.patchapplication/octet-stream; name=auth_leak.patchDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 511f891393..cf45cb882f 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1393,6 +1393,12 @@ pg_SSPI_recvauth(Port *port)
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("expected SSPI response, got message type %d",
mtype)));
+ if (sspictx != NULL)
+ {
+ DeleteSecurityContext(sspictx);
+ free(sspictx);
+ }
+ FreeCredentialsHandle(&sspicred);
return STATUS_ERROR;
}
@@ -1402,6 +1408,12 @@ pg_SSPI_recvauth(Port *port)
{
/* EOF - pq_getmessage already logged error */
pfree(buf.data);
+ if (sspictx != NULL)
+ {
+ DeleteSecurityContext(sspictx);
+ free(sspictx);
+ }
+ FreeCredentialsHandle(&sspicred);
return STATUS_ERROR;
}
@@ -2517,6 +2529,7 @@ InitializeLDAPConnection(Port *port, LDAP **ldap)
(errmsg("could not load function _ldap_start_tls_sA in wldap32.dll"),
errdetail("LDAP over SSL is not supported on this platform.")));
ldap_unbind(*ldap);
+ FreeLibrary(ldaphandle);
return STATUS_ERROR;
}logging_leaks.patchapplication/octet-stream; name=logging_leaks.patchDownload
diff --git a/src/common/logging.c b/src/common/logging.c
index c78ae793b8..982888eddb 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -78,11 +78,20 @@ pg_logging_init(const char *argv0)
value = e + 1;
if (strcmp(name, "error") == 0)
+ {
+ free(sgr_error);
sgr_error = strdup(value);
+ }
if (strcmp(name, "warning") == 0)
+ {
+ free(sgr_warning);
sgr_warning = strdup(value);
+ }
if (strcmp(name, "locus") == 0)
+ {
+ free(sgr_locus);
sgr_locus = strdup(value);
+ }
}
}postmaster_leak.patchapplication/octet-stream; name=postmaster_leak.patchDownload
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 7a92dac525..b3986bee75 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4719,6 +4719,8 @@ retry:
if (cmdLine[sizeof(cmdLine) - 2] != '\0')
{
elog(LOG, "subprocess command line too long");
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1;
}
@@ -4735,6 +4737,8 @@ retry:
{
elog(LOG, "CreateProcess call failed: %m (error code %lu)",
GetLastError());
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1;
}
@@ -4750,6 +4754,8 @@ retry:
GetLastError())));
CloseHandle(pi.hProcess);
CloseHandle(pi.hThread);
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1; /* log made by save_backend_variables */
}regex_leak.patchapplication/octet-stream; name=regex_leak.patchDownload
diff --git a/src/backend/regex/regexec.c b/src/backend/regex/regexec.c
index f7eaa76b02..d3acf65329 100644
--- a/src/backend/regex/regexec.c
+++ b/src/backend/regex/regexec.c
@@ -389,7 +389,11 @@ find(struct vars *v,
/* first, a shot with the search RE */
s = newdfa(v, &v->g->search, cm, &v->dfa1);
assert(!(ISERR() && s != NULL));
- NOERR();
+ if (ISERR())
+ {
+ freedfa(s);
+ return v->err;
+ }
MDEBUG(("\nsearch at %ld\n", LOFF(v->start)));
cold = NULL;
close = shortest(v, s, v->search_start, v->search_start, v->stop,
@@ -473,7 +477,11 @@ cfind(struct vars *v,
int ret;
s = newdfa(v, &v->g->search, cm, &v->dfa1);
- NOERR();
+ if (ISERR())
+ {
+ freedfa(s);
+ return v->err;
+ }
d = newdfa(v, cnfa, cm, &v->dfa2);
if (ISERR())
{restricted_token_leaks.patchapplication/octet-stream; name=restricted_token_leaks.patchDownload
diff --git a/src/common/restricted_token.c b/src/common/restricted_token.c
index 74ba7192a1..15ba86d1a8 100644
--- a/src/common/restricted_token.c
+++ b/src/common/restricted_token.c
@@ -76,6 +76,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ALL_ACCESS, &origToken))
{
pg_log_error("could not open process token: error code %lu", GetLastError());
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -89,6 +90,8 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
0, &dropSids[1].Sid))
{
pg_log_error("could not allocate SIDs: error code %lu", GetLastError());
+ CloseHandle(origToken);
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -187,6 +190,7 @@ get_restricted_token(void)
}
exit(x);
}
+ free(cmdline);
}
#endif
}
win32_shmem_leak.patchapplication/octet-stream; name=win32_shmem_leak.patchDownload
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index 30b07303ff..39953c3de7 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -576,6 +576,9 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
*/
elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
address, ShmemProtectiveRegion);
+ if (VirtualFree(ShmemProtectiveRegion, 0, MEM_RELEASE) == 0)
+ elog(FATAL, "failed to release reserved memory region (addr=%p): error code %lu",
+ ShmemProtectiveRegion, GetLastError());
return false;
}
@@ -592,6 +595,9 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
{
elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
address, UsedShmemSegAddr);
+ if (VirtualFree(UsedShmemSegAddr, 0, MEM_RELEASE) == 0)
+ elog(FATAL, "failed to release reserved memory region (addr=%p): error code %lu",
+ UsedShmemSegAddr, GetLastError());
return false;
}On Wed, Jan 22, 2020 at 05:51:51PM -0300, Ranier Vilela wrote:
After review the patches and build all and run regress checks for each
patch, those are the ones that don't break.
There is some progress. You should be careful about your patches,
as they generate compiler warnings. Here is one quote from gcc-9:
logging.c:87:13: warning: passing argument 1 of ‘free’ discards
‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
87 | free(sgr_warning);
But there are others.
if (strcmp(name, "error") == 0)
+ {
+ free(sgr_error);
sgr_error = strdup(value);
+ }
I don't see the point of doing that in logging.c. pg_logging_init()
is called only once per tools, so this cannot happen. Another point
that may matter here though is that we do not complain about OOMs.
That's really unlikely to happen, and if it happens it leads to
partially colored output.
- NOERR();
+ if (ISERR())
+ {
+ freedfa(s);
+ return v->err;
+ }
Can you design a query where this is a problem? pg_log_error("could not allocate SIDs: error code %lu",
GetLastError());
+ CloseHandle(origToken);
+ FreeLibrary(Advapi32Handle);
[...]
pg_log_error("could not open process token: error code %lu",
GetLastError());
+ FreeLibrary(Advapi32Handle);
return 0;
For those two ones, it looks that you are right. However, I think
that it would be safer to check if Advapi32Handle is NULL for both.
@@ -187,6 +190,7 @@ get_restricted_token(void)
}
exit(x);
}
+ free(cmdline);
Anything allocated with pg_strdup() should be free'd with pg_free(),
that's a matter of consistency.
+++ b/src/backend/postmaster/postmaster.c
@@ -4719,6 +4719,8 @@ retry:
if (cmdLine[sizeof(cmdLine) - 2] != '\0')
{
elog(LOG, "subprocess command line too long");
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
The three ones in postmaster.c are correct guesses. + if (sspictx != NULL)
+ {
+ DeleteSecurityContext(sspictx);
+ free(sspictx);
+ }
+ FreeCredentialsHandle(&sspicred);
This stuff is correctly free'd after calling AcceptSecurityContext()
in the SSPI code, but not the two other code paths. Looks right.
Actually, for the first one, wouldn't it be better to free those
resources *before* ereport(ERROR) on ERRCODE_PROTOCOL_VIOLATION?
That's an authentication path so it does not really matter but.. ldap_unbind(*ldap);
+ FreeLibrary(ldaphandle);
return STATUS_ERROR;
Yep. That's consistent to clean up.
+ if (VirtualFree(ShmemProtectiveRegion, 0, MEM_RELEASE) == 0)
+ elog(FATAL, "failed to release reserved memory region
(addr=%p): error code %lu",
+ ShmemProtectiveRegion, GetLastError());
return false;
No, that's not right. I think that it is possible to loop over
ShmemProtectiveRegion in some cases. And actually, your patch is dead
wrong because this is some code called by the postmaster and it cannot
use FATAL.Not all leaks detected by Coverity are fixed.
Coverity is a static analyzer, it misses a lot of things tied to the
context of the code, so you need to take its suggestions with a pinch
of salt.
--
Michael
Em sex., 24 de jan. de 2020 às 04:13, Michael Paquier <michael@paquier.xyz>
escreveu:
On Wed, Jan 22, 2020 at 05:51:51PM -0300, Ranier Vilela wrote:
After review the patches and build all and run regress checks for each
patch, those are the ones that don't break.There is some progress. You should be careful about your patches,
as they generate compiler warnings. Here is one quote from gcc-9:
logging.c:87:13: warning: passing argument 1 of ‘free’ discards
‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
87 | free(sgr_warning);
Well, in this cases, the solution is cast.
free((char *) sgr_warning);
But there are others.
if (strcmp(name, "error") == 0)
+ {
+ free(sgr_error);
sgr_error = strdup(value);
+ }
I don't see the point of doing that in logging.c. pg_logging_init()
is called only once per tools, so this cannot happen. Another point
that may matter here though is that we do not complain about OOMs.
That's really unlikely to happen, and if it happens it leads to
partially colored output.
Coverity show the alert, because he tries all the possibilites.Is inside a
loop.
It seems to me that the only way to happen is by the user, by introducing a
repeated and wrong sequence.
If ok, we can discard this patch, but free doens't hurt here.
- NOERR(); + if (ISERR()) + { + freedfa(s); + return v->err; + } Can you design a query where this is a problem?
I think for now, I’m not able to do it.
But, the fix is better do not you think.
The macro hides the return and the exchange does not change the final size.
If the ISERR() it never occurs here, nor would we need the macro.
pg_log_error("could not allocate SIDs: error code %lu",
GetLastError());
+ CloseHandle(origToken);
+ FreeLibrary(Advapi32Handle);
[...]
pg_log_error("could not open process token: error code %lu",
GetLastError());
+ FreeLibrary(Advapi32Handle);
return 0;
For those two ones, it looks that you are right. However, I think
that it would be safer to check if Advapi32Handle is NULL for both.
Michael, I did it differently and modified the function to not need to test
NULL, I think it was better.
@@ -187,6 +190,7 @@ get_restricted_token(void)
}
exit(x);
}
+ free(cmdline);
Anything allocated with pg_strdup() should be free'd with pg_free(),
that's a matter of consistency.
Done.
+++ b/src/backend/postmaster/postmaster.c @@ -4719,6 +4719,8 @@ retry: if (cmdLine[sizeof(cmdLine) - 2] != '\0') { elog(LOG, "subprocess command line too long"); + UnmapViewOfFile(param); + CloseHandle(paramHandle); The three ones in postmaster.c are correct guesses.Does that mean it is correct?
+ if (sspictx != NULL) + { + DeleteSecurityContext(sspictx); + free(sspictx); + } + FreeCredentialsHandle(&sspicred); This stuff is correctly free'd after calling AcceptSecurityContext() in the SSPI code, but not the two other code paths. Looks right. Actually, for the first one, wouldn't it be better to free those resources *before* ereport(ERROR) on ERRCODE_PROTOCOL_VIOLATION? That's an authentication path so it does not really matter but..
Done.
ldap_unbind(*ldap);
+ FreeLibrary(ldaphandle);
return STATUS_ERROR;
Yep. That's consistent to clean up.
Ok.
+ if (VirtualFree(ShmemProtectiveRegion, 0, MEM_RELEASE) == 0) + elog(FATAL, "failed to release reserved memory region (addr=%p): error code %lu", + ShmemProtectiveRegion, GetLastError()); return false; No, that's not right. I think that it is possible to loop over ShmemProtectiveRegion in some cases. And actually, your patch is dead wrong because this is some code called by the postmaster and it cannot use FATAL.
FATAL changed to LOG, you are right.
In case of loop, VirtualAllocEx wouldn't be called again?
Not all leaks detected by Coverity are fixed.
Coverity is a static analyzer, it misses a lot of things tied to the
context of the code, so you need to take its suggestions with a pinch
of salt.
Oh yes, true.
I think that all alerts are true, because they test all possibilities, even
those that are rarely, or almost impossible to happen.
Thank you for the review.
Best regards,
Ranier Vilela
Attachments:
auth_leak.patchapplication/octet-stream; name=auth_leak.patchDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 511f891393..bd8c7f5811 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1387,6 +1387,13 @@ pg_SSPI_recvauth(Port *port)
mtype = pq_getbyte();
if (mtype != 'p')
{
+ if (sspictx != NULL)
+ {
+ DeleteSecurityContext(sspictx);
+ free(sspictx);
+ }
+ FreeCredentialsHandle(&sspicred);
+
/* Only log error if client didn't disconnect. */
if (mtype != EOF)
ereport(ERROR,
@@ -1402,6 +1409,12 @@ pg_SSPI_recvauth(Port *port)
{
/* EOF - pq_getmessage already logged error */
pfree(buf.data);
+ if (sspictx != NULL)
+ {
+ DeleteSecurityContext(sspictx);
+ free(sspictx);
+ }
+ FreeCredentialsHandle(&sspicred);
return STATUS_ERROR;
}
@@ -2517,6 +2530,7 @@ InitializeLDAPConnection(Port *port, LDAP **ldap)
(errmsg("could not load function _ldap_start_tls_sA in wldap32.dll"),
errdetail("LDAP over SSL is not supported on this platform.")));
ldap_unbind(*ldap);
+ FreeLibrary(ldaphandle);
return STATUS_ERROR;
}postmaster_leak.patchapplication/octet-stream; name=postmaster_leak.patchDownload
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 7a92dac525..b3986bee75 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4719,6 +4719,8 @@ retry:
if (cmdLine[sizeof(cmdLine) - 2] != '\0')
{
elog(LOG, "subprocess command line too long");
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1;
}
@@ -4735,6 +4737,8 @@ retry:
{
elog(LOG, "CreateProcess call failed: %m (error code %lu)",
GetLastError());
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1;
}
@@ -4750,6 +4754,8 @@ retry:
GetLastError())));
CloseHandle(pi.hProcess);
CloseHandle(pi.hThread);
+ UnmapViewOfFile(param);
+ CloseHandle(paramHandle);
return -1; /* log made by save_backend_variables */
}regex_leak.patchapplication/octet-stream; name=regex_leak.patchDownload
diff --git a/src/backend/regex/regexec.c b/src/backend/regex/regexec.c
index f7eaa76b02..d3acf65329 100644
--- a/src/backend/regex/regexec.c
+++ b/src/backend/regex/regexec.c
@@ -389,7 +389,11 @@ find(struct vars *v,
/* first, a shot with the search RE */
s = newdfa(v, &v->g->search, cm, &v->dfa1);
assert(!(ISERR() && s != NULL));
- NOERR();
+ if (ISERR())
+ {
+ freedfa(s);
+ return v->err;
+ }
MDEBUG(("\nsearch at %ld\n", LOFF(v->start)));
cold = NULL;
close = shortest(v, s, v->search_start, v->search_start, v->stop,
@@ -473,7 +477,11 @@ cfind(struct vars *v,
int ret;
s = newdfa(v, &v->g->search, cm, &v->dfa1);
- NOERR();
+ if (ISERR())
+ {
+ freedfa(s);
+ return v->err;
+ }
d = newdfa(v, cnfa, cm, &v->dfa2);
if (ISERR())
{restricted_token_leaks.patchapplication/octet-stream; name=restricted_token_leaks.patchDownload
diff --git a/src/common/restricted_token.c b/src/common/restricted_token.c
index 74ba7192a1..f5cbc1f3bd 100644
--- a/src/common/restricted_token.c
+++ b/src/common/restricted_token.c
@@ -52,23 +52,24 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
HANDLE restrictedToken;
SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
SID_AND_ATTRIBUTES dropSids[2];
- __CreateRestrictedToken _CreateRestrictedToken = NULL;
+ __CreateRestrictedToken _CreateRestrictedToken;
HANDLE Advapi32Handle;
ZeroMemory(&si, sizeof(si));
si.cb = sizeof(si);
Advapi32Handle = LoadLibrary("ADVAPI32.DLL");
- if (Advapi32Handle != NULL)
+ if (Advapi32Handle == NULL)
{
- _CreateRestrictedToken = (__CreateRestrictedToken) GetProcAddress(Advapi32Handle, "CreateRestrictedToken");
+ pg_log_warning("cannot load ADVAPI32.DLL");
+ return 0;
}
+ _CreateRestrictedToken = (__CreateRestrictedToken) GetProcAddress(Advapi32Handle, "CreateRestrictedToken");
if (_CreateRestrictedToken == NULL)
{
pg_log_warning("cannot create restricted tokens on this platform");
- if (Advapi32Handle != NULL)
- FreeLibrary(Advapi32Handle);
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -76,6 +77,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ALL_ACCESS, &origToken))
{
pg_log_error("could not open process token: error code %lu", GetLastError());
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -89,6 +91,8 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
0, &dropSids[1].Sid))
{
pg_log_error("could not allocate SIDs: error code %lu", GetLastError());
+ CloseHandle(origToken);
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -187,6 +191,7 @@ get_restricted_token(void)
}
exit(x);
}
+ pg_free(cmdline);
}
#endif
}
Last time improvement to restricted_token.c
regards,
Ranier Vilela
Attachments:
restricted_token_leaks.patchapplication/octet-stream; name=restricted_token_leaks.patchDownload
diff --git a/src/common/restricted_token.c b/src/common/restricted_token.c
index 74ba7192a1..e58b4d243f 100644
--- a/src/common/restricted_token.c
+++ b/src/common/restricted_token.c
@@ -52,23 +52,21 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
HANDLE restrictedToken;
SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
SID_AND_ATTRIBUTES dropSids[2];
- __CreateRestrictedToken _CreateRestrictedToken = NULL;
+ __CreateRestrictedToken _CreateRestrictedToken;
HANDLE Advapi32Handle;
- ZeroMemory(&si, sizeof(si));
- si.cb = sizeof(si);
-
Advapi32Handle = LoadLibrary("ADVAPI32.DLL");
- if (Advapi32Handle != NULL)
+ if (Advapi32Handle == NULL)
{
- _CreateRestrictedToken = (__CreateRestrictedToken) GetProcAddress(Advapi32Handle, "CreateRestrictedToken");
+ pg_log_warning("cannot load ADVAPI32.DLL");
+ return 0;
}
+ _CreateRestrictedToken = (__CreateRestrictedToken) GetProcAddress(Advapi32Handle, "CreateRestrictedToken");
if (_CreateRestrictedToken == NULL)
{
pg_log_warning("cannot create restricted tokens on this platform");
- if (Advapi32Handle != NULL)
- FreeLibrary(Advapi32Handle);
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -76,6 +74,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ALL_ACCESS, &origToken))
{
pg_log_error("could not open process token: error code %lu", GetLastError());
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -89,6 +88,8 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
0, &dropSids[1].Sid))
{
pg_log_error("could not allocate SIDs: error code %lu", GetLastError());
+ CloseHandle(origToken);
+ FreeLibrary(Advapi32Handle);
return 0;
}
@@ -115,6 +116,9 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo)
AddUserToTokenDacl(restrictedToken);
#endif
+ ZeroMemory(&si, sizeof(si));
+ si.cb = sizeof(si);
+
if (!CreateProcessAsUser(restrictedToken,
NULL,
cmd,
@@ -187,6 +191,7 @@ get_restricted_token(void)
}
exit(x);
}
+ pg_free(cmdline);
}
#endif
}
On Fri, Jan 24, 2020 at 09:37:25AM -0300, Ranier Vilela wrote:
Em sex., 24 de jan. de 2020 às 04:13, Michael Paquier <michael@paquier.xyz>
escreveu:There is some progress. You should be careful about your patches,
as they generate compiler warnings. Here is one quote from gcc-9:
logging.c:87:13: warning: passing argument 1 of ‘free’ discards
‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
87 | free(sgr_warning);Well, in this cases, the solution is cast.
free((char *) sgr_warning);
Applying blindly a cast is never a good practice.
if (strcmp(name, "error") == 0)
+ {
+ free(sgr_error);
sgr_error = strdup(value);
+ }
I don't see the point of doing that in logging.c. pg_logging_init()
is called only once per tools, so this cannot happen. Another point
that may matter here though is that we do not complain about OOMs.
That's really unlikely to happen, and if it happens it leads to
partially colored output.Coverity show the alert, because he tries all the possibilites.Is
inside a loop. It seems to me that the only way to happen is by the
user, by introducing a repeated and wrong sequence.
Again, Coverity may say something that does not apply to the reality,
and sometimes it misses some spots. Here we should be looking at
query patterns which involve a memory leak. So I'd rather look at
that separately, and actually on a separate thread because that's not
a Windows-only code path. If you'd look at the rest of the regex
code, I suspect that there could a couple of ramifications which have
similar problems (I haven't looked at that myself).
For those two ones, it looks that you are right. However, I think
that it would be safer to check if Advapi32Handle is NULL for both.Michael, I did it differently and modified the function to not need to test
NULL, I think it was better.
advapi32.dll should be present in any modern Windows platform, so
logging an error is actually fine by me instead of a warning.
I have shaved from the patch the parts which are not completely
relevant to this thread, and committed a version addressing the most
obvious leaks after doing more tests, including the changes for
restricted_token.c as of 10a5252. Thanks.
--
Michael
Em dom., 26 de jan. de 2020 às 23:04, Michael Paquier <michael@paquier.xyz>
escreveu:
On Fri, Jan 24, 2020 at 09:37:25AM -0300, Ranier Vilela wrote:
Em sex., 24 de jan. de 2020 às 04:13, Michael Paquier <
michael@paquier.xyz>
escreveu:
There is some progress. You should be careful about your patches,
as they generate compiler warnings. Here is one quote from gcc-9:
logging.c:87:13: warning: passing argument 1 of ‘free’ discards
‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
87 | free(sgr_warning);Well, in this cases, the solution is cast.
free((char *) sgr_warning);Applying blindly a cast is never a good practice.
Ok.
if (strcmp(name, "error") == 0)
+ {
+ free(sgr_error);
sgr_error = strdup(value);
+ }
I don't see the point of doing that in logging.c. pg_logging_init()
is called only once per tools, so this cannot happen. Another point
that may matter here though is that we do not complain about OOMs.
That's really unlikely to happen, and if it happens it leads to
partially colored output.Coverity show the alert, because he tries all the possibilites.Is
inside a loop. It seems to me that the only way to happen is by the
user, by introducing a repeated and wrong sequence.Again, Coverity may say something that does not apply to the reality,
and sometimes it misses some spots. Here we should be looking at
query patterns which involve a memory leak. So I'd rather look at
that separately, and actually on a separate thread because that's not
a Windows-only code path. If you'd look at the rest of the regex
code, I suspect that there could a couple of ramifications which have
similar problems (I haven't looked at that myself).
Sure, as soon as I have time, I take another look.
For those two ones, it looks that you are right. However, I think
that it would be safer to check if Advapi32Handle is NULL for both.Michael, I did it differently and modified the function to not need to
test
NULL, I think it was better.
advapi32.dll should be present in any modern Windows platform, so
logging an error is actually fine by me instead of a warning.I have shaved from the patch the parts which are not completely
relevant to this thread, and committed a version addressing the most
obvious leaks after doing more tests, including the changes for
restricted_token.c as of 10a5252. Thanks.
Thank you Michael.
best regards,
Ranier Vilela
On Fri, Jan 24, 2020 at 2:13 AM Michael Paquier <michael@paquier.xyz> wrote:
No, that's not right. I think that it is possible to loop over
ShmemProtectiveRegion in some cases. And actually, your patch is dead
wrong because this is some code called by the postmaster and it cannot
use FATAL.
Uh, really? I am not aware of such a rule.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2020-Jan-28, Robert Haas wrote:
On Fri, Jan 24, 2020 at 2:13 AM Michael Paquier <michael@paquier.xyz> wrote:
No, that's not right. I think that it is possible to loop over
ShmemProtectiveRegion in some cases. And actually, your patch is dead
wrong because this is some code called by the postmaster and it cannot
use FATAL.Uh, really? I am not aware of such a rule.
I don't think we have ever expressed it as such, but certainly we prefer
postmaster to be super robust ... rather live with a some hundred bytes
leak rather than have it die and take the whole database service down
for what's essentially a fringe bug that has bothered no one in a decade
and a half.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Em ter., 28 de jan. de 2020 às 17:54, Robert Haas <robertmhaas@gmail.com>
escreveu:
On Fri, Jan 24, 2020 at 2:13 AM Michael Paquier <michael@paquier.xyz>
wrote:No, that's not right. I think that it is possible to loop over
ShmemProtectiveRegion in some cases. And actually, your patch is dead
wrong because this is some code called by the postmaster and it cannot
use FATAL.Uh, really? I am not aware of such a rule.
Well, in postmaster.c has a structure that makes use of the variable
ShmemProtectiveRegion, I think it is related to the function in
src/backend/port/win32_shmem.c.
On line 575 in src / backend / port / win32_shmem.c, there is a comment
that tells to not use FATAL.
"Don't use FATAL since we're running in the postmaster."
regards,
Ranier Vilela
On Tue, Jan 28, 2020 at 4:06 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
I don't think we have ever expressed it as such, but certainly we prefer
postmaster to be super robust ... rather live with a some hundred bytes
leak rather than have it die and take the whole database service down
for what's essentially a fringe bug that has bothered no one in a decade
and a half.
Well, yeah. I mean, I'm not saying it's a good idea in this instance
to FATAL here. I'm just saying that I don't think there is a general
rule that code which does FATAL in the postmaster is automatically
wrong, which is what I took Michael to be suggesting.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Em ter., 28 de jan. de 2020 às 18:06, Alvaro Herrera <
alvherre@2ndquadrant.com> escreveu:
On 2020-Jan-28, Robert Haas wrote:
On Fri, Jan 24, 2020 at 2:13 AM Michael Paquier <michael@paquier.xyz>
wrote:
No, that's not right. I think that it is possible to loop over
ShmemProtectiveRegion in some cases. And actually, your patch is dead
wrong because this is some code called by the postmaster and it cannot
use FATAL.Uh, really? I am not aware of such a rule.
I don't think we have ever expressed it as such, but certainly we prefer
postmaster to be super robust ... rather live with a some hundred bytes
leak rather than have it die and take the whole database service down
for what's essentially a fringe bug that has bothered no one in a decade
and a half.
Maybe it didn't bother anyone, because the Windows port is much less used.
Anyway, I believe that freeing the memory before returning false, will not
bring down the service, changing the patch to LOG, instead of FATAL.
The primary error of the patch was to use FATAL.
regards,
Ranier Vilela
On Tue, Jan 28, 2020 at 04:11:47PM -0500, Robert Haas wrote:
On Tue, Jan 28, 2020 at 4:06 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
I don't think we have ever expressed it as such, but certainly we prefer
postmaster to be super robust ... rather live with a some hundred bytes
leak rather than have it die and take the whole database service down
for what's essentially a fringe bug that has bothered no one in a decade
and a half.Well, yeah. I mean, I'm not saying it's a good idea in this instance
to FATAL here. I'm just saying that I don't think there is a general
rule that code which does FATAL in the postmaster is automatically
wrong, which is what I took Michael to be suggesting.
Re-reading the thread, I can see your point that my previous email may
read like a rule applying to the postmaster, so sorry for the
confusion.
Anyway, I was referring to the point mentioned in three places of
pgwin32_ReserveSharedMemoryRegion() to not use FATAL for this
routine. The issue with the order of DLL loading is hard to miss..
--
Michael