Offsets of `struct Port` are no longer constant

Started by Jacob Championabout 1 year ago6 messages
#1Jacob Champion
jacob.champion@enterprisedb.com
1 attachment(s)

Hi all,

A comment at the end of the Port struct says

/*
* OpenSSL structures. (Keep these last so that the locations of other
* fields are the same whether or not you build with SSL enabled.)
*/

but as part of the direct-SSL changes in d39a49c1e45 (cc'd Heikki),
some additional fields snuck in after it.

I assume it's too late to change this for 17, but should this be
addressed in HEAD? I've attached a draft patch (untested, beware)
which should no longer rely on field order.

Thanks,
--Jacob

Attachments:

port-offset.diffapplication/octet-stream; name=port-offset.diffDownload
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index d97d1e5f6b..e785b36d3f 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -207,12 +207,15 @@ typedef struct Port
 	bool		last_read_was_eof;
 
 	/*
-	 * OpenSSL structures. (Keep these last so that the locations of other
-	 * fields are the same whether or not you build with SSL enabled.)
+	 * OpenSSL structures. As with GSSAPI above, to keep struct offsets
+	 * constant, NULL pointers are stored when SSL support is not enabled.
 	 */
 #ifdef USE_OPENSSL
 	SSL		   *ssl;
 	X509	   *peer;
+#else
+	void	   *ssl;
+	void	   *peer;
 #endif
 
 	/*
#2Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Jacob Champion (#1)
Re: Offsets of `struct Port` are no longer constant

On 11/11/2024 18:34, Jacob Champion wrote:

Hi all,

A comment at the end of the Port struct says

/*
* OpenSSL structures. (Keep these last so that the locations of other
* fields are the same whether or not you build with SSL enabled.)
*/

but as part of the direct-SSL changes in d39a49c1e45 (cc'd Heikki),
some additional fields snuck in after it.

I assume it's too late to change this for 17, but should this be
addressed in HEAD? I've attached a draft patch (untested, beware)
which should no longer rely on field order.

Oops. Fortunately this is not likely to cause trouble in practice, I
can't imagine an extension peeking into 'raw_buf' and friends. But yeah,
we should do something at least on HEAD. +1 on your patch; I'll commit
that unless someone has better ideas.

On REL_17_STABLE, we should probably adjust the comment to warn that
'raw_buf' and friends can move depending on USE_OPENSSL.

--
Heikki Linnakangas
Neon (https://neon.tech)

#3Jacob Champion
jacob.champion@enterprisedb.com
In reply to: Heikki Linnakangas (#2)
Re: Offsets of `struct Port` are no longer constant

On Mon, Nov 11, 2024 at 11:13 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On REL_17_STABLE, we should probably adjust the comment to warn that
'raw_buf' and friends can move depending on USE_OPENSSL.

Yeah, makes sense.

--Jacob

#4Daniel Gustafsson
daniel@yesql.se
In reply to: Jacob Champion (#3)
Re: Offsets of `struct Port` are no longer constant

On 11 Nov 2024, at 20:17, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

On Mon, Nov 11, 2024 at 11:13 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On REL_17_STABLE, we should probably adjust the comment to warn that
'raw_buf' and friends can move depending on USE_OPENSSL.

Yeah, makes sense.

+1

#5Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Daniel Gustafsson (#4)
Re: Offsets of `struct Port` are no longer constant

On 11/11/2024 21:43, Daniel Gustafsson wrote:

On 11 Nov 2024, at 20:17, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

On Mon, Nov 11, 2024 at 11:13 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On REL_17_STABLE, we should probably adjust the comment to warn that
'raw_buf' and friends can move depending on USE_OPENSSL.

Yeah, makes sense.

+1

Ok, committed the "placeholder" fields to master, and just comment fixes
to REL_17_STABLE.

--
Heikki Linnakangas
Neon (https://neon.tech)

#6Jacob Champion
jacob.champion@enterprisedb.com
In reply to: Heikki Linnakangas (#5)
Re: Offsets of `struct Port` are no longer constant

On Fri, Nov 22, 2024 at 8:06 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

Ok, committed the "placeholder" fields to master, and just comment fixes
to REL_17_STABLE.

Thanks!

--Jacob