database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-06-25 01:02:05 +03:00
Code Activity

Fix type-safety problem with parallel aggregate serial/deserialization.

Browse Source 
The original specification for this called for the deserialization function
to have signature "deserialize(serialtype) returns transtype", which is a
security violation if transtype is INTERNAL (which it always would be in
practice) and serialtype is not (which ditto).  The patch blithely overrode
the opr_sanity check for that, which was sloppy-enough work in itself,
but the indisputable reason this cannot be allowed to stand is that CREATE
FUNCTION will reject such a signature and thus it'd be impossible for
extensions to create parallelizable aggregates.

The minimum fix to make the signature type-safe is to add a second, dummy
argument of type INTERNAL.  But to lock it down a bit more and make misuse
of INTERNAL-accepting functions less likely, let's get rid of the ability
to specify a "serialtype" for an aggregate and just say that the only
useful serialtype is BYTEA --- which, in practice, is the only interesting
value anyway, due to the usefulness of the send/recv infrastructure for
this purpose.  That means we only have to allow "serialize(internal)
returns bytea" and "deserialize(bytea, internal) returns internal" as
the signatures for these support functions.

In passing fix bogus signature of int4_avg_combine, which I found thanks
to adding an opr_sanity check on combinefunc signatures.

catversion bump due to removing pg_aggregate.aggserialtype and adjusting
signatures of assorted built-in functions.

David Rowley and Tom Lane

Discussion: <27247.1466185504@sss.pgh.pa.us>
This commit is contained in:
Tom Lane
2016-06-22 16:52:41 -04:00
parent e45e990e4b
commit f8ace5477e
18 changed files with 506 additions and 715 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
src/test/regress/expected/create_aggregate.out
View File

@ -102,36 +102,19 @@ CREATE AGGREGATE sumdouble (float8)
minvfunc = float8mi
);
-- aggregate combine and serialization functions
-- Ensure stype and serialtype can't be the same
-- can't specify just one of serialfunc and deserialfunc
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = internal
);
ERROR: aggregate serialization data type cannot be internal
-- if serialtype is specified we need a serialfunc and deserialfunc
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = bytea
);
ERROR: aggregate serialization function must be specified when serialization type is specified
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = bytea,
serialfunc = numeric_avg_serialize
);
ERROR: aggregate deserialization function must be specified when serialization type is specified
ERROR: must specify both or neither of serialization and deserialization functions
-- serialfunc must have correct parameters
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = bytea,
serialfunc = numeric_avg_deserialize,
deserialfunc = numeric_avg_deserialize
);
@ -141,27 +124,15 @@ CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = bytea,
serialfunc = numeric_avg_serialize,
deserialfunc = numeric_avg_serialize
);
ERROR: function numeric_avg_serialize(bytea) does not exist
-- ensure return type of serialfunc is checked
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = text,
serialfunc = numeric_avg_serialize,
deserialfunc = numeric_avg_deserialize
);
ERROR: return type of serialization function numeric_avg_serialize is not text
ERROR: function numeric_avg_serialize(bytea, internal) does not exist
-- ensure combine function parameters are checked
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = bytea,
serialfunc = numeric_avg_serialize,
deserialfunc = numeric_avg_deserialize,
combinefunc = int4larger
@ -173,18 +144,17 @@ CREATE AGGREGATE myavg (numeric)
stype = internal,
sfunc = numeric_avg_accum,
finalfunc = numeric_avg,
serialtype = bytea,
serialfunc = numeric_avg_serialize,
deserialfunc = numeric_avg_deserialize,
combinefunc = numeric_avg_combine
);
-- Ensure all these functions made it into the catalog
SELECT aggfnoid,aggtransfn,aggcombinefn,aggtranstype,aggserialfn,aggdeserialfn,aggserialtype
SELECT aggfnoid,aggtransfn,aggcombinefn,aggtranstype,aggserialfn,aggdeserialfn
FROM pg_aggregate
WHERE aggfnoid = 'myavg'::REGPROC;
aggfnoid | aggtransfn | aggcombinefn | aggtranstype | aggserialfn | aggdeserialfn | aggserialtype
----------+-------------------+---------------------+--------------+-----------------------+-------------------------+---------------
myavg | numeric_avg_accum | numeric_avg_combine | 2281 | numeric_avg_serialize | numeric_avg_deserialize | 17
aggfnoid | aggtransfn | aggcombinefn | aggtranstype | aggserialfn | aggdeserialfn
----------+-------------------+---------------------+--------------+-----------------------+-------------------------
myavg | numeric_avg_accum | numeric_avg_combine | 2281 | numeric_avg_serialize | numeric_avg_deserialize
(1 row)
DROP AGGREGATE myavg (numeric);

176
src/test/regress/expected/opr_sanity.out
View File

@ -279,21 +279,16 @@ ORDER BY 1, 2;
-- Look for functions that return type "internal" and do not have any
-- "internal" argument. Such a function would be a security hole since
-- it might be used to call an internal function from an SQL command.
-- As of 7.3 this query should find internal_in, and as of 9.6 aggregate
-- deserialization will be found too. These should contain a runtime check to
-- ensure they can only be called in an aggregate context.
-- As of 7.3 this query should find only internal_in, which is safe because
-- it always throws an error when called.
SELECT p1.oid, p1.proname
FROM pg_proc as p1
WHERE p1.prorettype = 'internal'::regtype AND NOT
'internal'::regtype = ANY (p1.proargtypes);
oid | proname
------+--------------------------
2741 | numeric_avg_deserialize
3336 | numeric_deserialize
3340 | numeric_poly_deserialize
2787 | int8_avg_deserialize
oid | proname
------+-------------
2304 | internal_in
(5 rows)
(1 row)
-- Look for functions that return a polymorphic type and do not have any
-- polymorphic argument. Calls of such functions would be unresolvable
@ -1442,6 +1437,84 @@ WHERE a.aggfnoid = p.oid AND
----------+---------+-----+---------+-----+---------
(0 rows)
-- Check that all combine functions have signature
-- combine(transtype, transtype) returns transtype
-- NOTE: use physically_coercible here, not binary_coercible, because
-- max and min on abstime are implemented using int4larger/int4smaller.
SELECT a.aggfnoid, p.proname
FROM pg_aggregate as a, pg_proc as p
WHERE a.aggcombinefn = p.oid AND
(p.pronargs != 2 OR
p.prorettype != p.proargtypes[0] OR
p.prorettype != p.proargtypes[1] OR
NOT physically_coercible(a.aggtranstype, p.proargtypes[0]));
aggfnoid | proname
----------+---------
(0 rows)
-- Check that no combine function for an INTERNAL transtype is strict.
SELECT a.aggfnoid, p.proname
FROM pg_aggregate as a, pg_proc as p
WHERE a.aggcombinefn = p.oid AND
a.aggtranstype = 'internal'::regtype AND p.proisstrict;
aggfnoid | proname
----------+---------
(0 rows)
-- serialize/deserialize functions should be specified only for aggregates
-- with transtype internal and a combine function, and we should have both
-- or neither of them.
SELECT aggfnoid, aggtranstype, aggserialfn, aggdeserialfn
FROM pg_aggregate
WHERE (aggserialfn != 0 OR aggdeserialfn != 0)
AND (aggtranstype != 'internal'::regtype OR aggcombinefn = 0 OR
aggserialfn = 0 OR aggdeserialfn = 0);
aggfnoid | aggtranstype | aggserialfn | aggdeserialfn
----------+--------------+-------------+---------------
(0 rows)
-- Check that all serialization functions have signature
-- serialize(internal) returns bytea
-- Also insist that they be strict; it's wasteful to run them on NULLs.
SELECT a.aggfnoid, p.proname
FROM pg_aggregate as a, pg_proc as p
WHERE a.aggserialfn = p.oid AND
(p.prorettype != 'bytea'::regtype OR p.pronargs != 1 OR
p.proargtypes[0] != 'internal'::regtype OR
NOT p.proisstrict);
aggfnoid | proname
----------+---------
(0 rows)
-- Check that all deserialization functions have signature
-- deserialize(bytea, internal) returns internal
-- Also insist that they be strict; it's wasteful to run them on NULLs.
SELECT a.aggfnoid, p.proname
FROM pg_aggregate as a, pg_proc as p
WHERE a.aggdeserialfn = p.oid AND
(p.prorettype != 'internal'::regtype OR p.pronargs != 2 OR
p.proargtypes[0] != 'bytea'::regtype OR
p.proargtypes[1] != 'internal'::regtype OR
NOT p.proisstrict);
aggfnoid | proname
----------+---------
(0 rows)
-- Check that aggregates which have the same transition function also have
-- the same combine, serialization, and deserialization functions.
-- While that isn't strictly necessary, it's fishy if they don't.
SELECT a.aggfnoid, a.aggcombinefn, a.aggserialfn, a.aggdeserialfn,
b.aggfnoid, b.aggcombinefn, b.aggserialfn, b.aggdeserialfn
FROM
pg_aggregate a, pg_aggregate b
WHERE
a.aggfnoid < b.aggfnoid AND a.aggtransfn = b.aggtransfn AND
(a.aggcombinefn != b.aggcombinefn OR a.aggserialfn != b.aggserialfn
OR a.aggdeserialfn != b.aggdeserialfn);
aggfnoid | aggcombinefn | aggserialfn | aggdeserialfn | aggfnoid | aggcombinefn | aggserialfn | aggdeserialfn
----------+--------------+-------------+---------------+----------+--------------+-------------+---------------
(0 rows)
-- Cross-check aggsortop (if present) against pg_operator.
-- We expect to find entries for bool_and, bool_or, every, max, and min.
SELECT DISTINCT proname, oprname
@ -1534,89 +1607,6 @@ WHERE proisagg AND provariadic != 0 AND a.aggkind = 'n';
-----+---------
(0 rows)
-- Check that all serial functions have a return type the same as the serial
-- type.
SELECT a.aggserialfn,a.aggserialtype,p.prorettype
FROM pg_aggregate a
INNER JOIN pg_proc p ON a.aggserialfn = p.oid
WHERE a.aggserialtype <> p.prorettype;
aggserialfn | aggserialtype | prorettype
-------------+---------------+------------
(0 rows)
-- Check that all the deserial functions have the same input type as the
-- serialtype
SELECT a.aggserialfn,a.aggserialtype,p.proargtypes[0]
FROM pg_aggregate a
INNER JOIN pg_proc p ON a.aggdeserialfn = p.oid
WHERE p.proargtypes[0] <> a.aggserialtype;
aggserialfn | aggserialtype | proargtypes
-------------+---------------+-------------
(0 rows)
-- An aggregate should either have a complete set of serialtype, serial func
-- and deserial func, or none of them.
SELECT aggserialtype,aggserialfn,aggdeserialfn
FROM pg_aggregate
WHERE (aggserialtype <> 0 OR aggserialfn <> 0 OR aggdeserialfn <> 0)
AND (aggserialtype = 0 OR aggserialfn = 0 OR aggdeserialfn = 0);
aggserialtype | aggserialfn | aggdeserialfn
---------------+-------------+---------------
(0 rows)
-- Check that all aggregates with serialtypes have internal states.
-- (There's no point in serializing anything apart from internal)
SELECT aggfnoid,aggserialtype,aggtranstype
FROM pg_aggregate
WHERE aggserialtype <> 0 AND aggtranstype <> 'internal'::regtype;
aggfnoid | aggserialtype | aggtranstype
----------+---------------+--------------
(0 rows)
-- Check that all serial functions are strict. It's wasteful for these to be
-- called with NULL values.
SELECT aggfnoid,aggserialfn
FROM pg_aggregate a
INNER JOIN pg_proc p ON a.aggserialfn = p.oid
WHERE p.proisstrict = false;
aggfnoid | aggserialfn
----------+-------------
(0 rows)
-- Check that all deserial functions are strict. It's wasteful for these to be
-- called with NULL values.
SELECT aggfnoid,aggdeserialfn
FROM pg_aggregate a
INNER JOIN pg_proc p ON a.aggdeserialfn = p.oid
WHERE p.proisstrict = false;
aggfnoid | aggdeserialfn
----------+---------------
(0 rows)
-- Check that no combine functions with an INTERNAL return type are strict.
SELECT aggfnoid,aggcombinefn
FROM pg_aggregate a
INNER JOIN pg_proc p ON a.aggcombinefn = p.oid
INNER JOIN pg_type t ON a.aggtranstype = t.oid
WHERE t.typname = 'internal' AND p.proisstrict = true;
aggfnoid | aggcombinefn
----------+--------------
(0 rows)
-- Check that aggregates which have the same transition function also have
-- the same combine, serialization, and deserialization functions.
SELECT a.aggfnoid, a.aggcombinefn, a.aggserialfn, a.aggdeserialfn,
b.aggfnoid, b.aggcombinefn, b.aggserialfn, b.aggdeserialfn
FROM
pg_aggregate a, pg_aggregate b
WHERE
a.aggfnoid < b.aggfnoid AND a.aggtransfn = b.aggtransfn AND
(a.aggcombinefn != b.aggcombinefn OR a.aggserialfn != b.aggserialfn
OR a.aggdeserialfn != b.aggdeserialfn);
aggfnoid | aggcombinefn | aggserialfn | aggdeserialfn | aggfnoid | aggcombinefn | aggserialfn | aggdeserialfn
----------+--------------+-------------+---------------+----------+--------------+-------------+---------------
(0 rows)
-- **************** pg_opfamily ****************
-- Look for illegal values in pg_opfamily fields
SELECT p1.oid

34
src/test/regress/sql/create_aggregate.sql
View File

@ -117,27 +117,11 @@ CREATE AGGREGATE sumdouble (float8)
-- aggregate combine and serialization functions
-- Ensure stype and serialtype can't be the same
-- can't specify just one of serialfunc and deserialfunc
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = internal
);
-- if serialtype is specified we need a serialfunc and deserialfunc
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = bytea
);
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = bytea,
serialfunc = numeric_avg_serialize
);
@ -146,7 +130,6 @@ CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = bytea,
serialfunc = numeric_avg_deserialize,
deserialfunc = numeric_avg_deserialize
);
@ -156,27 +139,15 @@ CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = bytea,
serialfunc = numeric_avg_serialize,
deserialfunc = numeric_avg_serialize
);
-- ensure return type of serialfunc is checked
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = text,
serialfunc = numeric_avg_serialize,
deserialfunc = numeric_avg_deserialize
);
-- ensure combine function parameters are checked
CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
serialtype = bytea,
serialfunc = numeric_avg_serialize,
deserialfunc = numeric_avg_deserialize,
combinefunc = int4larger
@ -188,14 +159,13 @@ CREATE AGGREGATE myavg (numeric)
stype = internal,
sfunc = numeric_avg_accum,
finalfunc = numeric_avg,
serialtype = bytea,
serialfunc = numeric_avg_serialize,
deserialfunc = numeric_avg_deserialize,
combinefunc = numeric_avg_combine
);
-- Ensure all these functions made it into the catalog
SELECT aggfnoid,aggtransfn,aggcombinefn,aggtranstype,aggserialfn,aggdeserialfn,aggserialtype
SELECT aggfnoid,aggtransfn,aggcombinefn,aggtranstype,aggserialfn,aggdeserialfn
FROM pg_aggregate
WHERE aggfnoid = 'myavg'::REGPROC;

129
src/test/regress/sql/opr_sanity.sql
View File

@ -228,9 +228,8 @@ ORDER BY 1, 2;
-- Look for functions that return type "internal" and do not have any
-- "internal" argument. Such a function would be a security hole since
-- it might be used to call an internal function from an SQL command.
-- As of 7.3 this query should find internal_in, and as of 9.6 aggregate
-- deserialization will be found too. These should contain a runtime check to
-- ensure they can only be called in an aggregate context.
-- As of 7.3 this query should find only internal_in, which is safe because
-- it always throws an error when called.
SELECT p1.oid, p1.proname
FROM pg_proc as p1
@ -937,6 +936,72 @@ WHERE a.aggfnoid = p.oid AND
a.aggminvtransfn = iptr.oid AND
ptr.proisstrict != iptr.proisstrict;
-- Check that all combine functions have signature
-- combine(transtype, transtype) returns transtype
-- NOTE: use physically_coercible here, not binary_coercible, because
-- max and min on abstime are implemented using int4larger/int4smaller.
SELECT a.aggfnoid, p.proname
FROM pg_aggregate as a, pg_proc as p
WHERE a.aggcombinefn = p.oid AND
(p.pronargs != 2 OR
p.prorettype != p.proargtypes[0] OR
p.prorettype != p.proargtypes[1] OR
NOT physically_coercible(a.aggtranstype, p.proargtypes[0]));
-- Check that no combine function for an INTERNAL transtype is strict.
SELECT a.aggfnoid, p.proname
FROM pg_aggregate as a, pg_proc as p
WHERE a.aggcombinefn = p.oid AND
a.aggtranstype = 'internal'::regtype AND p.proisstrict;
-- serialize/deserialize functions should be specified only for aggregates
-- with transtype internal and a combine function, and we should have both
-- or neither of them.
SELECT aggfnoid, aggtranstype, aggserialfn, aggdeserialfn
FROM pg_aggregate
WHERE (aggserialfn != 0 OR aggdeserialfn != 0)
AND (aggtranstype != 'internal'::regtype OR aggcombinefn = 0 OR
aggserialfn = 0 OR aggdeserialfn = 0);
-- Check that all serialization functions have signature
-- serialize(internal) returns bytea
-- Also insist that they be strict; it's wasteful to run them on NULLs.
SELECT a.aggfnoid, p.proname
FROM pg_aggregate as a, pg_proc as p
WHERE a.aggserialfn = p.oid AND
(p.prorettype != 'bytea'::regtype OR p.pronargs != 1 OR
p.proargtypes[0] != 'internal'::regtype OR
NOT p.proisstrict);
-- Check that all deserialization functions have signature
-- deserialize(bytea, internal) returns internal
-- Also insist that they be strict; it's wasteful to run them on NULLs.
SELECT a.aggfnoid, p.proname
FROM pg_aggregate as a, pg_proc as p
WHERE a.aggdeserialfn = p.oid AND
(p.prorettype != 'internal'::regtype OR p.pronargs != 2 OR
p.proargtypes[0] != 'bytea'::regtype OR
p.proargtypes[1] != 'internal'::regtype OR
NOT p.proisstrict);
-- Check that aggregates which have the same transition function also have
-- the same combine, serialization, and deserialization functions.
-- While that isn't strictly necessary, it's fishy if they don't.
SELECT a.aggfnoid, a.aggcombinefn, a.aggserialfn, a.aggdeserialfn,
b.aggfnoid, b.aggcombinefn, b.aggserialfn, b.aggdeserialfn
FROM
pg_aggregate a, pg_aggregate b
WHERE
a.aggfnoid < b.aggfnoid AND a.aggtransfn = b.aggtransfn AND
(a.aggcombinefn != b.aggcombinefn OR a.aggserialfn != b.aggserialfn
OR a.aggdeserialfn != b.aggdeserialfn);
-- Cross-check aggsortop (if present) against pg_operator.
-- We expect to find entries for bool_and, bool_or, every, max, and min.
@ -1004,64 +1069,6 @@ SELECT p.oid, proname
FROM pg_proc AS p JOIN pg_aggregate AS a ON a.aggfnoid = p.oid
WHERE proisagg AND provariadic != 0 AND a.aggkind = 'n';
-- Check that all serial functions have a return type the same as the serial
-- type.
SELECT a.aggserialfn,a.aggserialtype,p.prorettype
FROM pg_aggregate a
INNER JOIN pg_proc p ON a.aggserialfn = p.oid
WHERE a.aggserialtype <> p.prorettype;
-- Check that all the deserial functions have the same input type as the
-- serialtype
SELECT a.aggserialfn,a.aggserialtype,p.proargtypes[0]
FROM pg_aggregate a
INNER JOIN pg_proc p ON a.aggdeserialfn = p.oid
WHERE p.proargtypes[0] <> a.aggserialtype;
-- An aggregate should either have a complete set of serialtype, serial func
-- and deserial func, or none of them.
SELECT aggserialtype,aggserialfn,aggdeserialfn
FROM pg_aggregate
WHERE (aggserialtype <> 0 OR aggserialfn <> 0 OR aggdeserialfn <> 0)
AND (aggserialtype = 0 OR aggserialfn = 0 OR aggdeserialfn = 0);
-- Check that all aggregates with serialtypes have internal states.
-- (There's no point in serializing anything apart from internal)
SELECT aggfnoid,aggserialtype,aggtranstype
FROM pg_aggregate
WHERE aggserialtype <> 0 AND aggtranstype <> 'internal'::regtype;
-- Check that all serial functions are strict. It's wasteful for these to be
-- called with NULL values.
SELECT aggfnoid,aggserialfn
FROM pg_aggregate a
INNER JOIN pg_proc p ON a.aggserialfn = p.oid
WHERE p.proisstrict = false;
-- Check that all deserial functions are strict. It's wasteful for these to be
-- called with NULL values.
SELECT aggfnoid,aggdeserialfn
FROM pg_aggregate a
INNER JOIN pg_proc p ON a.aggdeserialfn = p.oid
WHERE p.proisstrict = false;
-- Check that no combine functions with an INTERNAL return type are strict.
SELECT aggfnoid,aggcombinefn
FROM pg_aggregate a
INNER JOIN pg_proc p ON a.aggcombinefn = p.oid
INNER JOIN pg_type t ON a.aggtranstype = t.oid
WHERE t.typname = 'internal' AND p.proisstrict = true;
-- Check that aggregates which have the same transition function also have
-- the same combine, serialization, and deserialization functions.
SELECT a.aggfnoid, a.aggcombinefn, a.aggserialfn, a.aggdeserialfn,
b.aggfnoid, b.aggcombinefn, b.aggserialfn, b.aggdeserialfn
FROM
pg_aggregate a, pg_aggregate b
WHERE
a.aggfnoid < b.aggfnoid AND a.aggtransfn = b.aggtransfn AND
(a.aggcombinefn != b.aggcombinefn OR a.aggserialfn != b.aggserialfn
OR a.aggdeserialfn != b.aggdeserialfn);
-- **************** pg_opfamily ****************