[PATCH] Missing Assert in the code

Started by Dmitry Nikitinabout 1 year ago5 messages

Dmitry Nikitin

pgsql-hackers@dima.nikitin.name

about 1 year ago

2 attachment(s)

Hello ,

The patch is pretty trivial.

--
Best regards,
Dmitry mailto:pgsql-hackers@dima.nikitin.name

Attachments:

0001-Get-rid-off-the-magic-number.patchtext/x-patch; name=0001-Get-rid-off-the-magic-number.patchDownload

From 0ff6a234740d7a7d2bb9572271a1dbdfd4f45f39 Mon Sep 17 00:00:00 2001
From: Dmitry Nikitin <postgresql@dima.nikitin.name>
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

0002-Add-Assert-to-stop-invalid-values-to-pass-on.patchtext/x-patch; name=0002-Add-Assert-to-stop-invalid-values-to-pass-on.patchDownload

From a75592838b2c7a8a164397c74ce1e21feb32ce44 Mon Sep 17 00:00:00 2001
From: Dmitry Nikitin <postgresql@dima.nikitin.name>
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

Alvaro Herrera

alvherre@alvh.no-ip.org

about 1 year ago

In reply to: Dmitry Nikitin (#1)

Re: [PATCH] Missing Assert in the code

On 2024-Nov-25, Dmitry Nikitin wrote:

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.

@@ -236,6 +236,8 @@ getRightMostTuple(Page page)
{
OffsetNumber maxoff = PageGetMaxOffsetNumber(page);
+	Assert(OffsetNumberIsValid(maxoff));
+
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

Dmitry Nikitin

pgsql-hackers@dima.nikitin.name

about 1 year ago

In reply to: Alvaro Herrera (#2)

Re: [PATCH] Missing Assert in the code

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.

--
Best regards,
Dmitry mailto:pgsql-hackers@dima.nikitin.name

Alvaro Herrera

alvherre@alvh.no-ip.org

about 1 year ago

In reply to: Dmitry Nikitin (#3)

Re: [PATCH] Missing Assert in the code

Hello

On 2024-Nov-26, Dmitry Nikitin wrote:

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

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.

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.

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)

Dmitry Nikitin

pgsql-hackers@dima.nikitin.name

about 1 year ago

In reply to: Alvaro Herrera (#4)

Re: [PATCH] Missing Assert in the code

Hello Alvaro,

Tuesday, November 26, 2024, 2:40:39 PM, you wrote:

AH> By the way, how did you find out about this problem? Is this
AH> hypothetical or was there an actual crash? AFAICS (some of?) the
AH> callers get to this code immediately after executing GinInitPage, so
AH> it's not clear to me that there's an actual bug here.
Code analyzer. And we (our company) must somehow address this issue (a long story why).
I understand that sometimes some cases are pretty impossible because of the "business-logic" that
makes errors checking redundant. However sometimes that "business-logic" changes later on... So this
is a moment when debug asserts come to play and save a lot of time and sane.
Although I don't mind of elog/whatever. I'm just not that familiar with the "business-logic" to make
an appropriate choice.

--
Best regards,
Dmitry mailto:pgsql-hackers@dima.nikitin.name