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