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

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