[PATCH] posix: Only check shm_unlink obj_err if necessary

[Gedare Bloom]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20200627/2b4066ba/attachment.html>