From c11c9a60c3e45439a871fa52da495171268b7df4 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Mon, 15 Mar 2021 10:46:42 -0400 Subject: [PATCH] Fix #863, check/report fcntl status The fcntl() function is documented as returning -1 on error, so test for this condition and report errno if so. There is no recourse/handling - this just reports the error. However - failure to set the O_NONBLOCK flag via this method will fall back to blocking mode being used (timeouts will not work as a result). --- src/os/portable/os-impl-bsd-sockets.c | 44 +++++++++++++++---- .../inc/ut-adaptor-portable-posix-io.h | 2 + .../src/ut-adaptor-portable-posix-io.c | 11 +++++ .../portable/src/coveragetest-bsd-sockets.c | 23 ++++++++++ 4 files changed, 72 insertions(+), 8 deletions(-) diff --git a/src/os/portable/os-impl-bsd-sockets.c b/src/os/portable/os-impl-bsd-sockets.c index 59bc5d26a..1b8e1d8e9 100644 --- a/src/os/portable/os-impl-bsd-sockets.c +++ b/src/os/portable/os-impl-bsd-sockets.c @@ -156,10 +156,24 @@ int32 OS_SocketOpen_Impl(const OS_object_token_t *token) * any blocking would be done explicitly via the select() wrappers */ os_flags = fcntl(impl->fd, F_GETFL); - os_flags |= OS_IMPL_SOCKET_FLAGS; - fcntl(impl->fd, F_SETFL, os_flags); - - impl->selectable = ((os_flags & O_NONBLOCK) != 0); + if (os_flags == -1) + { + /* No recourse if F_GETFL fails - just report the error and move on. */ + OS_DEBUG("fcntl(F_GETFL): %s\n", strerror(errno)); + } + else + { + os_flags |= OS_IMPL_SOCKET_FLAGS; + if (fcntl(impl->fd, F_SETFL, os_flags) == -1) + { + /* No recourse if F_SETFL fails - just report the error and move on. */ + OS_DEBUG("fcntl(F_SETFL): %s\n", strerror(errno)); + } + else + { + impl->selectable = ((os_flags & O_NONBLOCK) != 0); + } + } return OS_SUCCESS; } /* end OS_SocketOpen_Impl */ @@ -360,10 +374,24 @@ int32 OS_SocketAccept_Impl(const OS_object_token_t *sock_token, const OS_object_ * any blocking would be done explicitly via the select() wrappers */ os_flags = fcntl(conn_impl->fd, F_GETFL); - os_flags |= OS_IMPL_SOCKET_FLAGS; - fcntl(conn_impl->fd, F_SETFL, os_flags); - - conn_impl->selectable = ((os_flags & O_NONBLOCK) != 0); + if (os_flags == -1) + { + /* No recourse if F_GETFL fails - just report the error and move on. */ + OS_DEBUG("fcntl(F_GETFL): %s\n", strerror(errno)); + } + else + { + os_flags |= OS_IMPL_SOCKET_FLAGS; + if (fcntl(conn_impl->fd, F_SETFL, os_flags) == -1) + { + /* No recourse if F_SETFL fails - just report the error and move on. */ + OS_DEBUG("fcntl(F_SETFL): %s\n", strerror(errno)); + } + else + { + conn_impl->selectable = ((os_flags & O_NONBLOCK) != 0); + } + } } } } diff --git a/src/unit-test-coverage/portable/adaptors/inc/ut-adaptor-portable-posix-io.h b/src/unit-test-coverage/portable/adaptors/inc/ut-adaptor-portable-posix-io.h index a4cadae82..128864b85 100644 --- a/src/unit-test-coverage/portable/adaptors/inc/ut-adaptor-portable-posix-io.h +++ b/src/unit-test-coverage/portable/adaptors/inc/ut-adaptor-portable-posix-io.h @@ -39,5 +39,7 @@ * *****************************************************/ void UT_PortablePosixIOTest_Set_Selectable(osal_index_t local_id, bool is_selectable); +bool UT_PortablePosixIOTest_Get_Selectable(osal_index_t local_id); +void UT_PortablePosixIOTest_ResetImpl(osal_index_t local_id); #endif /* UT_ADAPTOR_PORTABLE_POSIX_IO_H */ diff --git a/src/unit-test-coverage/portable/adaptors/src/ut-adaptor-portable-posix-io.c b/src/unit-test-coverage/portable/adaptors/src/ut-adaptor-portable-posix-io.c index dea9b84d5..7dfc71d27 100644 --- a/src/unit-test-coverage/portable/adaptors/src/ut-adaptor-portable-posix-io.c +++ b/src/unit-test-coverage/portable/adaptors/src/ut-adaptor-portable-posix-io.c @@ -31,7 +31,18 @@ #include +void UT_PortablePosixIOTest_ResetImpl(osal_index_t local_id) +{ + OS_impl_filehandle_table[local_id].fd = -1; + OS_impl_filehandle_table[local_id].selectable = false; +} + void UT_PortablePosixIOTest_Set_Selectable(osal_index_t local_id, bool is_selectable) { OS_impl_filehandle_table[local_id].selectable = is_selectable; } + +bool UT_PortablePosixIOTest_Get_Selectable(osal_index_t local_id) +{ + return OS_impl_filehandle_table[local_id].selectable; +} diff --git a/src/unit-test-coverage/portable/src/coveragetest-bsd-sockets.c b/src/unit-test-coverage/portable/src/coveragetest-bsd-sockets.c index 7b4bc3141..5a13a14fa 100644 --- a/src/unit-test-coverage/portable/src/coveragetest-bsd-sockets.c +++ b/src/unit-test-coverage/portable/src/coveragetest-bsd-sockets.c @@ -29,6 +29,9 @@ #include "os-shared-file.h" #include "OCS_sys_socket.h" +#include "OCS_fcntl.h" + +#include "ut-adaptor-portable-posix-io.h" void Test_OS_SocketOpen_Impl(void) { @@ -36,6 +39,7 @@ void Test_OS_SocketOpen_Impl(void) /* Set up token for index 0 */ token.obj_idx = UT_INDEX_0; + UT_PortablePosixIOTest_ResetImpl(token.obj_idx); /* Invalid socket type */ OS_stream_table[0].socket_type = -1; @@ -55,6 +59,25 @@ void Test_OS_SocketOpen_Impl(void) OS_stream_table[0].socket_type = OS_SocketType_STREAM; OS_stream_table[0].socket_domain = OS_SocketDomain_INET6; OSAPI_TEST_FUNCTION_RC(OS_SocketOpen_Impl, (&token), OS_SUCCESS); + UtAssert_True(UT_PortablePosixIOTest_Get_Selectable(token.obj_idx), "Socket is selectable"); + + /* Failure in fcntl() GETFL */ + UT_PortablePosixIOTest_ResetImpl(token.obj_idx); + UT_ResetState(UT_KEY(OCS_fcntl)); + UT_SetDeferredRetcode(UT_KEY(OCS_fcntl), 1, -1); + OSAPI_TEST_FUNCTION_RC(OS_SocketOpen_Impl, (&token), OS_SUCCESS); + UtAssert_STUB_COUNT(OCS_fcntl, 1); + UtAssert_True(!UT_PortablePosixIOTest_Get_Selectable(token.obj_idx), + "Socket not selectable without O_NONBLOCK flag"); + + /* Failure in fcntl() SETFL */ + UT_PortablePosixIOTest_ResetImpl(token.obj_idx); + UT_ResetState(UT_KEY(OCS_fcntl)); + UT_SetDeferredRetcode(UT_KEY(OCS_fcntl), 2, -1); + OSAPI_TEST_FUNCTION_RC(OS_SocketOpen_Impl, (&token), OS_SUCCESS); + UtAssert_STUB_COUNT(OCS_fcntl, 2); + UtAssert_True(!UT_PortablePosixIOTest_Get_Selectable(token.obj_idx), + "Socket not selectable without O_NONBLOCK flag"); } /* ------------------- End of test cases --------------------------------------*/