sc: do not publish uninitialised TblPtrNew when CFE_TBL_GetAddress fails

[nurdymuny]

Summary

SC_ManageTable re-acquires a table address after CFE_TBL_Manage but writes the returned pointer into the shared SC_OperData.*TblAddr slot unconditionally, before checking the return code:

    void            *TblPtrNew;        // ← uninitialised local
    ...
    Result   = CFE_TBL_GetAddress(&TblPtrNew, TblHandle);
    *TblAddr = TblPtrNew; /* Note that CFE_TBL_GetAddress() sets this to NULL if it fails */

The inline comment is wrong. Per cfe_tbl.h, CFE_TBL_GetAddress leaves the output pointer undefined on any non-success return — only CFE_SUCCESS and CFE_TBL_INFO_UPDATED set it. The five failure codes (CFE_TBL_ERR_NEVER_LOADED, _INVALID_HANDLE, _NO_ACCESS, CFE_ES_ERR_RESOURCEID_NOT_VALID, CFE_TBL_BAD_ARGUMENT) all skip the write.

TblPtrNew is declared at sc_cmds.c:723 without an initialiser, so on any failure path the *TblAddr = TblPtrNew; write publishes whatever was on the stack at that slot into SC_OperData.AtsTblAddr[i] (or RTS / Append). Downstream code in sc_loads.c, sc_atsrq.c, and sc_cmds.c reads those slots via SC_GetAtsEntryAtOffset(...) and dereferences a wild pointer.

Change

1. Initialise TblPtrNew = NULL before the call so a failing CFE_TBL_GetAddress cannot leave stack garbage in it.
2. Only publish TblPtrNew to *TblAddr on CFE_SUCCESS or CFE_TBL_INFO_UPDATED; otherwise write NULL.
3. Replace the misleading inline comment with the actual cfe_tbl.h contract.

- /* Re-acquire table data pointer */
- Result   = CFE_TBL_GetAddress(&TblPtrNew, TblHandle);
- *TblAddr = TblPtrNew; /* Note that CFE_TBL_GetAddress() sets this to NULL if it fails */
+ /* documented cfe_tbl.h contract -- see comment in patch */
+ TblPtrNew = NULL;
+ Result    = CFE_TBL_GetAddress(&TblPtrNew, TblHandle);
+ if (Result == CFE_SUCCESS || Result == CFE_TBL_INFO_UPDATED)
+ {
+     *TblAddr = TblPtrNew;
+ }
+ else
+ {
+     *TblAddr = NULL;
+ }

Provenance

Surfaced by scj-hunt + Gigi ATTEND on the cfs_apps_sc_v01 fiber bundle. SC_ManageTable ranked in the top cluster at risk_score 8.8; the underlying mistake about the CFE_TBL_GetAddress contract was the same root cause as the FM finding in nasa/FM#143 — both files (and others) over-narrow the failure-code check. Same defense-in-depth shape as nasa/MM#116 (DataSize switch with no default case).

scj-hunt is open source at nurdymuny/shadow-clone-jutsu.

Test plan

• Build under existing CI (no new symbols, no new headers).
• Mock-fault CFE_TBL_GetAddress with CFE_TBL_ERR_INVALID_HANDLE, observe that the shared SC_OperData.AtsTblAddr[i] slot is set to NULL (cleanly null-checkable) instead of leaving stack garbage there for SC_GetAtsEntryAtOffset to dereference.

🤖 Generated with Claude Code

[nurdymuny]

FYI to reviewers: while sweeping the cFS apps for the same shape, I found that nasa/CF::CF_CheckTables (cf_app.c:48) has an inline comment block explicitly calling out the multi-success-code issue and uses

if (status < CFE_SUCCESS) { /* error */ }

(negative-only check) as the safest pattern, on the basis that cFE convention is "alt-success codes are positive, errors are negative".

If you'd prefer that idiom over the explicit allow-list in this PR (Status != CFE_SUCCESS && Status != CFE_TBL_INFO_UPDATED), I'm happy to switch. The CF approach is shorter and more future-proof against new info codes, the explicit allow-list is more conservative about what it accepts.

Also worth noting: I dug into cfe_tbl_api.c::CFE_TBL_GetAddress and the current implementation does *TblPtr = NULL; at the top of the function before doing the work, so in practice the published pointer ends up NULL on failure rather than stack garbage. The PR's defense-in-depth justification still stands relative to the documented cfe_tbl.h contract ("undefined on failure"), but the practical impact today is NULL-deref-downstream rather than wild-pointer-write.