Date: Mon, 28 Mar 2022 13:31:16 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Preventing indirection for IndexPageGetOpaque for known-size
 page special areas
Message-ID: <YkE6FNTng8a1SVas@paquier.xyz>
References: 
 <CAEze2WjE3+tGO9Fs9+iZMU+z6mMZKo54W1Zt98WKqbEUHbHOBg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="4HkTsHVIsffrlys8"
Content-Disposition: inline
In-Reply-To: 
 <CAEze2WjE3+tGO9Fs9+iZMU+z6mMZKo54W1Zt98WKqbEUHbHOBg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YkE6FNTng8a1SVas%40paquier.xyz>
Precedence: bulk


--4HkTsHVIsffrlys8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Feb 16, 2022 at 10:24:58PM +0100, Matthias van de Meent wrote:
> A first good reason to do this is preventing further damage when a
> page is corrupted: if I can somehow overwrite pd_special,
> non-assert-enabled builds would start reading and writing at arbitrary
> offsets from the page pointer, quite possibly in subsequent buffers
> (or worse, on the stack, in case of stack-allocated blocks).

Well, pageinspect has proved to be rather careless in this area for a
couple of years, but this just came from the fact that we called
PageGetSpecialPointer() before checking that PageGetSpecialSize() maps
with the MAXALIGN'd size of the opaque area, if the related AM has
one.

I am not sure that we need to worry about corruptions, as data
checksums would offer protection for most cases users usually need to
worry about, at the exception of page corruptions because of memory
for pages already read in the PG shared buffers from disk or even the
OS cache.

> A second reason would be less indirection to get to the opaque
> pointer. This should improve performance a bit in those cases where we
> (initially) only want to access the [Index]PageOpaque struct.

We don't have many cases that do that, do we?

> Lastly, 0002 and 0003 remove some repetitive tasks of dealing with the
> [nbtree, hash] opaques by providing a typed accessor macro similar to
> what is used in the GIN and (SP-)GIST index methods; improving the
> legibility of the code and decreasing the churn.

+#define PageGetSpecialOpaque(page, OpaqueDataTyp) \
+( \
+   AssertMacro(PageGetPageSize(page) == BLCKSZ && \
+               PageGetSpecialSize(page) == MAXALIGN(sizeof(OpaqueDataTyp))), \
+   (OpaqueDataTyp *) ((char *) (page) + (BLCKSZ - MAXALIGN(sizeof(OpaqueDataTyp)))) \
+)

Did you notice PageValidateSpecialPointer() that mentions MSVC?  Could
this stuff a problem if not an inline function.

Perhaps you should do a s/OpaqueDataTyp/OpaqueDataType/.

As a whole, this patch set looks like an improvement in terms of
checks and consistency regarding the special area handling across the
different AMs for the backend code, while reducing the MAXALIGN-ism on
all those sizes, and this tends to be missed.  Any other opinions?
--
Michael

--4HkTsHVIsffrlys8
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmJBOhQACgkQnvQgOdby
QH2jIw/+OsnNiI78i9hJVdKxU2/Twu7gZf42kXjf8ACtVRTn3N7O0gVp5Gb4EJk5
0DRbp58gJtMP+yNOelAyS4e8US4p6ALVdLJVJ+6RR6i9Ql++LFZTu1dADAH5DfwU
5cFoYc/xd+fyijHgW5rvgO+4OK/fllm6pAodWtMC+R1xspPpaHezU3j/W8BpBohu
pbsE6CUnIEwKswI9j8NFT48H/lrQCpl6z2a6SB6DkqWUfMPHmYWZGyk3ZqToJgNq
ce9mYEsb+7tWz6vYQ5xs05RGWb1lrfSk2MOhTKeMRl06mYkJRSrEJFIuUtl73jW7
WVpxDHdcvUeri0E6mcmMaOlg9bK+auHGTYZFwaPOQLOZAjsyOZUZOUwZvscsc+Iy
uYxyBZIm9yNTHExmAZrazwrkeBWMhvx+jEkoIurPBW8BHSL5COGAg5bi4xB3BK3t
CzpRNekI0FaCZ2XrZMwyGvGd6+RxMZXeF4751VwVxahJ30VxwY2A0OwsVUvJxQgw
dalPSLJ9UbP1VKU9OuVCHF6V0Z+9dVd4lG060G82G8H+6yFnkstOsuzDCeI1IrpO
rLXP+MSYHSlxmLjlsMeCliUBJUruXd8e1ExGGOVXDLUTxJzcNlYPwI95ZCV8WcA6
nv9bMfIjty3+A33KT7Us7fiXDI98QVFhplpajXb6m98r/L1XUi4=
=41lV
-----END PGP SIGNATURE-----

--4HkTsHVIsffrlys8--