Assert in pageinspect with NULL pages

On Thu, Feb 17, 2022 at 01:46:40PM +0500, Daria Lepikhova wrote:

INSERT INTO test1(y) SELECT 0 FROM generate_series(1,1E6) AS x;
SELECT page_checksum(repeat(E'\\000', 8192)::bytea, 1);

Indeed. Good catch, and that seems pretty old at quick glance for the
checksum part. I'll try to look at all that tomorrow.
--
Michael

On Thu, Feb 17, 2022 at 05:57:49PM +0900, Michael Paquier wrote:

On Thu, Feb 17, 2022 at 01:46:40PM +0500, Daria Lepikhova wrote:

INSERT INTO test1(y) SELECT 0 FROM generate_series(1,1E6) AS x;
SELECT page_checksum(repeat(E'\\000', 8192)::bytea, 1);

Indeed. Good catch, and that seems pretty old at quick glance for the
checksum part. I'll try to look at all that tomorrow.

Indeed, the problem in page_checksum() has been there since the beginning as
far as I can see.

About the patch, it's incorrectly using a hardcoded 8192 block-size rather than
using the computed :block_size (or computing one when it's not already the
case).

I'm also wondering if it wouldn't be better to return NULL rather than throwing
an error for uninitialized pages.

While at it, it could also be worthwhile to add tests for invalid blkno and
block size in page_checksum().

BRIN can also crash if passed a non-brin index.

I've been sitting on this one for awhile. Feel free to include it in your
patchset.

commit 08010a6037fc4e24a9ba05e5386e766f4310d35e
Author: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Tue Jan 19 00:25:15 2021 -0600

pageinspect: brin_page_items(): check that given relation is not only an index, but a brin one

diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index f1e64a39ef2..3de6dd943ef 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -16,6 +16,7 @@
 #include "access/brin_tuple.h"
 #include "access/htup_details.h"
 #include "catalog/index.h"
+#include "catalog/pg_am.h"
 #include "catalog/pg_type.h"
 #include "funcapi.h"
 #include "lib/stringinfo.h"
@@ -59,7 +60,7 @@ brin_page_type(PG_FUNCTION_ARGS)
 	if (raw_page_size != BLCKSZ)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("input page too small"),
+				 errmsg("input page wrong size"),
 				 errdetail("Expected size %d, got %d",
 						   BLCKSZ, raw_page_size)));

@@ -97,7 +98,7 @@ verify_brin_page(bytea *raw_page, uint16 type, const char *strtype)
if (raw_page_size != BLCKSZ)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("input page too small"),
+ errmsg("input page wrong size"),
errdetail("Expected size %d, got %d",
BLCKSZ, raw_page_size)));

@@ -169,7 +170,14 @@ brin_page_items(PG_FUNCTION_ARGS)
MemoryContextSwitchTo(oldcontext);

 	indexRel = index_open(indexRelid, AccessShareLock);
-	bdesc = brin_build_desc(indexRel);
+
+	/* Must be a BRIN index */
+	if (indexRel->rd_rel->relkind != RELKIND_INDEX ||
+			indexRel->rd_rel->relam != BRIN_AM_OID)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a BRIN index",
+					 RelationGetRelationName(indexRel))));

/* minimally verify the page we got */
page = verify_brin_page(raw_page, BRIN_PAGETYPE_REGULAR, "regular");
@@ -178,6 +186,7 @@ brin_page_items(PG_FUNCTION_ARGS)
* Initialize output functions for all indexed datatypes; simplifies
* calling them later.
*/
+ bdesc = brin_build_desc(indexRel);
columns = palloc(sizeof(brin_column_state *) * RelationGetDescr(indexRel)->natts);
for (attno = 1; attno <= bdesc->bd_tupdesc->natts; attno++)
{

On Thu, Feb 17, 2022 at 05:40:41PM +0800, Julien Rouhaud wrote:

About the patch, it's incorrectly using a hardcoded 8192 block-size rather than
using the computed :block_size (or computing one when it's not already the
case).

Well, the tests of pageinspect fail would already fail when using a
different page size than 8k, like the checksum ones :)

Anywa, I agree with your point that if this is not a reason to not
make the tests more portable if we can do easily.

I'm also wondering if it wouldn't be better to return NULL rather than throwing
an error for uninitialized pages.

Agreed that this is a sensible choice. NULL would be helpful for the
case where one compiles all the checksums of a relation with a full
scan based on the relation size, for example. All these behaviors
ought to be documented properly, as well. For SRFs, this should just
return no rows rather than generating an error.

While at it, it could also be worthwhile to add tests for invalid blkno and
block size in page_checksum().

If we are on that, we could also have tests for the various "input
page too small" errors. Now, these are just improvements, so I'd
rather treat this part separately of the issue reported, and add the
extra tests only on HEAD.

I can't help but notice that we should have a similar protection on
PageIsNew() in heap_page_items()? Now the code happens to work for an
empty page thanks to PageGetMaxOffsetNumber(), but this extra
protection would make the code more solid in the long-term IMO for the
full-zero case? It is worth noting that, in pageinspect, two of the
heap functions are the only ones to not be strict..

Something similar could be said about page_header() in rawpage.c,
though it is not wrong to return only zeros for a page full of zeros.

Shouldn't we similarly care about bt_metap() and
bt_page_stats_internal(), both callers of ReadBuffer(). The root
point is that the RBM_NORMAL mode of ReadBufferExtended() considers a
page full of zeros as valid, as per its top comments.

For the same reason, get_raw_page_internal() would work just fine and
return a page full of zeros.

Also, instead of an error in get_page_from_raw(), we should make sure
that all its callers are able to map with the case of a new page,
where we return 8kB worth of zeros from this inner routine.
--
Michael

On Thu, Feb 17, 2022 at 09:00:20PM -0600, Justin Pryzby wrote:

BRIN can also crash if passed a non-brin index.

I've been sitting on this one for awhile. Feel free to include it in your
patchset.

Ugh. Thanks! I am keeping a note about this one.
--
Michael

Hello Michael,

18.02.2022 06:07, Michael Paquier wrpte:

On Thu, Feb 17, 2022 at 09:00:20PM -0600, Justin Pryzby wrote:

BRIN can also crash if passed a non-brin index.

I've been sitting on this one for awhile. Feel free to include it in your
patchset.

Ugh. Thanks! I am keeping a note about this one.

Could you please confirm before committing the patchset that it fixes
the bug #16527 [1]/messages/by-id/16527-ef7606186f0610a1@postgresql.org? Or maybe I could check it?
(Original patch proposed by Daria doesn't cover that case, but if the
patch going to be improved, probably it's worth fixing that bug too.)

[1]: /messages/by-id/16527-ef7606186f0610a1@postgresql.org
/messages/by-id/16527-ef7606186f0610a1@postgresql.org

Best regards,
Alexander

On Mon, Feb 21, 2022 at 10:00:00AM +0300, Alexander Lakhin wrote:

Could you please confirm before committing the patchset that it fixes
the bug #16527 [1]? Or maybe I could check it?
(Original patch proposed by Daria doesn't cover that case, but if the
patch going to be improved, probably it's worth fixing that bug too.)

I have not directly checked, but that looks like the same issue to
me. By the way, I was waiting for an updated patch set, based on the
review provided upthread:
/messages/by-id/Yg8MPrV49/9Usqs1@paquier.xyz

And it seems better to group everything in a single commit as the same
areas are touched.
--
Michael

First of all, thanks for the review and remarks!

18.02.2022 08:02, Michael Paquier пишет:

On Thu, Feb 17, 2022 at 05:40:41PM +0800, Julien Rouhaud wrote:

About the patch, it's incorrectly using a hardcoded 8192 block-size rather than
using the computed :block_size (or computing one when it's not already the
case).

Well, the tests of pageinspect fail would already fail when using a
different page size than 8k, like the checksum ones :)

Anywa, I agree with your point that if this is not a reason to not
make the tests more portable if we can do easily.

Fixed.

I'm also wondering if it wouldn't be better to return NULL rather than throwing
an error for uninitialized pages.

Agreed that this is a sensible choice. NULL would be helpful for the
case where one compiles all the checksums of a relation with a full
scan based on the relation size, for example. All these behaviors
ought to be documented properly, as well. For SRFs, this should just
return no rows rather than generating an error.

So, I tried to implement this remark. However, further getting rid of
throwing an error and replacing it with a NULL return led to reproduce
the problem that Alexander Lakhin mentioned And here I need little more
time to figure it out.

And one more addition. In the previous version of the patch, I forgot to
add tests for the gist index, but the described problem is also relevant
for it.

--
Daria Lepikhova
Postgres Professional

18.02.2022 08:00, Justin Pryzby пишет:

BRIN can also crash if passed a non-brin index.

I've been sitting on this one for awhile. Feel free to include it in your
patchset.

commit 08010a6037fc4e24a9ba05e5386e766f4310d35e
Author: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Tue Jan 19 00:25:15 2021 -0600

pageinspect: brin_page_items(): check that given relation is not only an index, but a brin one

diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index f1e64a39ef2..3de6dd943ef 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -16,6 +16,7 @@
#include "access/brin_tuple.h"
#include "access/htup_details.h"
#include "catalog/index.h"
+#include "catalog/pg_am.h"
#include "catalog/pg_type.h"
#include "funcapi.h"
#include "lib/stringinfo.h"
@@ -59,7 +60,7 @@ brin_page_type(PG_FUNCTION_ARGS)
if (raw_page_size != BLCKSZ)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("input page too small"),
+				 errmsg("input page wrong size"),
errdetail("Expected size %d, got %d",
BLCKSZ, raw_page_size)));

@@ -97,7 +98,7 @@ verify_brin_page(bytea *raw_page, uint16 type, const char *strtype)
if (raw_page_size != BLCKSZ)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("input page too small"),
+ errmsg("input page wrong size"),
errdetail("Expected size %d, got %d",
BLCKSZ, raw_page_size)));

@@ -169,7 +170,14 @@ brin_page_items(PG_FUNCTION_ARGS)
MemoryContextSwitchTo(oldcontext);

indexRel = index_open(indexRelid, AccessShareLock);
-	bdesc = brin_build_desc(indexRel);
+
+	/* Must be a BRIN index */
+	if (indexRel->rd_rel->relkind != RELKIND_INDEX ||
+			indexRel->rd_rel->relam != BRIN_AM_OID)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a BRIN index",
+					 RelationGetRelationName(indexRel))));

/* minimally verify the page we got */
page = verify_brin_page(raw_page, BRIN_PAGETYPE_REGULAR, "regular");
@@ -178,6 +186,7 @@ brin_page_items(PG_FUNCTION_ARGS)
* Initialize output functions for all indexed datatypes; simplifies
* calling them later.
*/
+ bdesc = brin_build_desc(indexRel);
columns = palloc(sizeof(brin_column_state *) * RelationGetDescr(indexRel)->natts);
for (attno = 1; attno <= bdesc->bd_tupdesc->natts; attno++)
{

Thanks! This is a very valuable note. I think I will definitely add it
to the next version of the patch, as soon as I deal with the error
exception.

--

Daria Lepikhova
Postgres Professional

On Thu, Feb 17, 2022 at 09:00:20PM -0600, Justin Pryzby wrote:

BRIN can also crash if passed a non-brin index.

I've been sitting on this one for awhile. Feel free to include it in your
patchset.

So, I have begun a close lookup of this thread, and the problem you
are reporting here is worth fixing on its own, before we do something
for new pages as reported originally. There is more that caught my
attention than the brin AM not being check properly, because a couple
of code paths are fuzzy with the checks on the page sizes. My
impression of the whole thing is that we'd better generalize the use
of get_page_from_raw(), improving the code on many sides when the user
can provide a raw page in input (also I'd like to think that it is a
better practice anyway as any of those functions may finish to look
8-byte fields, but the current coding would silently break in
alignment-picky environments):
- Some code paths (hash, btree) would trigger elog(ERROR) but we
cannot use that for errors that can be triggered by the user.
- bt_page_items_bytea(), page_header() and fsm_page_contents() were
fuzzy on the page size check, so it was rather easy to generate
garbage with random data.
- page_checksum_internal() used a PageHeader, where I would have
expected a Page.
- More consistency in the error strings, meaning less contents to
translate in the long-term.

This first batch leads me to the attached, with tests to stress all
that for all the functions taking raw pages in input.
--
Michael

On Tue, Mar 15, 2022 at 06:32:44PM +0900, Michael Paquier wrote:

+	if (!IS_BRIN(indexRel))
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a %s index",
+						RelationGetRelationName(indexRel), "BRIN")));

If it were me, I'd write this without the extra parens around (errcode()).

+-- Suppress the DETAIL message, to allow the tests to work across various
+-- default page sizes.

I think you mean "various non-default page sizes" or "various page sizes".

--
Justin

On Tue, Mar 15, 2022 at 06:56:46AM -0500, Justin Pryzby wrote:

On Tue, Mar 15, 2022 at 06:32:44PM +0900, Michael Paquier wrote:

+-- Suppress the DETAIL message, to allow the tests to work across various
+-- default page sizes.

I think you mean "various non-default page sizes" or "various page sizes".

Thanks. Indeed, this sounded a bit weird. I have been able to look
at all that this morning and backpatched this first part.
--
Michael

On Mon, Feb 21, 2022 at 10:00:00AM +0300, Alexander Lakhin wrote:

Could you please confirm before committing the patchset that it fixes
the bug #16527 [1]? Or maybe I could check it?
(Original patch proposed by Daria doesn't cover that case, but if the
patch going to be improved, probably it's worth fixing that bug too.)

[1]
/messages/by-id/16527-ef7606186f0610a1@postgresql.org

FWIW, I think that this one has been fixed by 076f4d9, where we make
sure that the page is correctly aligned. Are you still seeing this
problem?
--
Michael

On Wed, Feb 23, 2022 at 12:09:02PM +0500, Daria Lepikhova wrote:

And one more addition. In the previous version of the patch, I forgot to add
tests for the gist index, but the described problem is also relevant for it.

So, I have looked at this second part of the thread, and concluded
that we should not fail for empty pages. First, we fetch pages from
the buffer pool in normal mode, where empty pages are valid. There is
also a second point in favor of doing so: code paths dedicated to hash
indexes already do that, marking such pages as simply "unused". The
proposal from Julien upthread sounds cleaner to me though in the long
run, as NULL gives the user the possibility to do a full-table scan
with simple clauses to filter out anything found as NULL.

Painting more PageIsNew() across the place requires a bit more work
than a simple ereport(ERROR) in get_page_from_raw(), of course, but
the gain is the portability of the functions.

(One can have a lot of fun playing with random inputs and breaking
most code paths, but that's not new.)
--
Michael

Hello Michael,
16.03.2022 10:39, Michael Paquier wrote:

On Mon, Feb 21, 2022 at 10:00:00AM +0300, Alexander Lakhin wrote:

Could you please confirm before committing the patchset that it fixes
the bug #16527 [1]? Or maybe I could check it?
(Original patch proposed by Daria doesn't cover that case, but if the
patch going to be improved, probably it's worth fixing that bug too.)

[1]
/messages/by-id/16527-ef7606186f0610a1@postgresql.org

FWIW, I think that this one has been fixed by 076f4d9, where we make
sure that the page is correctly aligned. Are you still seeing this
problem?

Yes, I've reproduced that issue on master (46d9bfb0).
pageinspect-zeros-v2.patch doesn't help too.

Best regards.
Alexander

On Wed, Mar 16, 2022 at 08:35:59PM +0300, Alexander Lakhin wrote:

Yes, I've reproduced that issue on master (46d9bfb0).

Okay. I'll try to look more closely at that, then. It feels like we
are just missing a MAXALIGN() in those code paths. Are you using any
specific CFLAGS or something environment-sensitive (macos, 32b, etc.)?

pageinspect-zeros-v2.patch doesn't help too.

Well, the scope is different, so that's not a surprise.
--
Michael

Hello Michael,
18.03.2022 05:06, Michael Paquier wrote:

Okay. I'll try to look more closely at that, then. It feels like we
are just missing a MAXALIGN() in those code paths. Are you using any
specific CFLAGS or something environment-sensitive (macos, 32b, etc.)?

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.

Best regards,
Alexander

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.
--
Michael

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.