Thread: Set of patch to address several Coverity issues
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
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
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
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.
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
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.