From c2287ba2cff59a50848151833404bce0e3cf0a70 Mon Sep 17 00:00:00 2001 From: Sebastian Huber Date: Tue, 10 Mar 2020 17:07:19 +0100 Subject: libio: Robust file descriptor reference counting There was a race conditon in the reference counting of file descriptors during a close() operation. After the call to the close handler, the rtems_libio_free() function cleared the flags to zero. However, at this point in time there may still exist some holders of the file descriptor. With RTEMS_DEBUG enabled this could lead to failed assertions in rtems_libio_iop_drop(). Change the code to use only atomic read-modify-write operations on the rtems_libio_iop::flags. --- cpukit/include/rtems/libio.h | 2 +- cpukit/include/rtems/libio_.h | 24 +----------- cpukit/libcsupport/src/fcntl.c | 4 +- cpukit/libcsupport/src/libio.c | 15 +++++++- cpukit/libcsupport/src/open.c | 6 +-- cpukit/libcsupport/src/sup_fs_deviceio.c | 6 +-- cpukit/libcsupport/src/termios.c | 6 +-- cpukit/libnetworking/rtems/rtems_syscall.c | 10 +++-- cpukit/posix/src/shmopen.c | 4 +- testsuites/fstests/fsclose01/init.c | 62 +++++++++++++++++++++++------- 10 files changed, 82 insertions(+), 57 deletions(-) diff --git a/cpukit/include/rtems/libio.h b/cpukit/include/rtems/libio.h index b5362320e8..bbaae10efd 100644 --- a/cpukit/include/rtems/libio.h +++ b/cpukit/include/rtems/libio.h @@ -1318,8 +1318,8 @@ extern const rtems_filesystem_limits_and_options_t * to (eg: offset, driver, pathname should be in that) */ struct rtems_libio_tt { - off_t offset; /* current offset into file */ Atomic_Uint flags; + off_t offset; /* current offset into file */ rtems_filesystem_location_info_t pathinfo; uint32_t data0; /* private to "driver" */ void *data1; /* ... */ diff --git a/cpukit/include/rtems/libio_.h b/cpukit/include/rtems/libio_.h index 916ba85b31..e9eb46263e 100644 --- a/cpukit/include/rtems/libio_.h +++ b/cpukit/include/rtems/libio_.h @@ -84,28 +84,6 @@ extern rtems_filesystem_mount_table_entry_t rtems_filesystem_null_mt_entry; */ extern rtems_filesystem_global_location_t rtems_filesystem_global_location_null; -/** - * @brief Sets the iop flags to the specified flags together with - * LIBIO_FLAGS_OPEN. - * - * Use this once a file descriptor allocated via rtems_libio_allocate() is - * fully initialized. - * - * @param[in] iop The iop. - * @param[in] flags The flags. - */ -static inline void rtems_libio_iop_flags_initialize( - rtems_libio_t *iop, - uint32_t flags -) -{ - _Atomic_Store_uint( - &iop->flags, - LIBIO_FLAGS_OPEN | flags, - ATOMIC_ORDER_RELEASE - ); -} - /** * @brief Sets the specified flags in the iop. * @@ -222,7 +200,7 @@ static inline void rtems_libio_iop_drop( rtems_libio_t *iop ) #define rtems_libio_check_is_open(_iop) \ do { \ - if (((_iop)->flags & LIBIO_FLAGS_OPEN) == 0) { \ + if ((rtems_libio_iop_flags(_iop) & LIBIO_FLAGS_OPEN) == 0) { \ errno = EBADF; \ return -1; \ } \ diff --git a/cpukit/libcsupport/src/fcntl.c b/cpukit/libcsupport/src/fcntl.c index ddbf5538f9..86b757e511 100644 --- a/cpukit/libcsupport/src/fcntl.c +++ b/cpukit/libcsupport/src/fcntl.c @@ -45,9 +45,9 @@ static int duplicate_iop( rtems_libio_t *iop ) */ rv = (*diop->pathinfo.handlers->open_h)( diop, NULL, oflag, 0 ); if ( rv == 0 ) { - rtems_libio_iop_flags_initialize( + rtems_libio_iop_flags_set( diop, - rtems_libio_fcntl_flags( oflag ) + LIBIO_FLAGS_OPEN | rtems_libio_fcntl_flags( oflag ) ); rv = rtems_libio_iop_to_descriptor( diop ); } else { diff --git a/cpukit/libcsupport/src/libio.c b/cpukit/libcsupport/src/libio.c index 0abb082a66..c9490c7aa5 100644 --- a/cpukit/libcsupport/src/libio.c +++ b/cpukit/libcsupport/src/libio.c @@ -134,11 +134,24 @@ void rtems_libio_free( rtems_libio_t *iop ) { + size_t zero; + rtems_filesystem_location_free( &iop->pathinfo ); rtems_libio_lock(); - iop = memset( iop, 0, sizeof( *iop ) ); + /* + * Clear everything except the reference count part. At this point in time + * there may be still some holders of this file descriptor. + */ + rtems_libio_iop_flags_clear( iop, LIBIO_FLAGS_REFERENCE_INC - 1U ); + zero = offsetof( rtems_libio_t, offset ); + memset( (char *) iop + zero, 0, sizeof( *iop ) - zero ); + + /* + * Append it to the free list. This increases the likelihood that a use + * after close is detected. + */ *rtems_libio_iop_free_tail = iop; rtems_libio_iop_free_tail = &iop->data1; diff --git a/cpukit/libcsupport/src/open.c b/cpukit/libcsupport/src/open.c index 86d0bbeab9..b21dcb1876 100644 --- a/cpukit/libcsupport/src/open.c +++ b/cpukit/libcsupport/src/open.c @@ -115,11 +115,7 @@ static int do_open( rtems_filesystem_eval_path_extract_currentloc( &ctx, &iop->pathinfo ); rtems_filesystem_eval_path_cleanup( &ctx ); - _Atomic_Store_uint( - &iop->flags, - rtems_libio_fcntl_flags( oflag ), - ATOMIC_ORDER_RELAXED - ); + rtems_libio_iop_flags_set( iop, rtems_libio_fcntl_flags( oflag ) ); rv = (*iop->pathinfo.handlers->open_h)( iop, path, oflag, mode ); diff --git a/cpukit/libcsupport/src/sup_fs_deviceio.c b/cpukit/libcsupport/src/sup_fs_deviceio.c index 294e14a260..4a7e0dd13c 100644 --- a/cpukit/libcsupport/src/sup_fs_deviceio.c +++ b/cpukit/libcsupport/src/sup_fs_deviceio.c @@ -34,7 +34,7 @@ int rtems_deviceio_open( rtems_libio_open_close_args_t args; args.iop = iop; - args.flags = iop->flags; + args.flags = rtems_libio_iop_flags( iop ); args.mode = mode; status = rtems_io_open( major, minor, &args ); @@ -75,7 +75,7 @@ ssize_t rtems_deviceio_read( args.offset = iop->offset; args.buffer = buf; args.count = nbyte; - args.flags = iop->flags; + args.flags = rtems_libio_iop_flags( iop ); args.bytes_moved = 0; status = rtems_io_read( major, minor, &args ); @@ -103,7 +103,7 @@ ssize_t rtems_deviceio_write( args.offset = iop->offset; args.buffer = RTEMS_DECONST( void *, buf ); args.count = nbyte; - args.flags = iop->flags; + args.flags = rtems_libio_iop_flags( iop ); args.bytes_moved = 0; status = rtems_io_write( major, minor, &args ); diff --git a/cpukit/libcsupport/src/termios.c b/cpukit/libcsupport/src/termios.c index b10a4ad774..81518f36de 100644 --- a/cpukit/libcsupport/src/termios.c +++ b/cpukit/libcsupport/src/termios.c @@ -2110,7 +2110,7 @@ rtems_termios_imfs_open (rtems_libio_t *iop, memset (&args, 0, sizeof (args)); args.iop = iop; - args.flags = iop->flags; + args.flags = rtems_libio_iop_flags (iop); args.mode = mode; rtems_termios_obtain (); @@ -2162,7 +2162,7 @@ rtems_termios_imfs_read (rtems_libio_t *iop, void *buffer, size_t count) args.iop = iop; args.buffer = buffer; args.count = count; - args.flags = iop->flags; + args.flags = rtems_libio_iop_flags (iop); sc = rtems_termios_linesw[tty->t_line].l_read (tty, &args); tty->tty_rcvwakeup = false; @@ -2202,7 +2202,7 @@ rtems_termios_imfs_write (rtems_libio_t *iop, const void *buffer, size_t count) args.iop = iop; args.buffer = RTEMS_DECONST (void *, buffer); args.count = count; - args.flags = iop->flags; + args.flags = rtems_libio_iop_flags (iop); sc = rtems_termios_linesw[tty->t_line].l_write (tty, &args); rtems_mutex_unlock (&tty->osem); diff --git a/cpukit/libnetworking/rtems/rtems_syscall.c b/cpukit/libnetworking/rtems/rtems_syscall.c index 5fa0e7d10c..56a8530564 100644 --- a/cpukit/libnetworking/rtems/rtems_syscall.c +++ b/cpukit/libnetworking/rtems/rtems_syscall.c @@ -87,7 +87,7 @@ rtems_bsdnet_makeFdForSocket (void *so) iop->pathinfo.handlers = &socket_handlers; iop->pathinfo.mt_entry = &rtems_filesystem_null_mt_entry; rtems_filesystem_location_add_to_mt_entry(&iop->pathinfo); - rtems_libio_iop_flags_initialize(iop, LIBIO_FLAGS_READ_WRITE); + rtems_libio_iop_flags_set(iop, LIBIO_FLAGS_OPEN | LIBIO_FLAGS_READ_WRITE); return fd; } @@ -739,14 +739,18 @@ rtems_bsdnet_write (rtems_libio_t *iop, const void *buffer, size_t count) static int so_ioctl (rtems_libio_t *iop, struct socket *so, uint32_t command, void *buffer) { + unsigned int nonblock; + switch (command) { case FIONBIO: + nonblock = rtems_libio_fcntl_flags (O_NONBLOCK); + if (*(int *)buffer) { - iop->flags |= O_NONBLOCK; + rtems_libio_iop_flags_set (iop, nonblock); so->so_state |= SS_NBIO; } else { - iop->flags &= ~O_NONBLOCK; + rtems_libio_iop_flags_clear (iop, nonblock); so->so_state &= ~SS_NBIO; } return 0; diff --git a/cpukit/posix/src/shmopen.c b/cpukit/posix/src/shmopen.c index 22b5868bda..054a035f16 100644 --- a/cpukit/posix/src/shmopen.c +++ b/cpukit/posix/src/shmopen.c @@ -286,14 +286,14 @@ int shm_open( const char *name, int oflag, mode_t mode ) iop->pathinfo.mt_entry = &rtems_filesystem_null_mt_entry; rtems_filesystem_location_add_to_mt_entry( &iop->pathinfo ); - flags = LIBIO_FLAGS_CLOSE_ON_EXEC; + flags = LIBIO_FLAGS_OPEN | LIBIO_FLAGS_CLOSE_ON_EXEC; if ( (oflag & O_ACCMODE) == O_RDONLY ) { flags |= LIBIO_FLAGS_READ; } else { flags |= LIBIO_FLAGS_READ_WRITE; } - rtems_libio_iop_flags_initialize( iop, flags ); + rtems_libio_iop_flags_set( iop, flags ); return fd; } diff --git a/testsuites/fstests/fsclose01/init.c b/testsuites/fstests/fsclose01/init.c index 445711d677..ac461c66be 100644 --- a/testsuites/fstests/fsclose01/init.c +++ b/testsuites/fstests/fsclose01/init.c @@ -1,15 +1,26 @@ /* - * Copyright (c) 2012, 2018 embedded brains GmbH. All rights reserved. + * Copyright (C) 2012, 2020 embedded brains GmbH (http://www.embedded-brains.de) * - * embedded brains GmbH - * Dornierstr. 4 - * 82178 Puchheim - * Germany - * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. * - * The license and distribution terms for this file may be - * found in the file LICENSE in this distribution or at - * http://www.rtems.org/license/LICENSE. + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. */ #ifdef HAVE_CONFIG_H @@ -32,7 +43,7 @@ const char rtems_test_name[] = "FSCLOSE 1"; typedef enum { - ACTION_ClOSE, + ACTION_CLOSE, ACTION_FCNTL, ACTION_FDATASYNC, ACTION_FCHDIR, @@ -328,7 +339,7 @@ static void worker_task(rtems_task_argument arg) wait(); switch (ctx->action) { - case ACTION_ClOSE: + case ACTION_CLOSE: ctx->wait_in_close = true; rv = close(ctx->fd); rtems_test_assert(rv == 0); @@ -460,25 +471,48 @@ static void test_close(test_context *ctx) ); rtems_test_assert(sc == RTEMS_SUCCESSFUL); - for (ac = ACTION_ClOSE; ac <= ACTION_WRITEV; ++ac) { + for (ac = ACTION_CLOSE; ac <= ACTION_WRITEV; ++ac) { + rtems_libio_t *iop; + unsigned int flags; + unsigned int expected_flags; + ctx->action = ac; ctx->fd = open(path, O_RDWR); rtems_test_assert(ctx->fd >= 0); + iop = rtems_libio_iop(ctx->fd); wakeup_worker(ctx); rv = close(ctx->fd); rtems_test_assert(rv == -1); - if (ac == ACTION_ClOSE) { + if (ac == ACTION_CLOSE) { rtems_test_assert(errno == EBADF); + + flags = rtems_libio_iop_hold(iop); + expected_flags = LIBIO_FLAGS_READ_WRITE; + rtems_test_assert(flags == expected_flags); + flags = rtems_libio_iop_flags(iop); + expected_flags = LIBIO_FLAGS_REFERENCE_INC | LIBIO_FLAGS_READ_WRITE; + rtems_test_assert(flags == expected_flags); } else { rtems_test_assert(errno == EBUSY); } wakeup_worker(ctx); + + if (ac == ACTION_CLOSE) { + flags = rtems_libio_iop_flags(iop); + expected_flags = LIBIO_FLAGS_REFERENCE_INC; + rtems_test_assert(flags == expected_flags); + rtems_libio_iop_drop(iop); + flags = rtems_libio_iop_flags(iop); + expected_flags = 0; + rtems_test_assert(flags == expected_flags); + } + rv = close(ctx->fd); - if (ac == ACTION_ClOSE) { + if (ac == ACTION_CLOSE) { rtems_test_assert(rv == -1); rtems_test_assert(errno == EBADF); } else { -- cgit v1.2.3