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

[Gedare Bloom]

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?

>>
>>
>>
>> > +      /*
>> >         *  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