void sk_free(STACK *); This function free()'s a stack structure. The elements in the stack will not be freed so one should 'pop' and free all elements from the stack before calling this function or call sk_pop_free() instead.
void sk_pop_free(STACK *st; void (*func)()); This function calls 'func' for each element on the stack, passing the element as the argument. sk_free() is then called to free the 'stack' structure.
On Tue, Apr 21, 2020 at 9:42 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Apr 20, 2020 at 08:40:06PM +0300, Roman Peshkurov wrote: > The problem is incorrect usage of openssl stack "destructor". We noticed it > on a graphs for the few months. It looked lika an increasing line of res > memory usage. > > Sorry, I've already made pull request (wasn't familiar with your flow). The > all information is described there: > https://github.com/postgres/postgres/pull/52
Thanks for taking the time to send an email here. I suspect that few people around here look at the cloned repo on github (I don't FWIW).
If this reference goes away, then then community archives would lose a part of its history. So, first, here is the patch for reference: --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -552,7 +552,7 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn *conn, if (rc != 0) break; } - sk_GENERAL_NAME_free(peer_san); + sk_GENERAL_NAME_pop_free(peer_san, GENERAL_NAME_free); }
Looking at the samples of the OpenSSL wiki, it looks that your guess is right: https://wiki.openssl.org/index.php/Hostname_validation After extracting the SAN data from the certificate using X509_get_ext_d2i(), we get to use sk_GENERAL_NAME_pop_free() for a correct in-depth free().
Now, we have in core a set of TAP tests in src/test/ssl with certificates that include some SAN configuration for sslmode=verify-full, able to trigger the code path you are mentioning for pgtls_verify_peer_name_matches_certificate_guts() within fe-secure-openssl.c. In order to see the leak, (I have been lazy with the method and did not use valgrind with a custom C program or just psql, but the result is the same), I have put a custom hack in the code path involved here by adding a tight loop repeating X509_get_ext_d2i() + sk_GENERAL_NAME_free(), then ran the TAP tests of src/test/ssl/. After that, the leak becomes really clear after 5k loops or such in psql, with sk_GENERAL_NAME_pop_free() preventing the issue any rise of memory usage.
sk_GENERAL_NAME_pop_free() exists down to OpenSSL 0.9.6, which is what I recall we officially support down to Postgres 9.5, so we are safe.
And finally, this needs to be backpatched all the way down to 9.5, so I'll take care of it if there are no objections.
