Re: Protect against possible memory corruption (src/backend/access/nbtree/nbtxlog.c) - Mailing list pgsql-hackers

On 11/07/2021 22:51, Ranier Vilela wrote:
> Hi,
> 
> While analyzing a possible use of an uninitialized variable, I checked that
> *_bt_restore_page* can lead to memory corruption,
> by not checking the maximum limit of array items which is
> MaxIndexTuplesPerPage.

> +    /* Protect against corrupted recovery file */
> +    nitems = (len / sizeof(IndexTupleData));
> +    if (nitems < 0 || nitems > MaxIndexTuplesPerPage)
> +        elog(PANIC, "_bt_restore_page: cannot restore %d items to page", nitems);
> +

That's not right. You don't get the number of items by dividing like 
that. 'len' includes the tuple data as well, not just the IndexTupleData 
header.

> @@ -73,12 +79,9 @@ _bt_restore_page(Page page, char *from, int len)
>      nitems = i;
>  
>      for (i = nitems - 1; i >= 0; i--)
> -    {
>          if (PageAddItem(page, items[i], itemsizes[i], nitems - i,
>                          false, false) == InvalidOffsetNumber)
>              elog(PANIC, "_bt_restore_page: cannot add item to page");
> -        from += itemsz;
> -    }
>  }

I agree with this change (except that I would leave the braces in 
place). The 'from' that's calculated here is plain wrong; oversight in 
commit 7e30c186da. Fortunately it's not used, so it can just be removed.

- Heikki