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 <ged...@rtems.org>
Sent: Thursday, June 25, 2020 16:49
To: Kinsey Moore <kinsey.mo...@oarcorp.com>
Cc: devel@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.mo...@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.mo...@oarcorp.com>
> Sent: Tuesday, January 28, 2020 12:37
> To: devel@rtems.org
> Cc: Kinsey Moore <kinsey.mo...@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@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
