[PATCH] posix: Only check shm_unlink obj_err if necessary

[Will]

Ticket 4016 opened for 5.1 and patch sent with appropriate close tag.

On Sat, Jun 27, 2020 at 4:30 PM Gedare Bloom <gedare at rtems.org> wrote:

> This needs a ticket now to apply to 5, and we'll want to apply to 6 also.
> Can you open a ticket and post a patch for 5.1?
>
> On Thu, Jun 25, 2020, 5:49 PM Kinsey Moore <kinsey.moore at oarcorp.com>
> wrote:
>
>> Hey Gedare,
>> Setting obj_err to 0 would get the desired outcome, but the logic as it
>> exists now is faulty. Setting obj_err to a value that can't be produced by
>> _POSIX_Shm_Get_by_name would be a better option, but it would still be
>> checking an error value after successful execution. _POSIX_Shm_Get_by_name
>> relies on _Objects_Get_by_name internally which states about obj_err: The
>> error indication in case of failure. If _POSIX_Shm_Get_by_name returns
>> OBJECTS_GET_BY_NAME_NO_OBJECT (which it can), the current logic treats that
>> as a success and operates on a NULL pointer.
>>
>> -----Original Message-----
>> From: Gedare Bloom <gedare at rtems.org>
>> Sent: Thursday, June 25, 2020 16:49
>> To: Kinsey Moore <kinsey.moore at oarcorp.com>
>> Cc: devel at rtems.org
>> Subject: Re: [PATCH] posix: Only check shm_unlink obj_err if necessary
>>
>> Hi Kinsey,
>>
>> I missed seeing this. Two quick questions for you.
>>
>> 1. does it also work to initialize obj_err to 0? that would be simpler
>> code.
>>
>> 2. I see the error handling logic changes slightly, with
>> OBJECTS_GET_BY_NAME_NO_OBJECT now returning ENOENT. I guess if it works to
>> init obj_err to 0, this case should be merged back to the
>> OBJECTS_GET_BY_NAME_INVALID_NAME case?
>>
>> On Thu, Jun 25, 2020 at 2:21 PM Kinsey Moore <kinsey.moore at oarcorp.com>
>> wrote:
>> >
>> > Is there anything stopping this from being merged? I just ran into this
>> bug again on the current project I'm working on.
>> >
>> > Kinsey
>> >
>> > -----Original Message-----
>> > From: Kinsey Moore <kinsey.moore at oarcorp.com>
>> > Sent: Tuesday, January 28, 2020 12:37
>> > To: devel at rtems.org
>> > Cc: Kinsey Moore <kinsey.moore at oarcorp.com>
>> > Subject: [PATCH] posix: Only check shm_unlink obj_err if necessary
>> >
>> > In the nominal case checked by spsysinit01, obj_err is unmodified if
>> _POSIX_Shm_Get_by_name returns non-NULL. In the case of shm_unlink, this
>> means an uninitialized value is passed into the switch and it appears this
>> test was passing by virtue of the stack having the right value on it in
>> most cases. This now checks obj_err only if _POSIX_Shm_Get_by_name returns
>> NULL.
>> > ---
>> >  cpukit/posix/src/shmunlink.c | 45
>> > ++++++++++++++++++------------------
>> >  1 file changed, 23 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/cpukit/posix/src/shmunlink.c
>> > b/cpukit/posix/src/shmunlink.c index 00d743ac80..39c2ba0d87 100644
>> > --- a/cpukit/posix/src/shmunlink.c
>> > +++ b/cpukit/posix/src/shmunlink.c
>> > @@ -29,28 +29,29 @@ int shm_unlink( const char *name )
>> >    _Objects_Allocator_lock();
>> >
>> >    shm = _POSIX_Shm_Get_by_name( name, 0, &obj_err );
>> > -  switch ( obj_err ) {
>> > -    case OBJECTS_GET_BY_NAME_INVALID_NAME:
>> > -      err = ENOENT;
>> > -      break;
>> > -
>> > -    case OBJECTS_GET_BY_NAME_NAME_TOO_LONG:
>> > -      err = ENAMETOOLONG;
>> > -      break;
>> > -
>> > -    case OBJECTS_GET_BY_NAME_NO_OBJECT:
>> > -    default:
>> > -      _Objects_Namespace_remove_string(
>> > -        &_POSIX_Shm_Information,
>> > -        &shm->Object
>> > -      );
>> > -
>> > -      if ( shm->reference_count == 0 ) {
>> > -        /* Only remove the shm object if no references exist to it.
>> Otherwise,
>> > -         * the shm object will be freed later in
>> _POSIX_Shm_Attempt_delete */
>> > -        _POSIX_Shm_Free( shm );
>> > -      }
>> > -      break;
>> > +  if ( shm ) {
>> > +    _Objects_Namespace_remove_string(
>> > +      &_POSIX_Shm_Information,
>> > +      &shm->Object
>> > +    );
>> > +
>> > +    if ( shm->reference_count == 0 ) {
>> > +      /* Only remove the shm object if no references exist to it.
>> Otherwise,
>> > +       * the shm object will be freed later in
>> _POSIX_Shm_Attempt_delete */
>> > +      _POSIX_Shm_Free( shm );
>> > +    }
>> > +  } else {
>> > +    switch ( obj_err ) {
>> > +      case OBJECTS_GET_BY_NAME_NAME_TOO_LONG:
>> > +        err = ENAMETOOLONG;
>> > +        break;
>> > +
>> > +      case OBJECTS_GET_BY_NAME_INVALID_NAME:
>> > +      case OBJECTS_GET_BY_NAME_NO_OBJECT:
>> > +      default:
>> > +        err = ENOENT;
>> > +        break;
>> > +    }
>> >    }
>> >
>> >    _Objects_Allocator_unlock();
>> > --
>> > 2.20.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/20200627/718dfb5e/attachment.html>