Re: [PATCH] Fix memory leak in pg_config

Thanks for the review.

LeakSanitizer is widely used tools, so I think it
would be useful to make PostgreSQL work cleanly with them. We could
either fix reported leaks where cleanup is straightforward, or maintain
suppressions for intentional process-lifetime allocations. I prefer
fixing the leaks where possible.

We use these sanitizers in our own work, so we can continue examining
their reports across PostgreSQL, fix actual leaks, add suppressions where
appropriate, and submit the changes as separate patches.

For pg_config, the cleanup is small and straightforward. A broad
suppression for get_configdata() could also hide future leaks from the
same call path.

In v2, I restructured the control flow using if/else, so
free_configdata() is called only once before returning from main().
Invalid arguments now set the exit code and break out of the loop
instead of calling exit().

Personally, I prefer the simpler control flow used in v1.

I also corrected the function definition style.

The updated patch is attached.

On 26-07-21 21:35, Andrey Borodin wrote:
> Hi Ivan,
>
>> LeakSanitizer reports 2829 bytes leaked in 47 allocations.
>
> I can confirm the leak and the fix. The 47 allocations are exactly 23
> names + 23 settings + 1 array. With your patch the tool reports zero.
>
> Fun archaeology: this leak celebrates its 10th anniversary this year.
> It arrived with a5c43b88694 (PG 9.6), which introduced get_configdata()
> with the comment "the caller is responsible for pfreeing the result".
> pg_config has been ignoring that contract ever since. Before 9.6 it
> used static buffers and had nothing to leak at all. Simpler times.
>
> One thing to be aware of: our CI runs sanitizers with
> detect_leaks=0, and the comment there says "too many uninteresting
> leak errors in short-lived binaries". This leak is a prime specimen
> of the genre, so the answer here may well be "the OS frees it faster
> than we do". If the goal is quiet LSan runs, a suppression list might
> scale better: there are many binaries other than pg_config that leak
> too. Anyway, I think it would be good to quite LSan if possible...
>
> Two small notes:
>
> - Validating the arguments before calling get_configdata() would
> leave a single free before return instead of three call sites.
> - The function definition puts "static" alone on its own line. The
> usual style is "static void" together, then the function name on
> the next line.
>
> Thank you!
>
> Best regards, Andrey Borodin.
>

--
Best wishes,
Ivan Kush
Tantor Labs LLC