On 2020-11-25 20:04, Alvaro Herrera wrote:
On 2020-Nov-25, Peter Eisentraut wrote:
bt_page_stats(PG_FUNCTION_ARGS)
{
text *relname = PG_GETARG_TEXT_PP(0);
- uint32 blkno = PG_GETARG_UINT32(1);
+ int64 blkno = PG_GETARG_INT64(1);
As a matter of style, I think it'd be better to have an int64 variable
that gets the value from PG_GETARG_INT64(), then you cast that to
another variable that's a BlockNumber and use that throughout the rest
of the code. So you'd avoid changes like this:
static bytea *get_raw_page_internal(text *relname, ForkNumber forknum,
-
BlockNumber blkno);
+ int64
blkno);
where the previous coding was correct, and the new one is dubious and it
forces you to add unnecessary range checks in that function:
@@ -144,11 +144,16 @@ get_raw_page_internal(text *relname, ForkNumber forknum,
BlockNumber blkno)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary tables of other
sessions")));
+ if (blkno < 0 || blkno > MaxBlockNumber)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number")));
+
The point of the patch is to have the range check somewhere. If you
just cast it, then you won't notice out of range arguments. Note that
other contrib modules that take block numbers work the same way.
