Horton

Thread

Commits

  1. Fix some memory leaks and improve restricted token handling on Windows

  1. [PATCH] Windows port, fix some resources leaks

    Ranier Vilela <ranier.vf@gmail.com> — 2020-01-19T20:49:08Z

    Hi, greetings everyone.

Continuing the process of improving windows port, I'm trying to fix some
leaks.

best regards,
Ranier Vilela

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

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2020-01-21T09:17:51Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2020-01-21T14:01:07Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2020-01-22T06:12:32Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2020-01-22T20:51:51Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2020-01-24T07:13:01Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2020-01-24T12:37:25Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2020-01-24T12:59:33Z

    Last time improvement to restricted_token.c

regards,
Ranier Vilela

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

    Michael Paquier <michael@paquier.xyz> — 2020-01-27T02:04:05Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2020-01-27T13:39:35Z

    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

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

    Robert Haas <robertmhaas@gmail.com> — 2020-01-28T20:54:35Z

    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

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-01-28T21:06:17Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2020-01-28T21:08:17Z

    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

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

    Robert Haas <robertmhaas@gmail.com> — 2020-01-28T21:11:47Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2020-01-28T21:19:48Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2020-01-29T07:24:11Z

    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