SC_LoadAts_Test_AtsEntryOverflow Unit Test indexes past array bounds

[keegan-moore]

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug

The SC_LoadAts_Test_AtsEntryOverflow unit test indexes past array boundaries.

There are two arrays that get indexed improperly, both in unit-test/sc_loads_tests.c:

1. The first is a file-global variable, uint32 SC_APP_TEST_GlobalAtsCmdStatusTbl[SC_NUMBER_OF_ATS * SC_MAX_ATS_CMDS];
   • this array has 2000 indexes (2 * 1000) by default
2. The second is local to SC_LoadAts_Test_AtsEntryOverflow, named: uint32 AtsTable[SC_ATS_BUFF_SIZE32];
   • this array has 4000 indexes (8000 / 2) by default

In SC_LoadAts_Test_AtsEntryOverflow, local variable j gets increased to a value of 4005. This variable is used to index into both of the above arrays. These accesses are indirect, through these pointers:

• SC_OperData.AtsCmdStatusTblAddr
• SC_OperData.AtsTblAddr

Also, I'm not sure if I'm understanding the test correctly, but it seems like i should be used instead of j, when initializing the AtsCmdStatusTblAddr array. Perhaps it would help to use more verbose variables than i and j.

To Reproduce

1. Add prints to print out the values of j before each
2. Run the unit tests

Expected behavior

All of these out-of-bound accesses seem invalid, and should be able to be removed.

This unit test should probably be revised and cleaned up anyway. For example, after the initial for loop, the test describes that its intending to fill the (one) last entry with an invalid value. However, what the actual test attempts to do is initialize two entries (not one).

Additionally, this unit test doesn't do much verification of SC_LoadAts to confirm that the function worked as expected. I'd expect more than just a single UtAssert_True(call_count_CFE_EVS_SendEvent == 0, call.

Code snips

void SC_LoadAts_Test_AtsEntryOverflow(void)
{
    SC_AtsEntryHeader_t *Entry;
    SC_AtsInfoTable_t    AtsInfoTbl;
    uint32               AtsTable[SC_ATS_BUFF_SIZE32];
    uint8                AtsIndex = 0;
    size_t               MsgSize1;
    size_t               MsgSize2;
    int                  BufEntrySize;
    int                  MaxBufEntries;
    int                  i, j;

    memset(&AtsInfoTbl, 0, sizeof(AtsInfoTbl));
    memset(&AtsTable, 0, sizeof(AtsTable));

    SC_InitTables();

    SC_OperData.AtsCmdStatusTblAddr[AtsIndex] = &SC_APP_TEST_GlobalAtsCmdStatusTbl[0];
    SC_OperData.AtsTblAddr[AtsIndex]          = &AtsTable[0];
    SC_OperData.AtsInfoTblAddr                = &AtsInfoTbl;

    MsgSize1      = SC_PACKET_MAX_SIZE;
    BufEntrySize  = ((MsgSize1 + SC_ROUND_UP_BYTES) / SC_BYTES_IN_WORD) + SC_ATS_HDR_NOPKT_WORDS;
    MaxBufEntries = SC_ATS_BUFF_SIZE32 / BufEntrySize;

    for (i = 0, j = 0; i < MaxBufEntries; i++, j += BufEntrySize)
    {
        Entry                                        = (SC_AtsEntryHeader_t *)&SC_OperData.AtsTblAddr[AtsIndex][j];
        Entry->CmdNumber                             = i + 1;
        SC_OperData.AtsCmdStatusTblAddr[AtsIndex][j] = SC_EMPTY;
        UT_SetDataBuffer(UT_KEY(CFE_MSG_GetSize), &MsgSize1, sizeof(MsgSize1), false);
    }

    /* Next entry should not leave enough buffer space for an ATS command header */
    Entry                                        = (SC_AtsEntryHeader_t *)&SC_OperData.AtsTblAddr[AtsIndex][j];
    Entry->CmdNumber                             = i++ + 1;
    SC_OperData.AtsCmdStatusTblAddr[AtsIndex][j] = SC_EMPTY;

    /* Use the remaining buffer space to calculate the final message size */
    MsgSize2 = (SC_ATS_BUFF_SIZE32 - SC_ATS_HDR_WORDS + 4 - j) * SC_BYTES_IN_WORD;
    UT_SetDataBuffer(UT_KEY(CFE_MSG_GetSize), &MsgSize2, sizeof(MsgSize2), false);

    /* Set up final entry that will create condition */
    j += ((MsgSize2 + SC_ROUND_UP_BYTES) / SC_BYTES_IN_WORD) + SC_ATS_HDR_WORDS;
    Entry            = (SC_AtsEntryHeader_t *)&SC_OperData.AtsTblAddr[AtsIndex][j];
    Entry->CmdNumber = i + 1;
    UT_SetDataBuffer(UT_KEY(CFE_MSG_GetSize), &MsgSize1, sizeof(MsgSize1), false);

    SC_OperData.AtsCmdStatusTblAddr[AtsIndex][j] = SC_EMPTY;

    /* Execute the function being tested */
    SC_LoadAts(AtsIndex);

    /* Verify results */
    call_count_CFE_EVS_SendEvent = UT_GetStubCount(UT_KEY(CFE_EVS_SendEvent));

    UtAssert_True(call_count_CFE_EVS_SendEvent == 0, "CFE_EVS_SendEvent was called %u time(s), expected 0",
                  call_count_CFE_EVS_SendEvent);
}

System observed on:

• x86_64
• OS: GNU/Linux 5.4
• Versions: Draco rc4

Additional context

This issue only seemed to pop up when we built the linux target for a 32-bit linux executable, with -m32.

Reporter Info

Keegan Moore
NASA/GSFC

[skliper]

Just ran into this myself, should be i but a better test design would be to initialize those tables in the startup instead of repeating code all over. The buffer overflow is a consistent issue, it's just it rarely shows up in a location that's checked.