On Wed, Jul 8, 2015 at 12:54 AM, Andres Freund <and...@anarazel.de> wrote: > On 2015-07-07 16:17:47 +0900, Michael Paquier wrote: >> 2) Potential pointer dereference in plperl.c, fixed by 0002 (sent >> previously here => >> CAB7nPqRBCWAXTLw0yBR=bk94cryxu8twvxgyyoxautw08ok...@mail.gmail.com). >> This is related to a change done by transforms. In short, >> plperl_call_perl_func@plperl.c will have a pointer dereference if >> desc->arg_arraytype[i] is InvalidOid. And AFAIK, >> fcinfo->flinfo->fn_oid can be InvalidOid in this code path. > > Aren't we in trouble if fn_oid isn't valid at that point? Why would it > be ok for it to be InvalidOid? The function oid is used in lots of > fundamental places, like actually compiling the plperl functions > (compile_plperl_function). > > Which path could lead to it validly being InvalidOid?
Arg... I thought I triggered a couple of weeks a problem in this code path when desc->arg_arraytype[i] is InvalidOid with argtypes == NULL. Visibly I did something wrong... Speaking of which, shouldn't this thing at least use OidIsValid? - if (fcinfo->flinfo->fn_oid) + if (OidIsValid(fcinfo->flinfo->fn_oid)) get_func_signature(fcinfo->flinfo->fn_oid, &argtypes, &nargs); >> 3) visibilitymap_truncate and FreeSpaceMapTruncateRel are doing a >> NULL-pointer check on rel->rd_smgr but it has been dereferenced in all >> the code paths leading to those checks. See 0003. For code readability >> mainly. > > FWIW, there's actually some reasoning/paranoia behind those > checks. smgrtruncate() sends out immediate smgr sinval messages, which > will invalidate rd_smgr. Now, I think in both cases there's currently > no way we'll run code between the smgrtruncate() and the if > (rel->rd_smgr) that does a CommandCounterIncrement() causing them to be > replayed locally, but there's some merit in future proofing. OK, let's drop this one then. >> 4) Return result of timestamp2tm is not checked 2 times in >> GetCurrentDateTime and GetCurrentTimeUsec, while all the other 40 >> calls of this function do sanity checks. Returning >> ERRCODE_DATETIME_VALUE_OUT_OF_RANGE in case of an error would be good >> for consistency. See 0004. (issue reported previously here >> CAB7nPqRSk=J8eUdd55fL-w+k=8sDTHLVBt-cgG9jWN=vo2o...@mail.gmail.com) > > But the other cases accept either arbitrary input or perform timezone > conversions. Not the case here, no? The callers of GetCurrentDateTime() and GetCurrentTimeUsec() do some work on pg_tm, see time_timetz() that does accept some input DecodeDateTime() for example. In any case, we are going to need at least (void) in front of those calls. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers