[PATCH 1/2] objectextendinformation.c: Fix Dereference after null check (CID #26033)

[Joel Sherrill]

On Mon, Mar 15, 2021 at 5:09 PM Gedare Bloom <gedare at rtems.org> wrote:

> On Mon, Mar 15, 2021 at 2:27 PM Joel Sherrill <joel at rtems.org> wrote:
> >
> >
> >
> > On Mon, Mar 15, 2021, 3:21 PM Gedare Bloom <gedare at rtems.org> wrote:
> >>
> >> On Fri, Mar 12, 2021 at 7:55 AM Ryan Long <ryan.long at oarcorp.com>
> wrote:
> >> >
> >> > CID 26033: Dereference after null check in
> _Objects_Extend_information().
> >> >
> >> > Closes #4326
> >> > ---
> >> >  cpukit/score/src/objectextendinformation.c | 11 +++++++++++
> >> >  1 file changed, 11 insertions(+)
> >> >
> >> > diff --git a/cpukit/score/src/objectextendinformation.c
> b/cpukit/score/src/objectextendinformation.c
> >> > index 9796eb9..c669301 100644
> >> > --- a/cpukit/score/src/objectextendinformation.c
> >> > +++ b/cpukit/score/src/objectextendinformation.c
> >> > @@ -171,6 +171,17 @@ Objects_Maximum _Objects_Extend_information(
> >> >
> >> >      if ( old_maximum > extend_count ) {
> >> >        /*
> >> > +       * Coverity thinks there is a way for this to be NULL (CID
> #26033).
> >> > +       * After much time spent analyzing this, no one has identified
> the
> >> > +       * conditions where this can actually occur. Adding this
> _Assert ensures
> >> > +       * that it is never NULL. If this assert is triggered,
> condition
> >> > +       * generating this case will have been identified and it can
> be revisted.
> >> > +       * This is being done out of an abundance of caution since we
> could have
> >> > +       * easily flagged this as a false positive and ignored it
> completely.
> >> > +       */
> >> > +      _Assert(information->object_blocks != NULL);
> >> > +
> >> That's interesting. It would help if you could share your analysis.
> >
> >
> > This is the oldest Coverity issue that is open. It is over five years
> old. Chris and I have tried multiple times to figure out if it is valid. We
> never get any confidence that it cannot occur.
> >
> >>
> >> How does
> >> 70  if ( information->object_blocks == NULL ) {
> >> be true, and if it is true, how does the exectuion flow proceed in
> >> such a way that it will not reach this assert?
> >
> >
> > No idea but it apparently doesn't based on our tests.
> >
> > Adding the assert is an attempt to finally find the case that trips
> this. It is either something I can never occur or something we don't know
> how to make happen. Either way the asserting like a good idea.
> >
> > if you have a test case in mind that can reproduce this coverity path,
> let's try it and push this to failure. But we have no evidence that it's
> ever occurred in the field.
>
> Can information->object_blocks be NULL at line 70?
>

Based on the coverage report case here, the answer is yes.

https://ftp.rtems.org/pub/rtems/people/joel/coverage/coverage-2021-02-28/xilinx_zynq_a9_qemu-coverage/score/annotated.html#range67

Everything was covered in this method except that one error case. And it
looks like there may be a covoar bug related to the branch that leads there
since it says always taken and the destination is never executed. That
can't happen.

This particular case was first reported by Coverity in Jan 2010. Chris and
I have looked at it multiple times and never can figure out how it would
happen. But we are not confident enough to mark it as false positive. Thus
the _Assert() should be tripped if this ever happens, Coverity should be
satisfied, and Chris and I won't worry we ignored something.

--joel

>
> >>
> >>
> >>
> >> > +      /*
> >> >         *  Copy each section of the table over. This has to be
> performed as
> >> >         *  separate parts as size of each block has changed.
> >> >         */
> >> > --
> >> > 1.8.3.1
> >> >
> >> > _______________________________________________
> >> > devel mailing list
> >> > devel at rtems.org
> >> > http://lists.rtems.org/mailman/listinfo/devel
> >> _______________________________________________
> >> devel mailing list
> >> devel at rtems.org
> >> http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210414/da6255d6/attachment.html>