Fix nasa#2550, Use resource ID for dump control index

Use a resourceID value for access into the dump control structure array. This allows for consistent lookup, matching, and free/in-use determination, as well as improved resilience to race conditions and stale data.
jphickey · Apr 19, 2024 · 1319746 · 1319746
1 parent d6ee17e
commit 1319746
Show file tree

Hide file tree

Showing 10 changed files with 496 additions and 185 deletions.
diff --git a/modules/resourceid/fsw/inc/cfe_core_resourceid_basevalues.h b/modules/resourceid/fsw/inc/cfe_core_resourceid_basevalues.h
@@ -73,6 +73,7 @@ enum
 
     /* TBL managed resources */
     CFE_RESOURCEID_TBL_VALRESULTID_BASE_OFFSET = OS_OBJECT_TYPE_USER + 8,
+    CFE_RESOURCEID_TBL_DUMPCTRLID_BASE_OFFSET  = OS_OBJECT_TYPE_USER + 9,
 
 };
 
@@ -99,6 +100,7 @@ enum
 
     /* TBL managed resources */
     CFE_TBL_VALRESULTID_BASE = CFE_RESOURCEID_MAKE_BASE(CFE_RESOURCEID_TBL_VALRESULTID_BASE_OFFSET),
+    CFE_TBL_DUMPCTRLID_BASE  = CFE_RESOURCEID_MAKE_BASE(CFE_RESOURCEID_TBL_DUMPCTRLID_BASE_OFFSET),
 
 };
 

diff --git a/modules/tbl/fsw/src/cfe_tbl_api.c b/modules/tbl/fsw/src/cfe_tbl_api.c
@@ -1096,44 +1096,42 @@ CFE_Status_t CFE_TBL_DumpToBuffer(CFE_TBL_Handle_t TblHandle)
     int32                  Status;
     CFE_TBL_RegistryRec_t *RegRecPtr   = NULL;
     CFE_TBL_DumpControl_t *DumpCtrlPtr = NULL;
-    CFE_ES_AppId_t         ThisAppId;
+    CFE_TBL_LoadBuff_t *   ActiveBufPtr;
 
     Status = CFE_TBL_TxnStartFromHandle(&Txn, TblHandle, CFE_TBL_TxnContext_ACCESSOR_APP);
 
     if (Status == CFE_SUCCESS)
     {
         Status = CFE_TBL_TxnGetTableStatus(&Txn);
 
-        RegRecPtr = CFE_TBL_TxnRegRec(&Txn);
-
-        CFE_TBL_TxnFinish(&Txn);
-    }
-    else
-    {
-        ThisAppId = CFE_TBL_TxnAppId(&Txn);
-
-        CFE_ES_WriteToSysLog("%s: App(%lu) does not have access to Tbl Handle=%d\n", __func__,
-                             CFE_RESOURCEID_TO_ULONG(ThisAppId), (int)TblHandle);
-    }
+        /* Make sure the table has been requested to be dumped */
+        if (Status == CFE_TBL_INFO_DUMP_PENDING)
+        {
+            RegRecPtr    = CFE_TBL_TxnRegRec(&Txn);
+            DumpCtrlPtr  = CFE_TBL_LocateDumpCtrlByID(RegRecPtr->DumpControlId);
+            ActiveBufPtr = CFE_TBL_GetActiveBuffer(RegRecPtr);
 
-    /* Make sure the table has been requested to be dumped */
-    if (Status == CFE_TBL_INFO_DUMP_PENDING)
-    {
-        DumpCtrlPtr = &CFE_TBL_Global.DumpControlBlocks[RegRecPtr->DumpControlIndex];
+            /* Copy the contents of the active buffer to the assigned dump buffer */
+            memcpy(DumpCtrlPtr->DumpBufferPtr->BufferPtr, ActiveBufPtr->BufferPtr, DumpCtrlPtr->Size);
 
-        /* Copy the contents of the active buffer to the assigned dump buffer */
-        memcpy(DumpCtrlPtr->DumpBufferPtr->BufferPtr, RegRecPtr->Buffers[0].BufferPtr, DumpCtrlPtr->Size);
+            /* Save the current time so that the header in the dump file can have the correct time */
+            DumpCtrlPtr->DumpBufferPtr->FileTime = CFE_TIME_GetTime();
 
-        /* Save the current time so that the header in the dump file can have the correct time */
-        DumpCtrlPtr->DumpBufferPtr->FileTime = CFE_TIME_GetTime();
+            /* Disassociate the dump request from the table */
+            RegRecPtr->DumpControlId = CFE_TBL_NO_DUMP_PENDING;
 
-        /* Disassociate the dump request from the table */
-        RegRecPtr->DumpControlIndex = CFE_TBL_NO_DUMP_PENDING;
+            /* Notify the Table Services Application that the dump buffer is ready to be written to a file */
+            DumpCtrlPtr->State = CFE_TBL_DUMP_PERFORMED;
 
-        /* Notify the Table Services Application that the dump buffer is ready to be written to a file */
-        DumpCtrlPtr->State = CFE_TBL_DUMP_PERFORMED;
+            Status = CFE_SUCCESS;
+        }
 
-        Status = CFE_SUCCESS;
+        CFE_TBL_TxnFinish(&Txn);
+    }
+    else
+    {
+        CFE_ES_WriteToSysLog("%s: App(%lu) does not have access to Tbl Handle=%lu\n", __func__,
+                             CFE_TBL_TxnAppIdAsULong(&Txn), CFE_TBL_TxnHandleAsULong(&Txn));
     }
 
     return Status;

diff --git a/modules/tbl/fsw/src/cfe_tbl_internal.c b/modules/tbl/fsw/src/cfe_tbl_internal.c
@@ -92,8 +92,6 @@ int32 CFE_TBL_EarlyInit(void)
     /* Initialize the Dump-Only Table Dump Control Blocks nonzero values */
     for (i = 0; i < CFE_PLATFORM_TBL_MAX_SIMULTANEOUS_LOADS; i++)
     {
-        CFE_TBL_Global.DumpControlBlocks[i].State = CFE_TBL_DUMP_FREE;
-
         /* Prevent Shared Buffers from being used until successfully allocated */
         CFE_TBL_Global.LoadBuffs[i].Taken = true;
     }
@@ -223,7 +221,7 @@ void CFE_TBL_InitRegistryRecord(CFE_TBL_RegistryRec_t *RegRecPtr)
     RegRecPtr->ValidateActiveId   = CFE_TBL_NO_VALIDATION_PENDING;
     RegRecPtr->ValidateInactiveId = CFE_TBL_NO_VALIDATION_PENDING;
     RegRecPtr->CDSHandle          = CFE_ES_CDS_BAD_HANDLE;
-    RegRecPtr->DumpControlIndex   = CFE_TBL_NO_DUMP_PENDING;
+    RegRecPtr->DumpControlId      = CFE_TBL_NO_DUMP_PENDING;
 
     CFE_TBL_HandleLinkInit(&RegRecPtr->AccessList);
 }
@@ -1021,19 +1019,22 @@ void CFE_TBL_ByteSwapUint32(uint32 *Uint32ToSwapPtr)
  *-----------------------------------------------------------------*/
 int32 CFE_TBL_CleanUpApp(CFE_ES_AppId_t AppId)
 {
-    uint32             i;
-    CFE_TBL_TxnState_t Txn;
+    uint32                 i;
+    CFE_TBL_TxnState_t     Txn;
+    CFE_TBL_DumpControl_t *DumpCtrlPtr;
 
     /* Scan Dump Requests to determine if any of the tables that */
     /* were to be dumped will be deleted */
     for (i = 0; i < CFE_PLATFORM_TBL_MAX_SIMULTANEOUS_LOADS; i++)
     {
+        DumpCtrlPtr = &CFE_TBL_Global.DumpControlBlocks[i];
+
         /* Check to see if the table to be dumped is owned by the App to be deleted */
-        if ((CFE_TBL_Global.DumpControlBlocks[i].State != CFE_TBL_DUMP_FREE) &&
-            CFE_RESOURCEID_TEST_EQUAL(CFE_TBL_Global.DumpControlBlocks[i].RegRecPtr->OwnerAppId, AppId))
+        if (CFE_TBL_DumpCtrlBlockIsUsed(DumpCtrlPtr) &&
+            CFE_RESOURCEID_TEST_EQUAL(DumpCtrlPtr->RegRecPtr->OwnerAppId, AppId))
         {
             /* If so, then remove the dump request */
-            CFE_TBL_Global.DumpControlBlocks[i].State = CFE_TBL_DUMP_FREE;
+            CFE_TBL_DumpCtrlBlockSetFree(DumpCtrlPtr);
         }
     }
 

diff --git a/modules/tbl/fsw/src/cfe_tbl_resource.c b/modules/tbl/fsw/src/cfe_tbl_resource.c
@@ -52,6 +52,18 @@ CFE_Status_t CFE_TBL_Handle_ToIndex(CFE_TBL_Handle_t TblHandle, uint32 *Idx)
     return CFE_SUCCESS;
 }
 
+/*----------------------------------------------------------------
+ *
+ * Implemented per public API
+ * See description in header file for argument/return detail
+ *
+ *-----------------------------------------------------------------*/
+CFE_Status_t CFE_TBL_DumpCtrlId_ToIndex(CFE_TBL_DumpCtrlId_t DumpCtrlId, uint32 *Idx)
+{
+    return CFE_ResourceId_ToIndex(CFE_RESOURCEID_UNWRAP(DumpCtrlId), CFE_TBL_DUMPCTRLID_BASE,
+                                  CFE_PLATFORM_TBL_MAX_SIMULTANEOUS_LOADS, Idx);
+}
+
 /*----------------------------------------------------------------
  *
  * Implemented per public API
@@ -152,6 +164,60 @@ CFE_TBL_Handle_t CFE_TBL_AccessDescriptorGetHandle(const CFE_TBL_AccessDescripto
     return (AccessDescPtr - CFE_TBL_Global.Handles);
 }
 
+/*----------------------------------------------------------------
+ *
+ * Application-scope internal function
+ * See description in header file for argument/return detail
+ *
+ *-----------------------------------------------------------------*/
+CFE_TBL_DumpControl_t *CFE_TBL_LocateDumpCtrlByID(CFE_TBL_DumpCtrlId_t BlockId)
+{
+    CFE_TBL_DumpControl_t *BlockPtr;
+    uint32                 Idx;
+
+    if (CFE_TBL_DumpCtrlId_ToIndex(BlockId, &Idx) == CFE_SUCCESS)
+    {
+        BlockPtr = &CFE_TBL_Global.DumpControlBlocks[Idx];
+    }
+    else
+    {
+        BlockPtr = NULL;
+    }
+
+    return BlockPtr;
+}
+
+/*----------------------------------------------------------------
+ *
+ * Application-scope internal function
+ * See description in header file for argument/return detail
+ *
+ *-----------------------------------------------------------------*/
+bool CFE_TBL_CheckDumpCtrlSlotUsed(CFE_ResourceId_t CheckId)
+{
+    CFE_TBL_DumpControl_t *BlockPtr;
+
+    /*
+     * Note - The pointer here should never be NULL because the ID should always be
+     * within the expected range, but if it ever is NULL, this should return true
+     * such that the caller will _not_ attempt to use the record.
+     */
+    BlockPtr = CFE_TBL_LocateDumpCtrlByID(CFE_TBL_DUMPCTRLID_C(CheckId));
+    return (BlockPtr == NULL || CFE_TBL_DumpCtrlBlockIsUsed(BlockPtr));
+}
+
+/*----------------------------------------------------------------
+ *
+ * Application-scope internal function
+ * See description in header file for argument/return detail
+ *
+ *-----------------------------------------------------------------*/
+CFE_ResourceId_t CFE_TBL_GetNextDumpCtrlBlock(void)
+{
+    return CFE_ResourceId_FindNext(CFE_TBL_Global.LastDumpCtrlBlockId, CFE_PLATFORM_TBL_MAX_SIMULTANEOUS_LOADS,
+                                   CFE_TBL_CheckDumpCtrlSlotUsed);
+}
+
 /*----------------------------------------------------------------
  *
  * Application-scope internal function