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.
