[rtems commit] libio: Robust file descriptor reference counting
Sebastian Huber
sebh at rtems.org
Fri Mar 13 08:57:19 UTC 2020
Module: rtems
Branch: master
Commit: c2287ba2cff59a50848151833404bce0e3cf0a70
Changeset: http://git.rtems.org/rtems/commit/?id=c2287ba2cff59a50848151833404bce0e3cf0a70
Author: Sebastian Huber <sebastian.huber at embedded-brains.de>
Date: Tue Mar 10 17:07:19 2020 +0100
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 b536232..bbaae10 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 916ba85..e9eb462 100644
--- a/cpukit/include/rtems/libio_.h
+++ b/cpukit/include/rtems/libio_.h
@@ -85,28 +85,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.
*
* @param[in] iop 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 ddbf553..86b757e 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 0abb082..c9490c7 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 86d0bbe..b21dcb1 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 294e14a..4a7e0dd 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 b10a4ad..81518f3 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 5fa0e7d..56a8530 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 22b5868..054a035 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 445711d..ac461c6 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 {
More information about the vc
mailing list