Re: Set of patch to address several Coverity issues

On Wed, Jul 8, 2015 at 12:54 AM, Andres Freund <andres(at)anarazel(dot)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=BK94cRYXU8TWVxGyYoxautw08OKeXw(at)mail(dot)gmail(dot)com).
>> This is related to a change done by transforms. In short,
>> plperl_call_perl_func(at)plperl(dot)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=VO2ogBQ(at)mail(dot)gmail(dot)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