[PATCH 2/2] posix: simplify paths and check errors in shm_open

[Gedare Bloom]

I have added some error checks for the heap-backed shm implementation,
and simplified the control flow to get all error checking done before
allocating memory. I will eventually send a v2 patch series, but I
wanted to show these self-contained fixes. I am looking into mmap
support presently.

On Fri, Aug 26, 2016 at 12:17 PM, Gedare Bloom <gedare at rtems.org> wrote:
> ---
>  cpukit/posix/src/shmopen.c | 84 +++++++++++++++++++++++++++-------------------
>  1 file changed, 49 insertions(+), 35 deletions(-)
>
> diff --git a/cpukit/posix/src/shmopen.c b/cpukit/posix/src/shmopen.c
> index 7823785..cbea88c 100644
> --- a/cpukit/posix/src/shmopen.c
> +++ b/cpukit/posix/src/shmopen.c
> @@ -39,6 +39,7 @@ static int shm_ftruncate( rtems_libio_t *iop, off_t length )
>    err = (*shm->shm_object.ops->object_resize)( shm, length );
>
>    if ( err != 0 ) {
> +    _POSIX_Shm_Release( shm, &queue_context );
>      rtems_set_errno_and_return_minus_one( err );
>    }
>
> @@ -60,6 +61,18 @@ static inline POSIX_Shm_Control *shm_allocate(
>    char *name;
>    struct timeval tv;
>
> +  /* Reject any name without a leading slash. */
> +  if ( name_arg[0] != '/' ) {
> +    *error = EINVAL;
> +    return NULL;
> +  }
> +
> +  /* Only create the object if requested. */
> +  if ( ( oflag & O_CREAT ) != O_CREAT ) {
> +    *error = ENOENT;
> +    return NULL;
> +  }
> +
>    name = _Workspace_String_duplicate( name_arg, name_len );
>    if ( name == NULL ) {
>      *error = ENOSPC;
> @@ -102,16 +115,8 @@ static inline bool shm_access_ok( POSIX_Shm_Control *shm, int oflag )
>    return rtems_filesystem_check_access( flags, shm->mode, shm->uid, shm->gid );
>  }
>
> -int shm_open( const char *name, int oflag, mode_t mode )
> +static inline int shm_check_oflag( int oflag )
>  {
> -  int err = 0;
> -  int fd;
> -  rtems_libio_t *iop;
> -  POSIX_Shm_Control *shm;
> -  size_t len;
> -  Objects_Get_by_name_error obj_err;
> -  Thread_queue_Context  queue_context;
> -
>    if ( ( oflag & O_ACCMODE ) != O_RDONLY && ( oflag & O_ACCMODE ) != O_RDWR ) {
>      rtems_set_errno_and_return_minus_one( EACCES );
>    }
> @@ -123,8 +128,28 @@ int shm_open( const char *name, int oflag, mode_t mode )
>    if ( ( oflag & O_TRUNC ) != 0 && ( oflag & O_ACCMODE ) != O_RDWR ) {
>      rtems_set_errno_and_return_minus_one( EACCES );
>    }
> +  return 0;
> +}
> +
> +int shm_open( const char *name, int oflag, mode_t mode )
> +{
> +  int err = 0;
> +  int fd;
> +  rtems_libio_t *iop;
> +  POSIX_Shm_Control *shm;
> +  size_t len;
> +  Objects_Get_by_name_error obj_err;
> +  Thread_queue_Context  queue_context;
> +
> +  if ( shm_check_oflag( oflag ) != 0 ) {
> +    return -1;
> +  }
>
>    /* TODO see if the object exists, shms available */
> +  iop = rtems_libio_allocate();
> +  if ( iop == NULL ) {
> +    rtems_set_errno_and_return_minus_one( EMFILE );
> +  }
>
>    _Objects_Allocator_lock();
>    shm = _POSIX_Shm_Get_by_name( name, &len, &obj_err );
> @@ -141,44 +166,33 @@ int shm_open( const char *name, int oflag, mode_t mode )
>
>        case OBJECTS_GET_BY_NAME_NO_OBJECT:
>        default:
> -        /* Reject any name without a leading slash. */
> -        if ( name[0] != '/' ) {
> -          err = EINVAL;
> -          break;
> -        }
> -
> -        if ( ( oflag & O_CREAT ) != O_CREAT ) {
> -          err = ENOENT;
> -          break;
> -        }
> -
>          shm = shm_allocate(name, len, oflag, mode, &err);
>          break;
>      }
>      _Objects_Allocator_unlock();
> -    if ( err != 0 ) {
> -      rtems_set_errno_and_return_minus_one( err );
> -    }
>    } else { /* shm exists */
>      _Objects_Allocator_unlock();
>      if ( ( oflag & ( O_EXCL | O_CREAT ) ) == ( O_EXCL | O_CREAT ) ) {
> -      rtems_set_errno_and_return_minus_one( EEXIST );
> -    }
> -    if ( !shm_access_ok( shm, oflag ) ) {
> -      rtems_set_errno_and_return_minus_one( EACCES );
> +      /* Request to create failed. */
> +      err = EEXIST;
> +    } else if ( !shm_access_ok( shm, oflag ) ) {
> +      err = EACCES;
> +    } else {
> +      _POSIX_Shm_Acquire_critical( shm, &queue_context );
> +      ++shm->reference_count;
> +      _POSIX_Shm_Release( shm, &queue_context );
>      }
> -    _POSIX_Shm_Acquire_critical( shm, &queue_context );
> -    ++shm->reference_count;
> -    _POSIX_Shm_Release( shm, &queue_context );
>    }
> -
> -  iop = rtems_libio_allocate();
> -  if ( iop == NULL ) {
> -    rtems_set_errno_and_return_minus_one( EMFILE );
> +  if ( err != 0 ) {
> +    rtems_libio_free( iop );
> +    rtems_set_errno_and_return_minus_one( err );
>    }
> +
>    if ( oflag & O_TRUNC ) {
> -      shm_ftruncate( iop, 0 );
> +    err = shm_ftruncate( iop, 0 );
> +    (void) err; /* ignore truncate error */
>    }
> +
>    fd = rtems_libio_iop_to_descriptor( iop );
>    iop->flags |= LIBIO_FLAGS_CLOSE_ON_EXEC;
>    if ( oflag & O_RDONLY ) {
> --
> 1.9.1
>