| From: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
|---|---|
| To: | Andreas Karlsson <andreas(at)proxel(dot)se> |
| Cc: | pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Tristan Partin <tristan(at)partin(dot)io> |
| Subject: | Re: Grab bag of smaller OpenSSL fixups |
| Date: | 2026-07-14 08:35:37 |
| Message-ID: | 214DC35C-1711-4E1F-A890-C4959EAB0ED8@yesql.se |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On 14 Jul 2026, at 00:25, Andreas Karlsson <andreas(at)proxel(dot)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
| Attachment | Content-Type | Size |
|---|---|---|
| v2-0001-ssl-Remove-static-var-tracking-tls_init_hook-warn.patch | application/octet-stream | 3.4 KB |
| v2-0002-ssl-Use-TLS_method-instead-of-deprecated-SSLv23_m.patch | application/octet-stream | 2.5 KB |
| v2-0003-ssl-Replace-deprecated-API-to-get-commonName.patch | application/octet-stream | 2.6 KB |
| v2-0004-ssl-Use-the-correct-feature-macros-for-TLS-protoc.patch | application/octet-stream | 3.0 KB |
| unknown_filename | text/plain | 1 byte |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Karina Litskevich | 2026-07-14 08:44:25 | Re: Implicit conversion from int64 to int32 when calling hash_get_num_entries |
| Previous Message | Mahendra Singh Thalor | 2026-07-14 08:30:54 | Restore check_mut_excl_opts, usage in pg_restore and pg_dumpall |