Grab bag of smaller OpenSSL fixups

On Mon Jul 13, 2026 at 2:17 PM UTC, Daniel Gustafsson wrote:

I've been collecting a few small fixups to the OpenSSL code and figured it was
time to send them over to keep the number of fixes more manageable.

* 0001: Remove the use of static variable to not interfere with the future
multithreading efforts
* 0002: Replace SSLv23_method with TLS_method, it has been an alias to the new
name since 2015 in OpenSSL
* 0003: Replace deprecated API X509_NAME_get_text_by_NID with the recommended
alternative X509_NAME_get_index_by_NID + X509_NAME_get_entry. The API was
deprecated in OpenSSL 4 and risk getting removed, with the alternatives being
available in all supported versions.
* 0004: Fix TLS protocol detection. We have been using the wrong macro for
feature test for a long time, replace with the actual feature test macro.
(There is no protocol downgrade here, it will error out when misconfiguring).

None of these change existing functionality or introduce new functionality.

The patches look good. I did have one comment in patch 0001. For the
hasWarned argument to init_host_context, do we you think we should add
an Assert(hasWarned) or add a pg_attribute_nonnull(3)? I see we
dereference the pointer without first checking for its validity. I know
it isn't common in Postgres source code to add such protections, but
I figured I would point it out anyway.

--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)

On 7/13/26 16:16, Daniel Gustafsson wrote:

I've been collecting a few small fixups to the OpenSSL code and figured it was
time to send them over to keep the number of fixes more manageable.

Nice!

* 0001: Remove the use of static variable to not interfere with the future
multithreading efforts

Looks good!

* 0002: Replace SSLv23_method with TLS_method, it has been an alias to the new
name since 2015 in OpenSSL

Looks good!

* 0003: Replace deprecated API X509_NAME_get_text_by_NID with the recommended
alternative X509_NAME_get_index_by_NID + X509_NAME_get_entry. The API was
deprecated in OpenSSL 4 and risk getting removed, with the alternatives being
available in all supported versions.

You should declare entry in the inner scope, not in the function scope.

The comment should maybe also add that entry and peer_cn_asn1 too are
OpenSSL owned pointers. Just saying it for peer_cn_internal seems a bit
odd to me.

Also this is subjective but I am not personally a fan of
MemoryContextAllocZero() + memcpy(). I prefer MemoryContextAlloc() +
memcpy() + peer_cn[len] = 0. I feel that explains better what is going
on. I have also seen us use strncpy() for this purpose which I guess
works too.

* 0004: Fix TLS protocol detection. We have been using the wrong macro for
feature test for a long time, replace with the actual feature test macro.
(There is no protocol downgrade here, it will error out when misconfiguring).

Looks good! That was a funny and easy to make mistake :)

--
Andreas Karlsson
Percona

On 13 Jul 2026, at 22:38, Tristan Partin <tristan@partin.io> wrote:

The patches look good.

Thanks for reviewing!

I did have one comment in patch 0001. For the
hasWarned argument to init_host_context, do we you think we should add
an Assert(hasWarned) or add a pg_attribute_nonnull(3)? I see we
dereference the pointer without first checking for its validity. I know
it isn't common in Postgres source code to add such protections, but
I figured I would point it out anyway.

Given that this is a very niche static function with very few callsites, I'm
not too worried about it being called with NULL. That being said, defensive
programming isn't just to defend against what we know but also what we don't
know about so I'm not against adding such protections. There are likely many
other functions in this file which could benefit more from pg_attribute_nonnull
but we also need to start somewhere.

--
Daniel Gustafsson

On 14 Jul 2026, at 00:25, Andreas Karlsson <andreas@proxel.se> wrote:
On 7/13/26 16:16, Daniel Gustafsson wrote:

* 0003: Replace deprecated API X509_NAME_get_text_by_NID with the recommended
alternative X509_NAME_get_index_by_NID + X509_NAME_get_entry. The API was
deprecated in OpenSSL 4 and risk getting removed, with the alternatives being
available in all supported versions.

You should declare entry in the inner scope, not in the function scope.

Ah, yes. It was already in the inner scope, but it should've been in
inner-inner scope =)

The comment should maybe also add that entry and peer_cn_asn1 too are OpenSSL owned pointers. Just saying it for peer_cn_internal seems a bit odd to me.

The other pointers are of OpenSSL data types and those are rarely freed unless
a matching _new or _dup method has been called, whereas peer_cn_internal is a
char pointer extracted with _get0_data. To me the latter seemed more
reasonable to believe it was caller owned, but I can remove the comment
altogether if it's deemed useless.

Also this is subjective but I am not personally a fan of MemoryContextAllocZero() + memcpy(). I prefer MemoryContextAlloc() + memcpy() + peer_cn[len] = 0. I feel that explains better what is going on. I have also seen us use strncpy() for this purpose which I guess works too.

We can't use strncpy or strlcpy here since OpenSSL had defined the string as
const unsigned char * so we'd get pointer-sign compiler warnings unless we do
trickery, which seems not worth it. I don't have strong opinions on how to
zero the buffer so changed to your proposal.

The attached also contains the non-null guards from Tristans review upthread.

--
Daniel Gustafsson

On 7/14/26 10:35, Daniel Gustafsson wrote:

The comment should maybe also add that entry and peer_cn_asn1 too are OpenSSL owned pointers. Just saying it for peer_cn_internal seems a bit odd to me.

The other pointers are of OpenSSL data types and those are rarely freed unless
a matching _new or _dup method has been called, whereas peer_cn_internal is a
char pointer extracted with _get0_data. To me the latter seemed more
reasonable to believe it was caller owned, but I can remove the comment
altogether if it's deemed useless.

I don't have a strong opinion here so do whatever you feel like, but at
least to me the current comment does not add much value.

The attached also contains the non-null guards from Tristans review upthread.

My compiler did not like the mix of both assert and nonnull attribute.

In file included from ../postgresql/src/include/postgres.h:48,
from
../postgresql/src/backend/libpq/be-secure-openssl.c:17:
../postgresql/src/backend/libpq/be-secure-openssl.c: In function
‘init_host_context’:
../postgresql/src/include/c.h:1018:20: warning: ‘nonnull’ argument
‘hasWarned’ compared to NULL [-Wnonnull-compare]
1018 | if (!(condition)) \
| ^
../postgresql/src/backend/libpq/be-secure-openssl.c:613:9: note: in
expansion of macro ‘Assert’
613 | Assert(hasWarned);
| ^~~~~~

--
Andreas Karlsson
Percona

On 14 Jul 2026, at 14:57, Andreas Karlsson <andreas@proxel.se> wrote:

My compiler did not like the mix of both assert and nonnull attribute.

Interesting, mine didn't complain, but it's also pretty redundant to have both
so removing the Assert and keeping the cost compile-time rather than runtime
seem the right move.

--
Daniel Gustafsson

On 7/14/26 15:17, Daniel Gustafsson wrote:

On 14 Jul 2026, at 14:57, Andreas Karlsson <andreas@proxel.se> wrote:

My compiler did not like the mix of both assert and nonnull attribute.

Interesting, mine didn't complain, but it's also pretty redundant to have both
so removing the Assert and keeping the cost compile-time rather than runtime
seem the right move.

If you are curious I am running gcc 13.3.

Compiler:

gcc (Ubuntu 13.3.0-6ubuntu2~24.04.1) 13.3.0

Flags according to meson compile --verbose:

cc -Isrc/backend/postgres_lib.a.p -Isrc/include
-I../postgresql/src/include -I/usr/include/libxml2
-I/usr/include/security -fdiagnostics-color=always
-D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O0 -g -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -D_GNU_SOURCE -Wpointer-arith
-Werror=vla -Wmissing-format-attribute -Wcast-function-type
-Wshadow=compatible-local -Wformat-security -Wmissing-prototypes
-Wold-style-declaration -Wold-style-definition -Wstrict-prototypes
-Wimplicit-fallthrough=5 -Wdeclaration-after-statement
-Wno-format-truncation -Wno-stringop-truncation -DUSE_VALGRIND -fPIC
-isystem /usr/include/mit-krb5 -pthread -DBUILDING_DLL -MD -MQ
src/backend/postgres_lib.a.p/libpq_be-secure-openssl.c.o -MF
src/backend/postgres_lib.a.p/libpq_be-secure-openssl.c.o.d -o
src/backend/postgres_lib.a.p/libpq_be-secure-openssl.c.o -c
../postgresql/src/backend/libpq/be-secure-openssl.c

--
Andreas Karlsson
Percona

On Tue Jul 14, 2026 at 7:49 AM UTC, Daniel Gustafsson wrote:

On 13 Jul 2026, at 22:38, Tristan Partin <tristan@partin.io> wrote:
I did have one comment in patch 0001. For the
hasWarned argument to init_host_context, do we you think we should add
an Assert(hasWarned) or add a pg_attribute_nonnull(3)? I see we
dereference the pointer without first checking for its validity. I know
it isn't common in Postgres source code to add such protections, but
I figured I would point it out anyway.

Given that this is a very niche static function with very few callsites, I'm
not too worried about it being called with NULL. That being said, defensive
programming isn't just to defend against what we know but also what we don't
know about so I'm not against adding such protections. There are likely many
other functions in this file which could benefit more from pg_attribute_nonnull
but we also need to start somewhere.

Indeed. There are places all over the codebase that would likely benefit
from pg_attribute_nonnull. Choosing when to add the annotation takes
a bit of thought.

--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)

On Tue Jul 14, 2026 at 8:35 AM UTC, Daniel Gustafsson wrote:

On 14 Jul 2026, at 00:25, Andreas Karlsson <andreas@proxel.se> wrote:
On 7/13/26 16:16, Daniel Gustafsson wrote:

* 0003: Replace deprecated API X509_NAME_get_text_by_NID with the recommended
alternative X509_NAME_get_index_by_NID + X509_NAME_get_entry. The API was
deprecated in OpenSSL 4 and risk getting removed, with the alternatives being
available in all supported versions.

You should declare entry in the inner scope, not in the function scope.

Ah, yes. It was already in the inner scope, but it should've been in
inner-inner scope =)

The comment should maybe also add that entry and peer_cn_asn1 too are OpenSSL owned pointers. Just saying it for peer_cn_internal seems a bit odd to me.

The other pointers are of OpenSSL data types and those are rarely freed unless
a matching _new or _dup method has been called, whereas peer_cn_internal is a
char pointer extracted with _get0_data. To me the latter seemed more
reasonable to believe it was caller owned, but I can remove the comment
altogether if it's deemed useless.

Also this is subjective but I am not personally a fan of MemoryContextAllocZero() + memcpy(). I prefer MemoryContextAlloc() + memcpy() + peer_cn[len] = 0. I feel that explains better what is going on. I have also seen us use strncpy() for this purpose which I guess works too.

We can't use strncpy or strlcpy here since OpenSSL had defined the string as
const unsigned char * so we'd get pointer-sign compiler warnings unless we do
trickery, which seems not worth it. I don't have strong opinions on how to
zero the buffer so changed to your proposal.

I wonder if it is worth leaving your justification in a comment. Other
than that, nothing to add.

--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)

On 14 Jul 2026, at 22:50, Tristan Partin <tristan@partin.io> wrote:

I wonder if it is worth leaving your justification in a comment. Other
than that, nothing to add.

Not sure if it's all that interesting, I did however add a function comment on
ssl_init_context which explains the hasWarned parameter as that was lacking.
The attached v3 removes the openssl-owned comment and fixed the compiler
warning, both mentioned upthread.

--
Daniel Gustafsson