Horton

Thread

Commits

  1. Remove dead assignment to local variable.

  2. Retain original physical order of tuples in redo of b-tree splits.

  1. Protect against possible memory corruption (src/backend/access/nbtree/nbtxlog.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2021-07-11T19:51:04Z

    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.

It can also generate a dangling pointer by incrementing it beyond the
limits it can point to.

While there, I promoted a reduction of scope and adaptation of the type of
the *len* parameter to match XLogRecGetBlockData function.

pass regress check at Windows and check-world at Linux.

regards,
Ranier Vilela

  2. Re: Protect against possible memory corruption (src/backend/access/nbtree/nbtxlog.c)

    Heikki Linnakangas <hlinnaka@iki.fi> — 2021-07-11T22:19:47Z

    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

  3. Re: Protect against possible memory corruption (src/backend/access/nbtree/nbtxlog.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2021-07-11T23:34:15Z

    Em dom., 11 de jul. de 2021 às 19:19, Heikki Linnakangas <hlinnaka@iki.fi>
escreveu:

> 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.
>
Thanks for the quick review.

Not totally wrong.
If it is not possible, know the upper limits, before the loop.
It is necessary to do this inside the loop.

attached v1 of patch.

regards,
Ranier Vilela

  4. Re: Protect against possible memory corruption (src/backend/access/nbtree/nbtxlog.c)

    Heikki Linnakangas <hlinnaka@iki.fi> — 2021-07-12T08:20:57Z

    On 12/07/2021 02:34, Ranier Vilela wrote:
> If it is not possible, know the upper limits, before the loop.
> It is necessary to do this inside the loop.

> @@ -49,10 +47,14 @@ _bt_restore_page(Page page, char *from, int len)
>  	 * To get the items back in the original order, we add them to the page in
>  	 * reverse.  To figure out where one tuple ends and another begins, we
>  	 * have to scan them in forward order first.
> +	 * Check the array upper limit to not overtake him.
>  	 */
>  	i = 0;
> -	while (from < end)
> +	while (from < end && i <= MaxIndexTuplesPerPage)
>  	{
> +		IndexTupleData itupdata;
> +		Size		itemsz;
> +
>  		/*
>  		 * As we step through the items, 'from' won't always be properly
>  		 * aligned, so we need to use memcpy().  Further, we use Item (which

If we bother checking it, we should throw an error if the check fails, 
not just silently soldier on. Also, shouldn't it be '<', not '<='? In 
general though, we don't do much checking on WAL records, we assume that 
the contents are sane. It would be nice to add more checks and make WAL 
redo routines more robust to corrupt records, but this seems like an odd 
place to start.

I committed the removal of bogus assignment to 'from'. Thanks!

- Heikki

  5. Re: Protect against possible memory corruption (src/backend/access/nbtree/nbtxlog.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2021-07-12T16:17:19Z

    Em seg., 12 de jul. de 2021 às 05:20, Heikki Linnakangas <hlinnaka@iki.fi>
escreveu:

> On 12/07/2021 02:34, Ranier Vilela wrote:
> > If it is not possible, know the upper limits, before the loop.
> > It is necessary to do this inside the loop.
>
> > @@ -49,10 +47,14 @@ _bt_restore_page(Page page, char *from, int len)
> >        * To get the items back in the original order, we add them to the
> page in
> >        * reverse.  To figure out where one tuple ends and another
> begins, we
> >        * have to scan them in forward order first.
> > +      * Check the array upper limit to not overtake him.
> >        */
> >       i = 0;
> > -     while (from < end)
> > +     while (from < end && i <= MaxIndexTuplesPerPage)
> >       {
> > +             IndexTupleData itupdata;
> > +             Size            itemsz;
> > +
> >               /*
> >                * As we step through the items, 'from' won't always be
> properly
> >                * aligned, so we need to use memcpy().  Further, we use
> Item (which
>
> If we bother checking it, we should throw an error if the check fails,
> not just silently soldier on. Also, shouldn't it be '<', not '<='?

Should be '<', you are right.

In
> general though, we don't do much checking on WAL records, we assume that
> the contents are sane. It would be nice to add more checks and make WAL
> redo routines more robust to corrupt records, but this seems like an odd
> place to start.
>
If WAL records can't be corrupted at _bt_restore_page, that's ok, it's safe.


> I committed the removal of bogus assignment to 'from'. Thanks!
>
Thanks for the commit.

regards,
Ranier Vilela