[PATCH] posix: Only check shm_unlink obj_err if necessary

[Kinsey Moore]

Sorry, accidentally sent from my personal account. That was, in fact, me.

Kinsey

From: Will <nyphbl8d at gmail.com>
Sent: Saturday, June 27, 2020 19:24
To: Gedare Bloom <gedare at rtems.org>
Cc: Kinsey Moore <kinsey.moore at oarcorp.com>; devel at rtems.org
Subject: Re: [PATCH] posix: Only check shm_unlink obj_err if necessary

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<mailto: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<mailto: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<mailto:gedare at rtems.org>>
Sent: Thursday, June 25, 2020 16:49
To: Kinsey Moore <kinsey.moore at oarcorp.com<mailto:kinsey.moore at oarcorp.com>>
Cc: devel at rtems.org<mailto: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<mailto: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<mailto:kinsey.moore at oarcorp.com>>
> Sent: Tuesday, January 28, 2020 12:37
> To: devel at rtems.org<mailto:devel at rtems.org>
> Cc: Kinsey Moore <kinsey.moore at oarcorp.com<mailto: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<mailto:devel at rtems.org>
> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel at rtems.org<mailto: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/20200628/eeb4b7a2/attachment-0001.html>