On 2018-08-30 14:46:06 -0700, Andres Freund wrote:
> Hi,
>
> On 2018-08-30 17:19:28 -0400, Tom Lane wrote:
> > So, I've been fooling around trying to get it to work without
> > -fno-strict-aliasing, but with little luck so far.
>
> The problem presumably is that pg_checksum_block() accesses the relevant
> fields as an uint32, whereas pg_checksum_page() accesses it as a
> PageHeader. That's an aliasing violation. *One* cast from char* to
> either type is fine, it's accessing under both those types that's
> problematic.
>
> One way to fix it would be to memcpy in/out the modified PageHeader, or
> just do offset math and memcpy to that offset.
It took me a bit to reproduce the issue (due to sheer stupidity on my
part: no, changing the flags passed to gcc to link pg_verify_checksums
doesn't do the trick), but the above indeed fixes the issue for me.
The attached is just for demonstration that the approach works.
Greetings,
Andres Freund
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 7cf3cf35c7a..c23b0dd5c37 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -78,7 +78,7 @@ skipfile(char *fn)
static void
scan_file(char *fn, int segmentno)
{
- char buf[BLCKSZ];
+ char *buf = palloc(BLCKSZ);
PageHeader header = (PageHeader) buf;
int f;
int blockno;
@@ -126,6 +126,8 @@ scan_file(char *fn, int segmentno)
}
close(f);
+
+ pfree(buf);
}
static void
diff --git a/src/include/storage/checksum_impl.h b/src/include/storage/checksum_impl.h
index 64d76229add..a258ea8d117 100644
--- a/src/include/storage/checksum_impl.h
+++ b/src/include/storage/checksum_impl.h
@@ -178,12 +178,14 @@ pg_checksum_block(char *data, uint32 size)
uint16
pg_checksum_page(char *page, BlockNumber blkno)
{
- PageHeader phdr = (PageHeader) page;
+ PageHeaderData phdr;
uint16 save_checksum;
uint32 checksum;
+ memcpy(&phdr, page, sizeof(PageHeaderData));
+
/* We only calculate the checksum for properly-initialized pages */
- Assert(!PageIsNew(page));
+ Assert(!PageIsNew(&page));
/*
* Save pd_checksum and temporarily set it to zero, so that the checksum
@@ -191,10 +193,14 @@ pg_checksum_page(char *page, BlockNumber blkno)
* Restore it after, because actually updating the checksum is NOT part of
* the API of this function.
*/
- save_checksum = phdr->pd_checksum;
- phdr->pd_checksum = 0;
+ save_checksum = phdr.pd_checksum;
+ phdr.pd_checksum = 0;
+ memcpy(page, &phdr, sizeof(PageHeaderData));
+
checksum = pg_checksum_block(page, BLCKSZ);
- phdr->pd_checksum = save_checksum;
+
+ phdr.pd_checksum = save_checksum;
+ memcpy(page, &phdr, sizeof(PageHeaderData));
/* Mix in the block number to detect transposed pages */
checksum ^= blkno;
