Re: Assert in pageinspect with NULL pages

Hi,

On Wed, Mar 23, 2022 at 05:16:41PM +0900, Michael Paquier wrote:
> On Fri, Mar 18, 2022 at 06:43:39AM +0300, Alexander Lakhin wrote:
> > Hello Michael,
> > No, just x86_64, Ubuntu 20.04, gcc 11, valgrind 3.15. I just put that query
> > in page.sql and see the server abort.
>
> Bah, of course, I have missed the valgrind part of the problem.
>
> The problem is that we attempt to verify a heap page here but its
> pd_special is BLCKSZ. This causes BrinPageType() to grab back a
> position of the area at the exact end of the page, via
> PageGetSpecialPointer(), hence the failure in reading two bytes
> outside the page. The result here is that the set of defenses in
> verify_brin_page() is poor: we should at least make sure that the
> special area is available for a read. As far as I can see, this is
> also possible in bt_page_items_bytea(), gist_page_opaque_info(),
> gin_metapage_info() and gin_page_opaque_info(). All those code paths
> should be protected with some checks on PageGetSpecialSize(), I
> guess, before attempting to read the special area of the page. Hash
> indexes are protected by checking the expected size of the special
> area, and one code path of btree relies on the opened relation to be a
> btree index.

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.

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.