Fix bogus casting in BlockIdGetBlockNumber().

tglsfdc · tglsfdc · commit 0fbdfaf79d0b · 2022-03-03T19:03:17.000-05:00
This macro cast the result to BlockNumber after shifting, not before, which is the wrong thing. Per the C spec, the uint16 fields would promote to int not unsigned int, so that (for 32-bit int) the shift potentially shifts a nonzero bit into the sign position. I doubt there are any production systems where this would actually end with the wrong answer, but it is undefined behavior per the C spec, and clang's -fsanitize=undefined option reputedly warns about it on some platforms. (I can't reproduce that right now, but the code is undeniably wrong per spec.) It's easy to fix by casting to BlockNumber (uint32) in the proper places. It's been wrong for ages, so back-patch to all supported branches. Report and patch by Zhihong Yu (cosmetic tweaking by me) Discussion: https://postgr.es/m/CALNJ-vT9r0DSsAOw9OXVJFxLENoVS_68kJ5x0p44atoYH+H4dg@mail.gmail.com
diff --git a/src/include/storage/block.h b/src/include/storage/block.h
@@ -115,7 +115,7 @@ typedef BlockIdData *BlockId;	/* block identifier */
 #define BlockIdGetBlockNumber(blockId) \
 ( \
 	AssertMacro(BlockIdIsValid(blockId)), \
-	(BlockNumber) (((blockId)->bi_hi << 16) | ((uint16) (blockId)->bi_lo)) \
+	((((BlockNumber) (blockId)->bi_hi) << 16) | ((BlockNumber) (blockId)->bi_lo)) \
 )
 
 #endif							/* BLOCK_H */

Original file line number	Diff line number	Diff line change
`@@ -115,7 +115,7 @@ typedef BlockIdData BlockId; / block identifier */`
`115`	`115`	`#define BlockIdGetBlockNumber(blockId) \`
`116`	`116`	`( \`
`117`	`117`	`AssertMacro(BlockIdIsValid(blockId)), \`
`118`		`- (BlockNumber) (((blockId)->bi_hi << 16) \| ((uint16) (blockId)->bi_lo)) \`
	`118`	`+ ((((BlockNumber) (blockId)->bi_hi) << 16) \| ((BlockNumber) (blockId)->bi_lo)) \`
`119`	`119`	`)`
`120`	`120`
`121`	`121`	`#endif /* BLOCK_H */`