Thread: 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

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