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

[Gedare Bloom]

On Wed, Apr 14, 2021 at 9:57 AM Gedare Bloom <gedare at rtems.org> wrote:
>
> On Wed, Apr 14, 2021 at 8:49 AM Joel Sherrill <joel at rtems.org> wrote:
> >
> >
> >
> > 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.
> >
>
> OK
>
Add the assert.

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