Not quite a security hole in internal_in - Mailing list pgsql-hackers

I noticed the following core-dump situation in CVS HEAD:

regression=# select array_agg_finalfn(null);
server closed the connection unexpectedly       This probably means the server terminated abnormally       before or
whileprocessing the request.
 
The connection to the server was lost. Attempting reset: Failed.

(You won't see a crash if you don't have Asserts on.)  The proximate
cause of this is that array_agg_finalfn is being a bit overoptimistic
about what it can Assert:
/* cannot be called directly because of internal-type argument */Assert(fcinfo->context &&       (IsA(fcinfo->context,
AggState)||        IsA(fcinfo->context, WindowAggState)));
 
if (PG_ARGISNULL(0))    PG_RETURN_NULL();   /* returns null iff no input values */

We should switch the order of the null-test and the Assert.  However,
this brings up the question of exactly why the assumption embedded
in that comment is wrong.  You're not supposed to be able to call
internal-accepting functions from SQL, and yet here I did so.

The reason I could get past the type-safety check is that internal_in,
which normally throws an error if one tries to create a constant of type
internal, is marked STRICT in pg_proc, and so it doesn't get called for
nulls.

This would be a serious security problem if it weren't for the fact that
nearly all internal-accepting functions in the backend are also marked
STRICT, and so they won't get called in this type of scenario.  A query
to pg_proc shows that the only ones that aren't strict are

regression=# select oid::regprocedure from pg_proc where 'internal'::regtype = any (proargtypes) and not proisstrict;
             oid                   
 

----------------------------------------array_agg_transfn(internal,anyelement)array_agg_finalfn(internal)domain_recv(internal,oid,integer)
(3 rows)

The first two are new in 8.4, and the third has adequate defenses
already.  So we don't have a security hole in any released version
right now.

However, this is obviously something that could bite us in the future.
What I think we should do about it is mark internal_in as nonstrict,
so that it gets called and can throw error for a null.  Probably the
same for all the other pseudotypes in pseudotypes.c, although internal
is the only one that we consider to be a security-critical datatype.

Normally we would consider a pg_proc change as requiring a catversion
bump.  Since we are already past 8.4 beta we couldn't do that without
forcing an initdb for beta testers.  What I'd like to do about this
is change the proisstrict settings in pg_proc.h but not bump catversion.
This will ensure the fix is in place and protecting future coding,
although possibly not getting enforced in 8.4 production instances that
were upgraded from beta (if there are any such).

Comments?
        regards, tom lane