Re: [PATCH] Windows port, fix some resources leaks - Mailing list pgsql-hackers

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.

        }
        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.