On Wed, Sep 23, 2020 at 1:41 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> Robert Haas <robertmh...@gmail.com> writes:
> > On Mon, Sep 21, 2020 at 3:53 PM Andres Freund <and...@anarazel.de> wrote:
> >> I think we mostly use it for the few places where we currently expose
> >> data as a signed integer on the SQL level, but internally actually treat
> >> it as a unsigned data.
>
> > So why is the right solution to that not DatumGetInt32() + a cast to uint32?
>
> You're ignoring the xid use-case, for which DatumGetUInt32 actually is
> the right thing.
There is DatumGetTransactionId() which should be used instead.
That made me search if there's PG_GETARG_TRANSACTIONID() and yes it's
there but only defined in xid.c. So pg_xact_commit_timestamp(),
pg_xact_commit_timestamp_origin() and pg_get_multixact_members() use
PG_GETARG_UNIT32. IMO those should be changed to use
PG_GETARG_TRANSACTIONID. That would require moving
PG_GETARG_TRANSACTIONID somewhere outside xid.c; may be fmgr.h where
other PG_GETARG_* are.
> I tend to agree though that if the SQL argument is
> of a signed type, the least API-abusing answer is a signed DatumGetXXX
> macro followed by whatever cast you need.
>
I looked for some uses of PG_GETARG_UNIT32() which is the counterpart
of DatumGetUint32(). Found some buggy usages apart from the ones which
can be converted to PG_GETARG_TRANSACTIONID listed above.
normal_rand() for example returns a huge number of rows and takes
forever if we pass a negative first argument to it. Someone could
misuse that for a DOS attack or it could be just an accident that they
pass a negative value to that function and the query takes forever.
explain analyze select count(*) from normal_rand(-1000000, 1.0, 1.0);
QUERY
PLAN
-----------------------------------------------------------------------------------------------------------------------------------------
Aggregate (cost=12.50..12.51 rows=1 width=8) (actual
time=2077574.718..2077574.719 rows=1 loops=1)
-> Function Scan on normal_rand (cost=0.00..10.00 rows=1000
width=0) (actual time=1005176.149..1729994.366 rows=4293967296
loops=1)
Planning Time: 0.346 ms
Execution Time: 2079034.835 ms
get_raw_page() also does similar thing but the effect is not as dangerous
SELECT octet_length(get_raw_page('test1', 'main', -1)) AS main_1;
ERROR: block number 4294967295 is out of range for relation "test1"
Similarly for bt_page_stats() and bt_page_items()
PFA patches to correct those.
There's Oracle compatible chr() which also uses PG_GETARG_UINT32() but
it's (accidentally?) reporting the negative inputs correctly because
it filters out very large values and reports those using %d. It's
arguable whether we should change that, so I have left it untouched.
But I think we should change that as well and get rid of
PG_GETARG_UNIT32 altogether. This will prevent any future misuse.
--
Best Wishes,
Ashutosh Bapat
From e13eeafc452a977a1e5b9e6c51b043f00dd2d5c7 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com>
Date: Tue, 22 Sep 2020 19:00:56 +0530
Subject: [PATCH 1/2] Handle negative number of tuples passed to normal_rand()
The function converts the first argument i.e. the number of tuples to
return into an unsigned integer which turns out to be huge number when a
negative value is passed. This causes the function to take much longer
time to execute. Instead return no rows in that case.
While at it, improve SQL test to test the number of tuples returned by
this function.
Ashutosh Bapat
---
contrib/tablefunc/expected/tablefunc.out | 15 +++++++++++----
contrib/tablefunc/sql/tablefunc.sql | 4 +++-
contrib/tablefunc/tablefunc.c | 4 +++-
3 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/contrib/tablefunc/expected/tablefunc.out b/contrib/tablefunc/expected/tablefunc.out
index fffadc6e1b..1c9ecef52c 100644
--- a/contrib/tablefunc/expected/tablefunc.out
+++ b/contrib/tablefunc/expected/tablefunc.out
@@ -3,10 +3,17 @@ CREATE EXTENSION tablefunc;
-- normal_rand()
-- no easy way to do this for regression testing
--
-SELECT avg(normal_rand)::int FROM normal_rand(100, 250, 0.2);
- avg
------
- 250
+SELECT avg(normal_rand)::int, count(*) FROM normal_rand(100, 250, 0.2);
+ avg | count
+-----+-------
+ 250 | 100
+(1 row)
+
+-- negative number of tuples
+SELECT avg(normal_rand)::int, count(*) FROM normal_rand(-1, 250, 0.2);
+ avg | count
+-----+-------
+ | 0
(1 row)
--
diff --git a/contrib/tablefunc/sql/tablefunc.sql b/contrib/tablefunc/sql/tablefunc.sql
index ec375b05c6..02e8a98c73 100644
--- a/contrib/tablefunc/sql/tablefunc.sql
+++ b/contrib/tablefunc/sql/tablefunc.sql
@@ -4,7 +4,9 @@ CREATE EXTENSION tablefunc;
-- normal_rand()
-- no easy way to do this for regression testing
--
-SELECT avg(normal_rand)::int FROM normal_rand(100, 250, 0.2);
+SELECT avg(normal_rand)::int, count(*) FROM normal_rand(100, 250, 0.2);
+-- negative number of tuples
+SELECT avg(normal_rand)::int, count(*) FROM normal_rand(-1, 250, 0.2);
--
-- crosstab()
diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c
index 02f02eab57..fd327b1c41 100644
--- a/contrib/tablefunc/tablefunc.c
+++ b/contrib/tablefunc/tablefunc.c
@@ -174,6 +174,7 @@ normal_rand(PG_FUNCTION_ARGS)
FuncCallContext *funcctx;
uint64 call_cntr;
uint64 max_calls;
+ int32 num_tuples;
normal_rand_fctx *fctx;
float8 mean;
float8 stddev;
@@ -193,7 +194,8 @@ normal_rand(PG_FUNCTION_ARGS)
oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
/* total number of tuples to be returned */
- funcctx->max_calls = PG_GETARG_UINT32(0);
+ num_tuples = PG_GETARG_INT32(0);
+ funcctx->max_calls = num_tuples > 0 ? num_tuples : 0;
/* allocate memory for user context */
fctx = (normal_rand_fctx *) palloc(sizeof(normal_rand_fctx));
--
2.17.1
From 66814d10a5bf1c2720a56da170cde916e89797da Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com>
Date: Wed, 23 Sep 2020 10:49:10 +0530
Subject: [PATCH 2/2] Negative block number passed to functions in pageinspect
module
When a negative integer is passed to get_raw_page(), bt_page_stats() and
bt_page_inspect() they report an invalid block number error with a bogus
number. This is because the input is casted to an unsigned integer which
for negative numbers happens to be a large value. This, at the worst, is
confusing. Accept the intput as a signed integer and report an error
with negative input as is.
Ashutosh Bapat.
---
contrib/pageinspect/btreefuncs.c | 16 ++++++++++++++--
contrib/pageinspect/expected/btree.out | 4 ++++
contrib/pageinspect/expected/page.out | 4 ++++
contrib/pageinspect/rawpage.c | 18 +++++++++++++++---
contrib/pageinspect/sql/btree.sql | 2 ++
contrib/pageinspect/sql/page.sql | 2 ++
6 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c
index 445605db58..494c711801 100644
--- a/contrib/pageinspect/btreefuncs.c
+++ b/contrib/pageinspect/btreefuncs.c
@@ -164,7 +164,7 @@ Datum
bt_page_stats(PG_FUNCTION_ARGS)
{
text *relname = PG_GETARG_TEXT_PP(0);
- uint32 blkno = PG_GETARG_UINT32(1);
+ int32 blkno = PG_GETARG_INT32(1);
Buffer buffer;
Relation rel;
RangeVar *relrv;
@@ -200,6 +200,12 @@ bt_page_stats(PG_FUNCTION_ARGS)
if (blkno == 0)
elog(ERROR, "block 0 is a meta page");
+ if (blkno < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number %d",
+ blkno)));
+
CHECK_RELATION_BLOCK_RANGE(rel, blkno);
buffer = ReadBuffer(rel, blkno);
@@ -409,7 +415,7 @@ Datum
bt_page_items(PG_FUNCTION_ARGS)
{
text *relname = PG_GETARG_TEXT_PP(0);
- uint32 blkno = PG_GETARG_UINT32(1);
+ int32 blkno = PG_GETARG_INT32(1);
Datum result;
FuncCallContext *fctx;
MemoryContext mctx;
@@ -420,6 +426,12 @@ bt_page_items(PG_FUNCTION_ARGS)
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to use pageinspect functions")));
+ if (blkno < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number %d",
+ blkno)));
+
if (SRF_IS_FIRSTCALL())
{
RangeVar *relrv;
diff --git a/contrib/pageinspect/expected/btree.out b/contrib/pageinspect/expected/btree.out
index 17bf0c5470..1ee0a6d791 100644
--- a/contrib/pageinspect/expected/btree.out
+++ b/contrib/pageinspect/expected/btree.out
@@ -32,6 +32,8 @@ btpo_flags | 3
SELECT * FROM bt_page_stats('test1_a_idx', 2);
ERROR: block number out of range
+SELECT * FROM bt_page_stats('test1_a_idx', -1);
+ERROR: invalid block number -1
SELECT * FROM bt_page_items('test1_a_idx', 0);
ERROR: block 0 is a meta page
SELECT * FROM bt_page_items('test1_a_idx', 1);
@@ -48,6 +50,8 @@ tids |
SELECT * FROM bt_page_items('test1_a_idx', 2);
ERROR: block number out of range
+SELECT * FROM bt_page_items('test1_a_idx', -1);
+ERROR: invalid block number -1
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 0));
ERROR: block is a meta page
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 1));
diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index b6aea0124b..66de360eed 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -12,6 +12,8 @@ SELECT octet_length(get_raw_page('test1', 'main', 0)) AS main_0;
SELECT octet_length(get_raw_page('test1', 'main', 1)) AS main_1;
ERROR: block number 1 is out of range for relation "test1"
+SELECT octet_length(get_raw_page('test1', 'main', -1)) AS main_1;
+ERROR: invalid block number -1
SELECT octet_length(get_raw_page('test1', 'fsm', 0)) AS fsm_0;
fsm_0
-------
@@ -49,6 +51,8 @@ SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
8192 | 4
(1 row)
+SELECT pagesize, version FROM page_header(get_raw_page('test1', -1));
+ERROR: invalid block number -1
SELECT page_checksum(get_raw_page('test1', 0), 0) IS NOT NULL AS silly_checksum_test;
silly_checksum_test
---------------------
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index c0181506a5..9035a50342 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -46,7 +46,7 @@ Datum
get_raw_page(PG_FUNCTION_ARGS)
{
text *relname = PG_GETARG_TEXT_PP(0);
- uint32 blkno = PG_GETARG_UINT32(1);
+ int32 blkno = PG_GETARG_INT32(1);
bytea *raw_page;
/*
@@ -59,7 +59,13 @@ get_raw_page(PG_FUNCTION_ARGS)
(errmsg("wrong number of arguments to get_raw_page()"),
errhint("Run the updated pageinspect.sql script.")));
- raw_page = get_raw_page_internal(relname, MAIN_FORKNUM, blkno);
+ if (blkno < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number %d",
+ blkno)));
+
+ raw_page = get_raw_page_internal(relname, MAIN_FORKNUM, (uint32) blkno);
PG_RETURN_BYTEA_P(raw_page);
}
@@ -76,10 +82,16 @@ get_raw_page_fork(PG_FUNCTION_ARGS)
{
text *relname = PG_GETARG_TEXT_PP(0);
text *forkname = PG_GETARG_TEXT_PP(1);
- uint32 blkno = PG_GETARG_UINT32(2);
+ int32 blkno = PG_GETARG_INT32(2);
bytea *raw_page;
ForkNumber forknum;
+ if (blkno < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number %d",
+ blkno)));
+
forknum = forkname_to_number(text_to_cstring(forkname));
raw_page = get_raw_page_internal(relname, forknum, blkno);
diff --git a/contrib/pageinspect/sql/btree.sql b/contrib/pageinspect/sql/btree.sql
index 8eac64c7b3..ef581f4838 100644
--- a/contrib/pageinspect/sql/btree.sql
+++ b/contrib/pageinspect/sql/btree.sql
@@ -9,10 +9,12 @@ SELECT * FROM bt_metap('test1_a_idx');
SELECT * FROM bt_page_stats('test1_a_idx', 0);
SELECT * FROM bt_page_stats('test1_a_idx', 1);
SELECT * FROM bt_page_stats('test1_a_idx', 2);
+SELECT * FROM bt_page_stats('test1_a_idx', -1);
SELECT * FROM bt_page_items('test1_a_idx', 0);
SELECT * FROM bt_page_items('test1_a_idx', 1);
SELECT * FROM bt_page_items('test1_a_idx', 2);
+SELECT * FROM bt_page_items('test1_a_idx', -1);
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 0));
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 1));
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index bd049aeb24..05170e34fd 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -10,6 +10,7 @@ VACUUM test1; -- set up FSM
SELECT octet_length(get_raw_page('test1', 'main', 0)) AS main_0;
SELECT octet_length(get_raw_page('test1', 'main', 1)) AS main_1;
+SELECT octet_length(get_raw_page('test1', 'main', -1)) AS main_1;
SELECT octet_length(get_raw_page('test1', 'fsm', 0)) AS fsm_0;
SELECT octet_length(get_raw_page('test1', 'fsm', 1)) AS fsm_1;
@@ -23,6 +24,7 @@ SELECT octet_length(get_raw_page('test1', 'xxx', 0));
SELECT get_raw_page('test1', 0) = get_raw_page('test1', 'main', 0);
SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
+SELECT pagesize, version FROM page_header(get_raw_page('test1', -1));
SELECT page_checksum(get_raw_page('test1', 0), 0) IS NOT NULL AS silly_checksum_test;
--
2.17.1
