Re: Assert in pageinspect with NULL pages

On Thu, Mar 24, 2022 at 04:27:40PM +0800, Julien Rouhaud wrote:
> I worked on a patch to fix the problem. The functions you mention were indeed
> missing some check, but after digging a bit more I found that other problem
> existed. For instance, feeding a btree page to a gist_page_items_bytea() (both
> pages have the same special size) can be quite problematic too, as you can end
> up accessing bogus data when retrieving the items. I also added additional
> sanity checks with what is available (gist_page_id for gist, zero-level for
> btree leaf pages and so on), and tried to cover everything with tests.

Thanks for the patch!

I have reviewed what you have sent, bumping on a couple of issues:
- The tests of btree and BRIN failed with 32-bit builds, because
MAXALIGN returns shorter special area sizes in those cases. This can
be fixed by abusing of \set VERBOSITY to mask the error details. We
already do that in some of the tests to make them portable.
- Some of the tests were not necessary, overlapping with stuff that
already existed.
- Some checks missed a MAXALIGN().
- Did some tweaks with the error messages.
- Some error messages used an incorrect term to define the index AM
type, aka s/gist/GiST/ or s/brin/BRIN/.
- errdetail() requires a sentence, finishing with a dot (some places
of hashfuncs.c missed that.
- Not your fault, but hash indexes would complain about corrupted
indexes which could be misleading for the user if passing down a
correct page from an incorrect index AM.
- While on it, I have made the error messages more generic in the
places where I could do so. What I have finished with seems to have a
good balance.

> I'm a bit worried about the btree tests stability. I avoid emitting the level
> found to help with that, but it still depends on what other AM will put in
> their special page.

Well, the limit of the pageinspect model comes from the fact that it
is possible to pass down any bytea and all those code paths would
happily process the blobs as long as they are 8kB. Pages can be
crafted as well to bypass some of the checks. This is superuser-only,
so people have to be careful, but preventing out-of-bound reads is a
different class of problem, as long as these come from valid pages.

With all that in place, I get the attached. It is Friday, so I am not
going to take my bets on the buildfarm today with a rather-risky
patch. Monday/Tuesday will be fine.
--
Michael