Merge pull request nasa#639 from nasa/integration-candidate

Integration Candidate: 2020-11-03

README.md

@@ -10,6 +10,18 @@ The autogenerated OSAL user's guide can be viewed at <https://github.com/nasa/cF
 
 ## Version History
 
+### Development Build: 5.1.0-rc1+dev68
+
+- When `OS_DEBUG` is enabled, this adds a message if mutex give/take actions occur outside the expected sequence. This informs the user (via the debug console) if a lock is taken more than once or if a lock is given by a different task than the one that originally took it:
+```
+OS_MutSemTake():216:WARNING: Task 65547 taking mutex 327685 while owned by task 65547
+```
+- Removes all FIXME comments
+- Resolves security/filename race issue by opening file and acting on descriptor by adding fstat stub
+- Squashed the minor recommended bugs
+- UtAssert macros now accept variable string arguments.The `UtAssert_True` wrapper around call is no longer needed to accommodate dynamic string output, thus removing the double assert. UtAssert macros will now be able to offer more information by themselves.
+- See <https://github.com/nasa/osal/pull/639>
+
 ### Development Build: 5.1.0-rc1+dev60
 
 - Appliy standard formating, whitespace-only changes

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;

src/os/inc/osapi-version.h

@@ -30,17 +30,16 @@
 /*
  * Development Build Macro Definitions
  */
-#define OS_BUILD_NUMBER   60
+#define OS_BUILD_NUMBER   67
 #define OS_BUILD_BASELINE "v5.1.0-rc1"
 
 /*
  * Version Macro Definitions
  */
 #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 */
 
 /*

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;
     }

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 */

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;
 
 /*

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);
             }
         }
 

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;

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.

src/unit-test-coverage/portable/src/coveragetest-posix-files.c

@@ -96,23 +96,28 @@ 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();
     RefStat.st_gid   = UT_PortablePosixFileTest_GetSelfEGID();
     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);
 }
 

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 */

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

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;

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)
      */

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, ...) \