Subject: [PATCH] Missing Assert in the code

From: Dmitry Nikitin <[email-masked]>
Date: Mon, 25 Nov 2024 10:32:27 +0300

Hello ,

  The patch is pretty trivial.

-- 
Best regards,
 Dmitry                          mailto:[email-masked]

From 0ff6a234740d7a7d2bb9572271a1dbdfd4f45f39 Mon Sep 17 00:00:00 2001
From: Dmitry Nikitin <[email-masked]>
Date: Mon, 25 Nov 2024 10:31:53 +0300
Subject: [PATCH 1/2] Get rid off the "magic number"

---
 src/include/storage/bufpage.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 6222d46e53..390d04103e 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -367,7 +367,7 @@ PageGetItem(Page page, ItemId itemId)
  *		of items on the page.
  *
  *		NOTE: if the page is not initialized (pd_lower == 0), we must
- *		return zero to ensure sane behavior.
+ *		return InvalidOffsetNumber to ensure sane behavior.
  */
 static inline OffsetNumber
 PageGetMaxOffsetNumber(Page page)
@@ -375,7 +375,7 @@ PageGetMaxOffsetNumber(Page page)
 	PageHeader	pageheader = (PageHeader) page;
 
 	if (pageheader->pd_lower <= SizeOfPageHeaderData)
-		return 0;
+		return InvalidOffsetNumber;
 	else
 		return (pageheader->pd_lower - SizeOfPageHeaderData) / sizeof(ItemIdData);
 }
-- 
2.39.5

From a75592838b2c7a8a164397c74ce1e21feb32ce44 Mon Sep 17 00:00:00 2001
From: Dmitry Nikitin <[email-masked]>
Date: Mon, 25 Nov 2024 10:32:40 +0300
Subject: [PATCH 2/2] Add Assert to stop invalid values to pass on

PageGetMaxOffsetNumber() can legitimately return zero
(InvalidOffsetNumber) as an indication of error. However there are no
any checks against that. As a result, for exampe, subsequent
PageGetItemId() will be accessing an array at -1.
---
 src/backend/access/gin/ginentrypage.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/backend/access/gin/ginentrypage.c b/src/backend/access/gin/ginentrypage.c
index 94ef951e14..193119ac22 100644
--- a/src/backend/access/gin/ginentrypage.c
+++ b/src/backend/access/gin/ginentrypage.c
@@ -236,6 +236,8 @@ getRightMostTuple(Page page)
 {
 	OffsetNumber maxoff = PageGetMaxOffsetNumber(page);
 
+	Assert(OffsetNumberIsValid(maxoff));
+
 	return (IndexTuple) PageGetItem(page, PageGetItemId(page, maxoff));
 }
 
-- 
2.39.5

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 25 Nov 2024 20:51:31 +0100

>  	return (IndexTuple) PageGetItem(page, PageGetItemId(page, maxoff));

Hmm, I think if we believe this to be really possible, we should have an
'if/elog' test (or maybe a full ereport with ERRCODE_DATA_CORRUPTED
errcode) rather than an assertion.  I think the assertion adds nothing
of value here, but maybe an 'if' test would.

Did you examine the other callers of PageGetMaxOffsetNumber()?  It's a
large bunch.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
[…] indem ich in meinem Leben oft an euch gedacht, euch glücklich zu machen. Seyd es!
A menudo he pensado en vosotros, en haceros felices. ¡Sedlo, pues!
		Heiligenstädter Testament, L. v. Beethoven, 1802
		https://de.wikisource.org/wiki/Heiligenstädter_Testament

---

From: Dmitry Nikitin <[email-masked]>
Date: Tue, 26 Nov 2024 12:24:37 +0300

Hello Alvaro,

Monday, November 25, 2024, 10:51:31 PM, you wrote:

AH> Hmm, I think if we believe this to be really possible, we should have an
AH> 'if/elog' test (or maybe a full ereport with ERRCODE_DATA_CORRUPTED
AH> errcode) rather than an assertion.  I think the assertion adds nothing
AH> of value here, but maybe an 'if' test would.
I don't mind of 'if' however it's not clear for me what to follow on other (error) branch of that 'if'.

AH> Did you examine the other callers of PageGetMaxOffsetNumber()?  It's a
AH> large bunch.
Mostly it used in loops. So some code will be gracefully skipped and that's it.
However the case discussed is different because -1 index of an array will be accessed. Which is much
worse than a bare assertion at least.

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 26 Nov 2024 12:40:39 +0100

Hello

> other (error) branch of that 'if'.

An elog(ERROR) doesn't continue execution of the function (it's a
longjmp to transaction abort and error recovery), so you don't have to
worry about that.

My point is that an Assert() is only executed in a debug build;
production builds don't have assertions.

> Mostly it used in loops. So some code will be gracefully skipped and

Yeah, I noticed that in a few that I looked at, but it's over a hundred
of them.

> However the case discussed is different because -1 index of an array
> will be accessed. Which is much worse than a bare assertion at least.

Sure.

By the way, how did you find out about this problem?  Is this
hypothetical or was there an actual crash?  AFAICS (some of?) the
callers get to this code immediately after executing GinInitPage, so
it's not clear to me that there's an actual bug here.

... Ah, maybe ginFindLeafPage could be at risk, through ->isMoveRight.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Porque Kim no hacía nada, pero, eso sí,
con extraordinario éxito" ("Kim", Kipling)