[PATCH] libio: Robust file descriptor reference counting

Sebastian Huber sebastian.huber at embedded-brains.de
Tue Mar 10 19:09:45 UTC 2020


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 enable 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
- *  <rtems at embedded-brains.de>
+ * 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 {
-- 
2.16.4



More information about the devel mailing list