Re: BUG #16348: Memory leak when parsing config

On Tue, Apr 7, 2020 at 5:44 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > I've analyzed all related code and can't see any code that's responsible
> > for freeing the variable.
>
> If you are talking about set_config_sourcefile, that does its very
> own cleanup:
>
> sourcefile = guc_strdup(elevel, sourcefile);
> if (record->sourcefile)
> free(record->sourcefile);
> record->sourcefile = sourcefile;
> record->sourceline = sourceline;
>
> That "free()" removes any previously-allocated sourcefile string.
>
> [...]
>
> I know what the high-level design is here: the temp memory context is
> for data that we don't need anymore after parsing the config file.
> But the sourceline string needs to be kept around in case somebody
> wants to see it (eg for the pg_settings view). What you propose
> *will* break that.

Thanks for your detailed analysis -- it's pretty educational! Basing on your
analysis, I'm able to fix the error in my previous analysis: we need to consider
two scenarios now. ParseConfigFileInternal returns struct ConfigVariable *, and
it has two use (or direct invokers).

One is show_all_file_settings (for pg_settings view). The returned linked list
and sourcefile referenced by it should not be freed, because it's used to
construct query results.

Another invoker is ProcessConfigFile. Here's the problem: ProcessConfigFile does
NOT use the result, which means it won't free the filename referenced in the
returned linked list. The returned filename is allocated by
set_config_sourcefile exactly.

> Generally, the results of automated leak analysis tools need to be taken
> with a mountain of salt, particularly if what you are paying attention
> to is what remains allocated at program exit. Such data can only fairly
> be called a leak if there is no accessible pointer to it --- but the tools
> are pretty bad at making that determination, at least with C programs.

Your consideration is reasonable: remaining heap object can still be referenced
when exit; in this case, it's not a leak. But have you tried LeakSanitizer, a
"modern" memory leak detector? LeakSanitizer works like a garbage collector ---
it does trace objects from the "root set", including the globals, the registers,
each thread's stack, and TLS variables. As long as you stay away from pointer
black magics, LeakSanitizer is unlikely to produce false-positives.

Even if I believe in LeakSanitizer, it's absolutely stupid to ignore the
recommendations from an experienced developer. To really make sure that there's
memory leak, I verified the leak with gdb. When ProcessConfigFile is entered, I
set a breakpoint to print guc_strdup's return value ($rax) in
set_config_sourcefile. I also set a breakpoint on free to print the argument
($rdi). To sum up: for any returned pointer ofguc_strdup, if a
corresponding free
cannot be found, then we can confirm that there's a leak.

Attached is the output of gdb, which confirms the leak.