Horton

Re: Assert in pageinspect with NULL pages

Julien Rouhaud <rjuju123@gmail.com>

View thread View on postgr.es
From: Julien Rouhaud <rjuju123@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alexander Lakhin <exclusion@gmail.com>, Justin Pryzby <pryzby@telsasoft.com>, Daria Lepikhova <d.lepikhova@postgrespro.ru>, pgsql-hackers@postgresql.org
Date: 2022-03-24T08:27:40Z
Lists: pgsql-hackers

Attachments

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.

Commits

  1. pageinspect: Fix handling of all-zero pages

  2. pageinspect: Add more sanity checks to prevent out-of-bound reads

  3. pageinspect: Fix handling of page sizes and AM types

  4. Teach pageinspect about nbtree deduplication.