BUG #16321: Memory leaks in PostmasterMain

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

BUG #16321: Memory leaks in PostmasterMain

PG Bug reporting form
The following bug has been logged on the website:

Bug reference:      16321
Logged by:          Hugh Wang
Email address:      [hidden email]
PostgreSQL version: 12.2
Operating system:   Linux
Description:        

The argument parsing duplicates strings, but never frees them.

For example, when you pass "-D $DATA_DIR" to postmaster, postmaster
duplicates the string here:
https://github.com/postgres/postgres/blob/master/src/backend/postmaster/postmaster.c#L698

The duplicated string is passed to `SelectConfigFiles`, which does
everything except freeing the string.

Reply | Threaded
Open this post in threaded view
|

Re: BUG #16321: Memory leaks in PostmasterMain

Tom Lane-2
PG Bug reporting form <[hidden email]> writes:
> The argument parsing duplicates strings, but never frees them.

This hardly amounts to enough of a problem to worry about.  The
string might be leaked, or it might not, but tracking whether it
is is more trouble than it's worth.  Generally we only worry about
memory leaks if they (a) can waste a lot of memory or (b) can
repeat, and thereby accumulate to waste a lot of memory.  Surely
neither one applies to postmaster argument parsing.

> For example, when you pass "-D $DATA_DIR" to postmaster, postmaster
> duplicates the string here:
> https://github.com/postgres/postgres/blob/master/src/backend/postmaster/postmaster.c#L698
> The duplicated string is passed to `SelectConfigFiles`, which does
> everything except freeing the string.

This is a great example of a case where the cure is likely to be
worse than the disease.  SelectConfigFiles surely has little business
freeing its input string (indeed, it couldn't do so without casting
away the "const").  On the other hand, the caller doesn't really
know whether SelectConfigFiles is going to stash away a copy of the
pointer; it wouldn't be unreasonable for it to do so.  So in order
to not perhaps-leak a few dozen bytes, we'd have to make that API
more complicated and more fragile.  It's not a win.

As for why we strdup the argument in the first place, see here:

https://www.postgresql.org/message-id/flat/20121008184026.GA28752%40momjian.us

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16321: Memory leaks in PostmasterMain

Hugh Wang
Hi Tom,

On Fri, Mar 27, 2020 at 2:52 PM Tom Lane <[hidden email]> wrote:
PG Bug reporting form <[hidden email]> writes:
> The argument parsing duplicates strings, but never frees them.

This hardly amounts to enough of a problem to worry about.  The
string might be leaked, or it might not, but tracking whether it
is is more trouble than it's worth.  Generally we only worry about
memory leaks if they (a) can waste a lot of memory or (b) can
repeat, and thereby accumulate to waste a lot of memory.  Surely
neither one applies to postmaster argument parsing.

Your analysis is pretty educational! If the leak is small and has low impact, then the leak itself is not important; yet fixing the bug brings more complexity.

However, from the perspective of automated bug finding, I think removing the bug is beneficial. I'm trying to find bugs in PostgreSQL with sanitizers (the leak is reported by LeakSanitizer). If the bug cannot be fixed, LeakSanitizer stops at this shallow point, which prevents detecting more bugs in deep logic.
 
> For example, when you pass "-D $DATA_DIR" to postmaster, postmaster
> duplicates the string here:
> https://github.com/postgres/postgres/blob/master/src/backend/postmaster/postmaster.c#L698
> The duplicated string is passed to `SelectConfigFiles`, which does
> everything except freeing the string.

This is a great example of a case where the cure is likely to be
worse than the disease.  SelectConfigFiles surely has little business
freeing its input string (indeed, it couldn't do so without casting
away the "const").  On the other hand, the caller doesn't really
know whether SelectConfigFiles is going to stash away a copy of the
pointer; it wouldn't be unreasonable for it to do so.  So in order
to not perhaps-leak a few dozen bytes, we'd have to make that API
more complicated and more fragile.  It's not a win.

As for why we strdup the argument in the first place, see here:

https://www.postgresql.org/message-id/flat/20121008184026.GA28752%40momjian.us

                        regards, tom lane

Thanks,
Hugh
Reply | Threaded
Open this post in threaded view
|

Re: BUG #16321: Memory leaks in PostmasterMain

Francisco Olarte
On Mon, Mar 30, 2020 at 4:11 AM Hugh Wang <[hidden email]> wrote:
> On Fri, Mar 27, 2020 at 2:52 PM Tom Lane <[hidden email]> wrote:
>> PG Bug reporting form <[hidden email]> writes:
>> > The argument parsing duplicates strings, but never frees them.
...
> Your analysis is pretty educational! If the leak is small and has low impact, then the leak itself is not important; yet fixing the bug brings more complexity.

Bear in mind some of this cases are not leaks. You may need the string
live for the whole program lifetime and elect to free them by exiting.

I've had similar cases with a category of my programs which work by
reading many input files and building structures in memory, which I
then use to print some reports. After that I free them by exiting the
program, which is much faster, no leak there. There are a whole lot of
programs which do this, they memory use raises and raises while they
work, and they need all of them, and they free all at once by exiting,
its a valid way to free resources when the only resource is used
memory, the OS frees it on exit.

> However, from the perspective of automated bug finding, I think removing the bug is beneficial. I'm trying to find bugs in PostgreSQL with sanitizers (the leak is reported by LeakSanitizer). If the bug cannot be fixed, LeakSanitizer stops at this shallow point, which prevents detecting more bugs in deep logic.

This may not be a bug. Doesn't leak sanitizer have a way to mark some
data as never freed? A leak is something which you should track and
are not, which may eventually lead to your memory use raising. This
doesn't seem to be the case.


Francisco Olarte.