Re: [PATCH] Windows port, fix some resources leaks

Em sex., 24 de jan. de 2020 às 04:13, Michael Paquier <michael(at)paquier(dot)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