Thread: Set of patch to address several Coverity issues

Set of patch to address several Coverity issues

From
Michael Paquier
Date:
Hi all,

As there have been complaints that it was hard to follow all the small
patches I have sent to fix the issues related to Coverity, here they
are gathered with patches for each one of them:
1) Missing return value checks in jsonfuncs.c, fixed by 0001 (send
here previously =>
CAB7nPqQcj3hU9P7a6VUHoMepJkOYQrjXnT1g2f7Qy_CQ0Q8G_g@mail.gmail.com)
 JsonbIteratorNext is missing a set of (void) in front of its calls.
2) Potential pointer dereference in plperl.c, fixed by 0002 (sent
previously here =>
CAB7nPqRBCWAXTLw0yBR=BK94cRYXU8TWVxGyYoxautw08OKeXw@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.
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.
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@mail.gmail.com)

Each issue is independent, please feel free to comment.
Regards,
--
Michael

Attachment

Re: Set of patch to address several Coverity issues

From
Andres Freund
Date:
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@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?

> 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.

> 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@mail.gmail.com)

But the other cases accept either arbitrary input or perform timezone
conversions. Not the case here, no?

- Andres



Re: Set of patch to address several Coverity issues

From
Michael Paquier
Date:
On Wed, Jul 8, 2015 at 12:54 AM, Andres Freund <andres@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=BK94cRYXU8TWVxGyYoxautw08OKeXw@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=VO2ogBQ@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



Re: Set of patch to address several Coverity issues

From
Andres Freund
Date:
On 2015-07-08 14:11:59 +0900, Michael Paquier wrote:
> 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);

We do the current type of tests in a bunch of places, I'd not modify
code just to change it. But since apparently the whole test is
pointless, I could see replacing it by an assert.

> >> 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@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.

So what? GetCurrentDateTime()'s returned data is still pretty much
guaranteed to be correct unless a lot of things have gone wrong
previously?

> In any case, we are going to need at least (void) in front of those calls.

We're "needing" nothing of the sort.



Re: Set of patch to address several Coverity issues

From
Michael Paquier
Date:
On Wed, Jul 8, 2015 at 6:16 PM, Andres Freund wrote:
>> In any case, we are going to need at least (void) in front of those calls.
>
> We're "needing" nothing of the sort.

I don't really understand your reluctance here. As one example, see
c831593 where similar fixes are done and even back-patched.
-- 
Michael



Re: Set of patch to address several Coverity issues

From
Andres Freund
Date:
On 2015-07-09 22:57:25 +0900, Michael Paquier wrote:
> On Wed, Jul 8, 2015 at 6:16 PM, Andres Freund wrote:
> >> In any case, we are going to need at least (void) in front of those calls.
> >
> > We're "needing" nothing of the sort.
> 
> I don't really understand your reluctance here. As one example, see
> c831593 where similar fixes are done and even back-patched.

That doesn't make it a required thing. And the changes there we more
than just adding a (void).

To me this kind of changes are busywork. Analsys tools are there to make
our work easier, not to generate more. There's good reasons why, with
other tools, in the past we've rejected lots of bogus "issues", even if
we could have silenced them by changing the code.