Thread: [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
Attachment
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.
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.
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.
In case of failure, it was necessary to release the handlers properly.
regards,
Ranier Vilela
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
Last time improvement to restricted_token.c
regards,
Ranier Vilela
Attachment
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
Attachment
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.
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