[PATCH] pg_surgery: check the page header and line pointers

Hi Shihao,

The maxoffset > MaxHeapTuplesPerPage check looks correct to me and fixes
the reported access past include_this_tid[].

I think heap_force_freeze() needs one more check. PageIsVerified()
validates the basic page header ordering, but not individual line
pointers, and ItemIdIsNormal() only checks the line pointer state. An
LP_NORMAL item can still point outside the heap tuple area, while
PageGetItem() does not validate its offset or length. We then modify a
HeapTupleHeader at that address.

Before accepting an item for freezing, I think we should at least verify
that its length can hold a tuple header, its offset is MAXALIGNed, and the
tuple fits between pd_upper and pd_special. heap_force_kill() does not
dereference the tuple, so it probably should remain able to mark such a
line pointer dead.

The test's hardcoded 291 is MaxHeapTuplesPerPage only with the default
BLCKSZ. I think it's fine - a lot of tests fail with other block sizes.
More importantly, the test does not exercise the new invalid-header
branch, so it would pass without the code change on a default build.
I suggest omitting it unless we can add a test with an actually corrupted
header. See amcheck tests fro details, but I'm not sure it actually
worth the trouble here.

Thank you!

Best regards, Andrey Borodin.

Hi Andrey,

Thanks for reviewing!

I think heap_force_freeze() needs one more check.

I agree with you, check added

See amcheck tests fro details, but I'm not sure it actually
worth the trouble here.

In my opinion, every change we made should have some
kind of regression tests. I understand there are resource
limit for machines but I believe engineering wise this is the
right way to do.
I added a TAP test to simulate a corrupt page.

Thanks
Shihao

Thanks, v2 addresses both points. The code changes look good to me.

The TAP test still has one small portability issue: it hardcodes 8192.
It does not need to support every BLCKSZ, but it could read SHOW
block_size and skip unless it is 8192. Otherwise a 4 kB build rejects
the header in PageIsVerified() before reaching the new check.

  +# t_lp: leave the header alone, but point the first line pointer past the
  +# end of the page.  Item is (lp_off = 32767, lp_flags = LP_NORMAL,
  +# lp_len = 100) packed into the 32-bit ItemIdData word.

The exact lp_off and lp_len values produced by the packed ItemIdData
word depend on the platform's bit-field ordering. The word is invalid
with either ordering, so the test is fine, but the comment can avoid
claiming those exact values.

With those minor test adjustments, this looks ready for committer to me.

Best regards, Andrey Borodin.

Hi Andrey,

Otherwise a 4 kB build rejects
the header in PageIsVerified() before reaching the new check.

Haven't thought about that, updated tests with dynamic block_size.

the comment can avoid
claiming those exact values.

Also haven't thought about that, updated.

Thanks,
Shihao

Hi Shihao,

I reviewed the v3 patch, and it looks good to me. I have one optional
thought about the malformed line-pointer test:

Depending on the platform’s bit-field ordering, the encoded word may
produce an lp_off of either 32767 or 100. Since both values can be
misaligned on a build with 8-byte MAXALIGN, the condition may
short-circuit at the alignment check without exercising the
page-boundary checks. Would it make sense to use aligned values such
as 32736 and 128, so that either bit-field interpretation reaches a
boundary check? This is not a correctness issue with the patch, just a
possible way to make the test more targeted.

Otherwise, the patch looks good to me.

Best Regards,
Nitin Jadhav
Azure Database for PostgreSQL
Microsoft

Hi Nitin,

Thanks for the review, and good catch.

Since both values can be
misaligned on a build with 8-byte MAXALIGN, the condition may
short-circuit at the alignment check without exercising the
page-boundary checks. Would it make sense to use aligned values such
as 32736 and 128,so that either bit-field interpretation reaches a
boundary check?

You are right. With the old value, lp_off decodes as either 32767 or
100, and neither one is a multiple of 8. So on a build with 8-byte
MAXALIGN the code stops at the alignment check and never reaches the
pd_upper / pd_special checks. The test looked like it passed, but it
only covered one of the three branches.

v4 uses 32736 and 128 as you suggested. Both are MAXALIGNed and both
are at least as long as a heap tuple header, so the code always gets
as far as a page boundary check

I also make a few cosmetic changes for errcode and variable declaration.

Given we had 2 LGTM for this patch, I will mark it Ready For Committer

Thanks,
Shihao