Обсуждение: BUG #16160: Minor memory leak in case of starting postgres server with SSL encryption

The following bug has been logged on the website:

Bug reference:      16160
Logged by:          Dmitry Uspenskiy
Email address:      duspensky@ya.ru
PostgreSQL version: 12.1
Operating system:   Any
Description:

LeakSanitizer shows memory leak at the following place:

[ts-1] ==17346==ERROR: LeakSanitizer: detected memory leaks
[ts-1] 
[ts-1] Direct leak of 144 byte(s) in 1 object(s) allocated from:
[ts-1]     #0 0x563b7f in __interceptor_malloc
/home/duspensky/code/yugabyte-db/thirdparty/build/common/llvm-7.0.1.src/../../../src/llvm-7.0.1.src/projects/comp
iler-rt/lib/asan/asan_malloc_linux.cc:146
[ts-1]     #1 0x7f697e6a7ee7 in CRYPTO_malloc
(/home/duspensky/.linuxbrew-yb-build/linuxbrew-20181203T161736/lib/libcrypto.so.1.0.0+0x71ee7)
[ts-1]     #2 0x7f697e746b2e in DH_new_method
(/home/duspensky/.linuxbrew-yb-build/linuxbrew-20181203T161736/lib/libcrypto.so.1.0.0+0x110b2e)
[ts-1]     #3 0x7f697e74609c in dh_cb
(/home/duspensky/.linuxbrew-yb-build/linuxbrew-20181203T161736/lib/libcrypto.so.1.0.0+0x11009c)
[ts-1]     #4 0x7f697e78a1fc in ASN1_item_ex_new
(/home/duspensky/.linuxbrew-yb-build/linuxbrew-20181203T161736/lib/libcrypto.so.1.0.0+0x1541fc)
[ts-1]     #5 0x7f697e78f09a in ASN1_item_ex_d2i
(/home/duspensky/.linuxbrew-yb-build/linuxbrew-20181203T161736/lib/libcrypto.so.1.0.0+0x15909a)
[ts-1]     #6 0x7f697e78f7ba in ASN1_item_d2i
(/home/duspensky/.linuxbrew-yb-build/linuxbrew-20181203T161736/lib/libcrypto.so.1.0.0+0x1597ba)
[ts-1]     #7 0x7f697e7a0ad3 in PEM_read_bio_DHparams
(/home/duspensky/.linuxbrew-yb-build/linuxbrew-20181203T161736/lib/libcrypto.so.1.0.0+0x16aad3)
[ts-1]     #8 0xcd75bd in load_dh_buffer

/home/duspensky/code/yugabyte-db/src/postgres/src/backend/libpq/../../../../../../src/postgres/src/backend/libpq/be-secure-openssl.c:848:7
[ts-1]     #9 0xcd4d07 in initialize_dh

/home/duspensky/code/yugabyte-db/src/postgres/src/backend/libpq/../../../../../../src/postgres/src/backend/libpq/be-secure-openssl.c:976:8
[ts-1]     #10 0xcd437c in be_tls_init

/home/duspensky/code/yugabyte-db/src/postgres/src/backend/libpq/../../../../../../src/postgres/src/backend/libpq/be-secure-openssl.c:198:7
[ts-1]     #11 0xeecd86 in PostmasterMain

/home/duspensky/code/yugabyte-db/src/postgres/src/backend/postmaster/../../../../../../src/postgres/src/backend/postmaster/postmaster.c:981:10
[ts-1]     #12 0xcd79e3 in PostgresServerProcessMain

/home/duspensky/code/yugabyte-db/src/postgres/src/backend/main/../../../../../../src/postgres/src/backend/main/main.c:234:3
[ts-1]     #13 0xcd8081 in main
(/home/duspensky/code/yugabyte-db/build/asan-clang-dynamic-ninja/postgres/bin/postgres+0xcd8081)

According to the following information

https://wiki.openssl.org/index.php/Diffie-Hellman_parameters

DH_free function must be called after SSL_CTX_set_tmp_dh

On Wed, Dec 11, 2019 at 04:22:03PM +0000, PG Bug reporting form wrote:
> According to the following information
>
> https://wiki.openssl.org/index.php/Diffie-Hellman_parameters
>
> DH_free function must be called after SSL_CTX_set_tmp_dh

That's not directly mentioned on their docs actually:
https://www.openssl.org/docs/man1.0.2/man3/SSL_CTX_set_tmp_dh.html

But it seems to me that you are right.  If I look at the OpenSSL code,
ssl3_ctrl() does a DH_free() on error when going though
SSL_CTRL_SET_TMP_DH, and the code copies the DH directly using
DHparams_dup() so we don't need to keep any reference to it in our
code.

One more disturbing issue is that we can would accumulate garbage if
we keep reloading the SSL context in the postmaster.  For this reason,
it could justify a backpatch down to the point where SSL parameters
are reloadable.  On the other hand, the leak is small, so my take
is actually to just fix HEAD and call it a day.

Attached is a patch, I'll go commit that if there are no objections.
The DH handling does not really change regarding the way it gets
free'd or not down to 0.9.8.
--
Michael

On Fri, Dec 13, 2019 at 03:39:15PM +0900, Michael Paquier wrote:
> Attached is a patch, I'll go commit that if there are no objections.
> The DH handling does not really change regarding the way it gets
> free'd or not down to 0.9.8.

And committed.  Dmitry has pointed out offline that we need to do the
same with the error code path, and he is right as OpenSSL does not
touch the passed-in DH information for 0.9.8~.
--
Michael

Andres Freund <andres@anarazel.de> writes:
> On 2021-03-16 14:44:55 -0400, Stephen Frost wrote:
>> OOMs errors should be gracefully handled and PG should continue to
>> function.  Was that not the case..?

> Depends on what you mean with graceful. Unless you restart the database
> you'll eventually not be able to do anything anymore, since even the
> smallest memory allocation will fail?

Yeah, a leak in the postmaster is not gonna end well.

I went ahead and back-patched e0e569e1d.  While looking at that,
I noticed that the load_dh_file subroutine was also a few DH_frees
shy of a load, so I fixed that too.

            regards, tom lane