From f2efc1ca0882da11489eef4614ce69cc47903546 Mon Sep 17 00:00:00 2001 From: Jacob Hageman Date: Thu, 22 Oct 2020 17:25:48 -0400 Subject: [PATCH 1/4] Fix #327 and #333, Squash LGTM warnings --- src/os/inc/osapi-os-filesys.h | 2 +- src/os/portable/os-impl-bsd-sockets.c | 1 + src/os/portable/os-impl-posix-files.c | 14 +++++++++++-- src/os/shared/src/osapi-filesys.c | 10 ++++++--- src/os/shared/src/osapi-select.c | 1 - .../portable/src/coveragetest-posix-files.c | 21 ++++++++++++------- .../ut-stubs/inc/OCS_stat.h | 1 + .../ut-stubs/override_inc/stat.h | 1 + .../ut-stubs/src/posix-stat-stubs.c | 14 +++++++++++++ .../oscore-test/ut_oscore_countsem_test.c | 2 +- 10 files changed, 51 insertions(+), 16 deletions(-) diff --git a/src/os/inc/osapi-os-filesys.h b/src/os/inc/osapi-os-filesys.h index 36ea5a9e0..764ae0d4e 100644 --- a/src/os/inc/osapi-os-filesys.h +++ b/src/os/inc/osapi-os-filesys.h @@ -157,7 +157,7 @@ typedef struct */ typedef enum { - OS_FILE_FLAG_NONE, + OS_FILE_FLAG_NONE = 0x00, OS_FILE_FLAG_CREATE = 0x01, OS_FILE_FLAG_TRUNCATE = 0x02, } OS_file_flag_t; diff --git a/src/os/portable/os-impl-bsd-sockets.c b/src/os/portable/os-impl-bsd-sockets.c index efac29de9..524fe42a3 100644 --- a/src/os/portable/os-impl-bsd-sockets.c +++ b/src/os/portable/os-impl-bsd-sockets.c @@ -538,6 +538,7 @@ int32 OS_SocketAddrInit_Impl(OS_SockAddr_t *Addr, OS_SocketDomain_t Domain) break; #endif default: + sa_family = 0; addrlen = 0; break; } diff --git a/src/os/portable/os-impl-posix-files.c b/src/os/portable/os-impl-posix-files.c index 365732652..43628d5df 100644 --- a/src/os/portable/os-impl-posix-files.c +++ b/src/os/portable/os-impl-posix-files.c @@ -196,6 +196,14 @@ int32 OS_FileChmod_Impl(const char *local_path, uint32 access) mode_t readbits; mode_t writebits; struct stat st; + int fd; + + /* Open file to avoid filename race potential */ + fd = open(local_path, O_RDONLY); + if (fd < 0) + { + return OS_ERROR; + } /* * In order to preserve any OTHER mode bits, @@ -206,7 +214,7 @@ int32 OS_FileChmod_Impl(const char *local_path, uint32 access) * which is generally not part of the OSAL API, but * is important for the underlying OS. */ - if (stat(local_path, &st) < 0) + if (fstat(fd, &st) < 0) { return OS_ERROR; } @@ -252,11 +260,13 @@ int32 OS_FileChmod_Impl(const char *local_path, uint32 access) } /* finally, write the modified mode back to the file */ - if (chmod(local_path, st.st_mode) < 0) + if (fchmod(fd, st.st_mode) < 0) { return OS_ERROR; } + close(fd); + return OS_SUCCESS; } /* end OS_FileChmod_Impl */ diff --git a/src/os/shared/src/osapi-filesys.c b/src/os/shared/src/osapi-filesys.c index 9179dbd81..30d562aa4 100644 --- a/src/os/shared/src/osapi-filesys.c +++ b/src/os/shared/src/osapi-filesys.c @@ -178,9 +178,13 @@ int32 OS_FileSys_Initialize(char *address, const char *fsdevname, const char *fs } else { - /* to avoid leaving in an intermediate state, - * this also stops the volume if formatting failed. */ - OS_FileSysStopVolume_Impl(local_id); + /* + * To avoid leaving in an intermediate state, + * this also stops the volume if formatting failed. + * Cast to void to repress analysis warnings for + * ignored return value. + */ + (void)OS_FileSysStopVolume_Impl(local_id); } } diff --git a/src/os/shared/src/osapi-select.c b/src/os/shared/src/osapi-select.c index 161e4b04b..5a3fe885f 100644 --- a/src/os/shared/src/osapi-select.c +++ b/src/os/shared/src/osapi-select.c @@ -90,7 +90,6 @@ int32 OS_SelectMultiple(OS_FdSet *ReadSet, OS_FdSet *WriteSet, int32 msecs) int32 return_code; /* - * FIXME: * This does not currently increment any refcounts. * That means a file/socket can be closed while actively inside a * OS_SelectMultiple() call in another thread. diff --git a/src/unit-test-coverage/portable/src/coveragetest-posix-files.c b/src/unit-test-coverage/portable/src/coveragetest-posix-files.c index 9e8ed38e3..7f1080e27 100644 --- a/src/unit-test-coverage/portable/src/coveragetest-posix-files.c +++ b/src/unit-test-coverage/portable/src/coveragetest-posix-files.c @@ -96,15 +96,20 @@ void Test_OS_FileChmod_Impl(void) */ struct OCS_stat RefStat; - /* failure mode 1 (stat) */ - UT_SetForceFail(UT_KEY(OCS_stat), -1); + /* failure mode 0 (open) */ + UT_SetForceFail(UT_KEY(OCS_open), -1); OSAPI_TEST_FUNCTION_RC(OS_FileChmod_Impl, ("local", OS_READ_WRITE), OS_ERROR); - UT_ClearForceFail(UT_KEY(OCS_stat)); + UT_ClearForceFail(UT_KEY(OCS_open)); - /* failure mode 2 (chmod) */ - UT_SetForceFail(UT_KEY(OCS_chmod), -1); + /* failure mode 1 (fstat) */ + UT_SetForceFail(UT_KEY(OCS_fstat), -1); OSAPI_TEST_FUNCTION_RC(OS_FileChmod_Impl, ("local", OS_READ_WRITE), OS_ERROR); - UT_ClearForceFail(UT_KEY(OCS_chmod)); + UT_ClearForceFail(UT_KEY(OCS_fstat)); + + /* failure mode 2 (fchmod) */ + UT_SetForceFail(UT_KEY(OCS_fchmod), -1); + OSAPI_TEST_FUNCTION_RC(OS_FileChmod_Impl, ("local", OS_READ_WRITE), OS_ERROR); + UT_ClearForceFail(UT_KEY(OCS_fchmod)); /* all permission bits with uid/gid match */ RefStat.st_uid = UT_PortablePosixFileTest_GetSelfEUID(); @@ -112,7 +117,7 @@ void Test_OS_FileChmod_Impl(void) RefStat.st_mode = ~0; RefStat.st_size = 1234; RefStat.st_mtime = 5678; - UT_SetDataBuffer(UT_KEY(OCS_stat), &RefStat, sizeof(RefStat), false); + UT_SetDataBuffer(UT_KEY(OCS_fstat), &RefStat, sizeof(RefStat), false); /* nominal 1 - full permissions with file owned by own uid/gid */ OSAPI_TEST_FUNCTION_RC(OS_FileChmod_Impl, ("local", OS_READ_WRITE), OS_SUCCESS); @@ -124,7 +129,7 @@ void Test_OS_FileChmod_Impl(void) /* nominal 3 - non-owned file */ ++RefStat.st_uid; ++RefStat.st_gid; - UT_SetDataBuffer(UT_KEY(OCS_stat), &RefStat, sizeof(RefStat), false); + UT_SetDataBuffer(UT_KEY(OCS_fstat), &RefStat, sizeof(RefStat), false); OSAPI_TEST_FUNCTION_RC(OS_FileChmod_Impl, ("local", OS_READ_WRITE), OS_SUCCESS); } diff --git a/src/unit-test-coverage/ut-stubs/inc/OCS_stat.h b/src/unit-test-coverage/ut-stubs/inc/OCS_stat.h index 3a9f79d55..c05d4f53a 100644 --- a/src/unit-test-coverage/ut-stubs/inc/OCS_stat.h +++ b/src/unit-test-coverage/ut-stubs/inc/OCS_stat.h @@ -64,6 +64,7 @@ extern int OCS_fchmod(int fd, OCS_mode_t mode); extern int OCS_chmod(const char *path, OCS_mode_t mode); extern int OCS_mkdir(const char *path, ...); extern int OCS_stat(const char *file, struct OCS_stat *buf); +extern int OCS_fstat(int fd, struct OCS_stat *buf); /* ----------------------------------------- */ /* prototypes normally declared in sys/statvfs.h */ diff --git a/src/unit-test-coverage/ut-stubs/override_inc/stat.h b/src/unit-test-coverage/ut-stubs/override_inc/stat.h index b391c2c95..a53ada220 100644 --- a/src/unit-test-coverage/ut-stubs/override_inc/stat.h +++ b/src/unit-test-coverage/ut-stubs/override_inc/stat.h @@ -29,6 +29,7 @@ /* ----------------------------------------- */ #define stat OCS_stat +#define fstat OCS_fstat #define fchmod OCS_fchmod #define chmod OCS_chmod #define mkdir OCS_mkdir diff --git a/src/unit-test-coverage/ut-stubs/src/posix-stat-stubs.c b/src/unit-test-coverage/ut-stubs/src/posix-stat-stubs.c index af937bd9f..397b7446b 100644 --- a/src/unit-test-coverage/ut-stubs/src/posix-stat-stubs.c +++ b/src/unit-test-coverage/ut-stubs/src/posix-stat-stubs.c @@ -66,6 +66,20 @@ int OCS_stat(const char *file, struct OCS_stat *buf) return Status; } +int OCS_fstat(int fd, struct OCS_stat *buf) +{ + int32 Status; + + Status = UT_DEFAULT_IMPL(OCS_fstat); + + if (Status == 0 && UT_Stub_CopyToLocal(UT_KEY(OCS_fstat), buf, sizeof(*buf)) < sizeof(*buf)) + { + memset(buf, 0, sizeof(*buf)); + } + + return Status; +} + int OCS_statvfs(const char *file, struct OCS_statvfs *buf) { int32 Status; diff --git a/src/unit-tests/oscore-test/ut_oscore_countsem_test.c b/src/unit-tests/oscore-test/ut_oscore_countsem_test.c index e35112dc3..b76651b9e 100644 --- a/src/unit-tests/oscore-test/ut_oscore_countsem_test.c +++ b/src/unit-tests/oscore-test/ut_oscore_countsem_test.c @@ -121,7 +121,7 @@ void UT_os_count_sem_create_test() testDesc = "#4 Initial-count-too-high"; /* - * FIXME: This test can only be done if the OS defines a specific "SEM_VALUE_MAX" + * This test can only be done if the OS defines a specific "SEM_VALUE_MAX" * The OSAL should define this for itself, but it currently does not. * (This macro is not currently defined in RTEMS) */ From 2189b0ec2a92fc683e40f48f54a7cbb0f161b4c4 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Fri, 23 Oct 2020 16:34:38 -0400 Subject: [PATCH 2/4] Fix #623, add debug messages for mutex double locks if OS_DEBUG is enabled, this adds a message if mutex give/take actions occur outside the expected sequence. In particular, this warns if a task takes a mutex more than once. --- src/os/shared/inc/os-shared-mutex.h | 3 ++- src/os/shared/src/osapi-mutex.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/os/shared/inc/os-shared-mutex.h b/src/os/shared/inc/os-shared-mutex.h index 3f8af42b0..218c60343 100644 --- a/src/os/shared/inc/os-shared-mutex.h +++ b/src/os/shared/inc/os-shared-mutex.h @@ -32,7 +32,8 @@ typedef struct { - char obj_name[OS_MAX_API_NAME]; + char obj_name[OS_MAX_API_NAME]; + osal_id_t last_owner; } OS_mutex_internal_record_t; /* diff --git a/src/os/shared/src/osapi-mutex.c b/src/os/shared/src/osapi-mutex.c index 53a52a377..28cbc73ee 100644 --- a/src/os/shared/src/osapi-mutex.c +++ b/src/os/shared/src/osapi-mutex.c @@ -162,11 +162,24 @@ int32 OS_MutSemGive(osal_id_t sem_id) OS_common_record_t *record; uint32 local_id; int32 return_code; + osal_id_t self_task; /* Check Parameters */ return_code = OS_ObjectIdGetById(OS_LOCK_MODE_NONE, LOCAL_OBJID_TYPE, sem_id, &local_id, &record); if (return_code == OS_SUCCESS) { + self_task = OS_TaskGetId(); + + if (!OS_ObjectIdEqual(OS_mutex_table[local_id].last_owner, self_task)) + { + OS_DEBUG("WARNING: Task %lu giving mutex %lu while owned by task %lu\n", + OS_ObjectIdToInteger(self_task), + OS_ObjectIdToInteger(sem_id), + OS_ObjectIdToInteger(OS_mutex_table[local_id].last_owner)); + } + + OS_mutex_table[local_id].last_owner = OS_OBJECT_ID_UNDEFINED; + return_code = OS_MutSemGive_Impl(local_id); } @@ -187,12 +200,27 @@ int32 OS_MutSemTake(osal_id_t sem_id) OS_common_record_t *record; uint32 local_id; int32 return_code; + osal_id_t self_task; /* Check Parameters */ return_code = OS_ObjectIdGetById(OS_LOCK_MODE_NONE, LOCAL_OBJID_TYPE, sem_id, &local_id, &record); if (return_code == OS_SUCCESS) { return_code = OS_MutSemTake_Impl(local_id); + if (return_code == OS_SUCCESS) + { + self_task = OS_TaskGetId(); + + if (OS_ObjectIdDefined(OS_mutex_table[local_id].last_owner)) + { + OS_DEBUG("WARNING: Task %lu taking mutex %lu while owned by task %lu\n", + OS_ObjectIdToInteger(self_task), + OS_ObjectIdToInteger(sem_id), + OS_ObjectIdToInteger(OS_mutex_table[local_id].last_owner)); + } + + OS_mutex_table[local_id].last_owner = self_task; + } } return return_code; From 4948e93d269b4965676fe17895ee19025688632b Mon Sep 17 00:00:00 2001 From: Alan Gibson Date: Wed, 21 Oct 2020 16:00:47 -0400 Subject: [PATCH 3/4] Fix #628, Update UtAssert macros with dynamic string formatting --- ut_assert/inc/utassert.h | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/ut_assert/inc/utassert.h b/ut_assert/inc/utassert.h index 37c852ec4..2cf56b747 100644 --- a/ut_assert/inc/utassert.h +++ b/ut_assert/inc/utassert.h @@ -90,49 +90,50 @@ typedef struct #define UtAssert_True(Expression, ...) UtAssertEx(Expression, UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* Evaluates a expression as either true or false. true means the test passed, false means the test failed. */ -#define UtAssert_Bool(Expression, Description) UtAssert(Expression, Description, __FILE__, __LINE__) +#define UtAssert_Bool(Expression, ...) \ + UtAssertEx(Expression, UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* Asserts a test failure */ #define UtAssert_Failed(...) UtAssertEx(false, UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* Compares two integers and determines if they are equal within a specified absolute tolerance. */ -#define UtAssert_IntegerCmpAbs(x, y, Tolerance, Description) \ - UtAssert((abs((x) - (y)) <= (Tolerance)), Description, __FILE__, __LINE__) +#define UtAssert_IntegerCmpAbs(x, y, Tolerance, ...) \ + UtAssertEx((abs((x) - (y)) <= (Tolerance)), UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* Compares two floating point numbers and determines if they are equal within a specified absolute tolerance. */ -#define UtAssert_DoubleCmpAbs(x, y, Tolerance, Description) \ - UtAssert((fabs((x) - (y)) <= (Tolerance)), Description, __FILE__, __LINE__) +#define UtAssert_DoubleCmpAbs(x, y, Tolerance, ...) \ + UtAssertEx((fabs((x) - (y)) <= (Tolerance)), UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* Compares two floating point numbers and determines if they are equal within a specified relative tolerance. */ -#define UtAssert_DoubleCmpRel(x, y, Ratio, Description) \ - UtAssert((fabs((x) - (y)) / (x) <= (Ratio)), Description, __FILE__, __LINE__) +#define UtAssert_DoubleCmpRel(x, y, Ratio, ...) \ + UtAssertEx((fabs((x) - (y))/(x) <= (Ratio)), UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* Compares two strings and determines if they are equal. */ -#define UtAssert_StrCmp(String1, String2, Description) \ - UtAssert((strcmp(String1, String2) == 0), Description, __FILE__, __LINE__) +#define UtAssert_StrCmp(String1, String2, ...) \ + UtAssertEx((strcmp(String1, String2) == 0), UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* Compares at most Length characters of two strings and determines if they are equal. */ -#define UtAssert_StrnCmp(String1, String2, Length, Description) \ - UtAssert((strncmp(String1, String2, Length) == 0), Description, __FILE__, __LINE__) +#define UtAssert_StrnCmp(String1, String2, Length, ...) \ + UtAssertEx((strncmp(String1, String2, Length) == 0), UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* Compares two regions of memory and determines if they are equal. */ -#define UtAssert_MemCmp(Memory1, Memory2, Length, Description) \ - UtAssert((memcmp(Memory1, Memory2, Length) == 0), Description, __FILE__, __LINE__) +#define UtAssert_MemCmp(Memory1, Memory2, Length, ...) \ + UtAssertEx((memcmp(Memory1, Memory2, Length) == 0), UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* Compares a region of memory to a static pattern and determines if they are equal. Note: Use UtMemSet to * fill a region of memory with a static pattern. */ -#define UtAssert_MemCmpValue(Memory, Value, Length, Description) \ - UtAssert((UtMemCmpValue(Memory, Value, Length)), Description, __FILE__, __LINE__) +#define UtAssert_MemCmpValue(Memory, Value, Length, ...) \ + UtAssertEx((UtMemCmpValue(Memory, Value, Length)), UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* Compares a region of memory to a byte count pattern and determines if they are equal. Note: Use UtMemFill to * fill a region of memory with a byte count pattern. */ -#define UtAssert_MemCmpCount(Memory, Length, Description) \ - UtAssert((UtMemCmpCount(Memory, Length)), Description, __FILE__, __LINE__) +#define UtAssert_MemCmpCount(Memory, Length, ...) \ + UtAssertEx((UtMemCmpCount(Memory, Length)), UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* Compares a region of memory with the contents of a binary file and determines if they are equal. Note: Use * UtMem2BinFile to copy a region of memory to a binary file. */ -#define UtAssert_Mem2BinFileCmp(Memory, Filename, Description) \ - UtAssert((UtMem2BinFileCmp(Memory, Filename)), Description, __FILE__, __LINE__) +#define UtAssert_Mem2BinFileCmp(Memory, Filename, ...) \ + UtAssertEx((UtMem2BinFileCmp(Memory, Filename)), UtAssert_GetContext(), __FILE__, __LINE__, __VA_ARGS__) /* A wrapper around UtAssertEx that allows the user to specify the failure type and a more descriptive message */ #define UtAssert_Type(Type, Expression, ...) \ From 2562b1f8f0c7b6da3304f13628e0c09d4e77fce2 Mon Sep 17 00:00:00 2001 From: astrogeco <59618057+astrogeco@users.noreply.github.com> Date: Tue, 3 Nov 2020 14:59:23 -0500 Subject: [PATCH 4/4] Bump to v5.1.0-rc1+dev68 and update ReadMe Small edit to REVISION version comment in osapi-version.h --- README.md | 12 ++++++++++++ src/os/inc/osapi-version.h | 7 +++---- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index dac9b1f06..69ce4e7ed 100644 --- a/README.md +++ b/README.md @@ -10,6 +10,18 @@ The autogenerated OSAL user's guide can be viewed at + ### Development Build: 5.1.0-rc1+dev60 - Appliy standard formating, whitespace-only changes diff --git a/src/os/inc/osapi-version.h b/src/os/inc/osapi-version.h index 26adec058..98d95a4a8 100644 --- a/src/os/inc/osapi-version.h +++ b/src/os/inc/osapi-version.h @@ -30,7 +30,7 @@ /* * Development Build Macro Definitions */ -#define OS_BUILD_NUMBER 60 +#define OS_BUILD_NUMBER 67 #define OS_BUILD_BASELINE "v5.1.0-rc1" /* @@ -38,9 +38,8 @@ */ #define OS_MAJOR_VERSION 5 /*!< @brief ONLY APPLY for OFFICIAL releases. Major version number. */ #define OS_MINOR_VERSION 0 /*!< @brief ONLY APPLY for OFFICIAL releases. Minor version number. */ -#define OS_REVISION \ - 99 /*!< @brief ONLY APPLY for OFFICIAL releases. Revision version number. If set to "99" it indicates a \ - development version. */ +#define OS_REVISION 99 /*!< @brief ONLY APPLY for OFFICIAL releases. Revision version number. A value of "99" indicates an unreleased development version. */ + #define OS_MISSION_REV 0 /*!< @brief ONLY USED by MISSION Implementations. Mission revision */ /*