Skip to content

Commit

Permalink
Fix nasa#432, buffer error in the VxWorks sysmon module
Browse files Browse the repository at this point in the history 
The cpu number to poll was not range checked until _after_ the memset.
This is not the correct order of operations, it must range the element
number in the array before writing/clearing it.
  • Loading branch information
@jphickey
jphickey committed Sep 12, 2024
1 parent f592144 commit 8b005cc
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 58 deletions.
102 changes: 53 additions & 49 deletions fsw/modules/vxworks_sysmon/vxworks_sysmon.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,19 @@ void vxworks_sysmon_Init(uint32_t local_module_id)

int vxworks_sysmon_update_stat(const char *fmt, ...)
{
vxworks_sysmon_cpuload_state_t *state = &vxworks_sysmon_global.cpu_load;
vxworks_sysmon_cpuload_core_t *core_p = &state->per_core[state->num_cpus];

vxworks_sysmon_cpuload_state_t *state;
vxworks_sysmon_cpuload_core_t *core_p;
int curr_load;
va_list arg;

state = &vxworks_sysmon_global.cpu_load;

if (state->poll_core_no >= VXWORKS_SYSMON_MAX_CPUS)
{
state->poll_core_no = 0;
}

core_p = &state->per_core[state->poll_core_no];
memset(&core_p->idle_state, 0, sizeof(vxworks_sysmon_va_arg_t));

va_start(arg, fmt);
Expand All @@ -71,58 +79,55 @@ int vxworks_sysmon_update_stat(const char *fmt, ...)
/* only want IDLE string */
if(strncmp("IDLE", core_p->idle_state.name, 4) == 0)
{
if(state->num_cpus < VXWORKS_SYSMON_MAX_CPUS)
/*
** Example format:
** (* printRtn) (spyFmt2, "IDLE", "", "", "", totalPerCent, spyIdleTicks,
** incPerCent, tmpIdleIncTicks);
**
** NAME ENTRY TID PRI total % (ticks) delta % (ticks)
** -------- -------- ----- --- --------------- ---------------
** IDLE 95% ( 7990) 95% ( 1998)
**
*/
core_p->idle_state.placeholder = va_arg(arg, char *);
core_p->idle_state.placeholder = va_arg(arg, char *);
core_p->idle_state.placeholder = va_arg(arg, char *);

core_p->idle_state.total_idle_percent = va_arg(arg, int);
core_p->idle_state.total_idle_ticks = va_arg(arg, int);
core_p->idle_state.idle_percent_since_last_report = va_arg(arg, int);
core_p->idle_state.idle_ticks_since_last_report = va_arg(arg, int);

curr_load = VXWORKS_SYSMON_MAX_SCALE - core_p->idle_state.idle_percent_since_last_report;

if (curr_load >= 100)
{
/*
** Example format:
** (* printRtn) (spyFmt2, "IDLE", "", "", "", totalPerCent, spyIdleTicks,
** incPerCent, tmpIdleIncTicks);
**
** NAME ENTRY TID PRI total % (ticks) delta % (ticks)
** -------- -------- ----- --- --------------- ---------------
** IDLE 95% ( 7990) 95% ( 1998)
**
*/
core_p->idle_state.placeholder = va_arg(arg, char *);
core_p->idle_state.placeholder = va_arg(arg, char *);
core_p->idle_state.placeholder = va_arg(arg, char *);

core_p->idle_state.total_idle_percent = va_arg(arg, int);
core_p->idle_state.total_idle_ticks = va_arg(arg, int);
core_p->idle_state.idle_percent_since_last_report = va_arg(arg, int);
core_p->idle_state.idle_ticks_since_last_report = va_arg(arg, int);

curr_load = VXWORKS_SYSMON_MAX_SCALE - core_p->idle_state.idle_percent_since_last_report;

if (curr_load >= 100)
{
core_p->avg_load = 0xFFFFFF; /* max */
}
else if (curr_load <= 0)
{
core_p->avg_load = 0;
}
else
{
core_p->avg_load = (0x1000 * curr_load) / VXWORKS_SYSMON_MAX_SCALE ;
core_p->avg_load |= (core_p->avg_load << 12); /* Expand from 12->24 bit */
}

VXWORKS_SYSMON_DEBUG("CFE_PSP(vxworks_sysmon): load=%06x\n", (unsigned int)core_p->avg_load);
VXWORKS_SYSMON_DEBUG("Name: %s, Total Percent: %d, Total Ticks: %d, Idle Percent: %d, Idle Ticks: %d\n",
core_p->idle_state.name,
core_p->idle_state.total_idle_percent,
core_p->idle_state.total_idle_ticks,
core_p->idle_state.idle_percent_since_last_report,
core_p->idle_state.idle_ticks_since_last_report);
core_p->avg_load = 0xFFFFFF; /* max */
}
else if (curr_load <= 0)
{
core_p->avg_load = 0;
}
else
{
core_p->avg_load = (0x1000 * curr_load) / VXWORKS_SYSMON_MAX_SCALE ;
core_p->avg_load |= (core_p->avg_load << 12); /* Expand from 12->24 bit */
}

state->num_cpus++;
VXWORKS_SYSMON_DEBUG("CFE_PSP(vxworks_sysmon): load=%06x\n", (unsigned int)core_p->avg_load);
VXWORKS_SYSMON_DEBUG("Name: %s, Total Percent: %d, Total Ticks: %d, Idle Percent: %d, Idle Ticks: %d\n",
core_p->idle_state.name,
core_p->idle_state.total_idle_percent,
core_p->idle_state.total_idle_ticks,
core_p->idle_state.idle_percent_since_last_report,
core_p->idle_state.idle_ticks_since_last_report);

} /* end of cpu check */
} /* end of idle check */

va_end(arg);

++state->poll_core_no;

return 0;
}

Expand Down Expand Up @@ -151,7 +156,6 @@ void vxworks_sysmon_Task(void)
{
OS_TaskDelay(VXWORKS_SYSMON_SAMPLE_DELAY);

state->num_cpus = 0;
spyReportCommon( (FUNCPTR)vxworks_sysmon_update_stat);
}

Expand Down
2 changes: 1 addition & 1 deletion fsw/modules/vxworks_sysmon/vxworks_sysmon.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ typedef struct vxworks_sysmon_cpuload_state

osal_id_t task_id;

uint8_t num_cpus;
uint8_t poll_core_no;

vxworks_sysmon_cpuload_core_t per_core[VXWORKS_SYSMON_MAX_CPUS];

Expand Down
13 changes: 5 additions & 8 deletions unit-test-coverage/modules/vxworks_sysmon/src/coveragetest-vxworks_sysmon.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,17 +297,16 @@ void Test_UpdateStat_Nominal(void)
UtAssert_True(vxworks_sysmon_global.cpu_load.per_core[0].avg_load == AvgLoad, "Nominal Case: 100 percents cpuload");

/* Nominal Case: Max Cpu Num */
IdleTaskLoad = 0;
vxworks_sysmon_global.cpu_load.num_cpus = 1;
IdleTaskLoad = 0;
vxworks_sysmon_global.cpu_load.poll_core_no = 1 + VXWORKS_SYSMON_MAX_CPUS;
vxworks_sysmon_update_stat(fmt, "IDLE", "", "", "", 95, 7990, IdleTaskLoad, 1998); /* Function under test */
UtAssert_True(vxworks_sysmon_global.cpu_load.num_cpus == 1, "Nominal Case: Max Cpu Nums");
UtAssert_UINT8_EQ(vxworks_sysmon_global.cpu_load.poll_core_no, 1);

/* Nominal Case: Not Idle String */
memset(&vxworks_sysmon_global, 0, sizeof(vxworks_sysmon_global));
vxworks_sysmon_update_stat(fmt, "KERNEL", "", "", "", 95, 7990, 95, 1998); /* Function under test */
UtAssert_True(vxworks_sysmon_global.cpu_load.per_core[0].avg_load == 0 && vxworks_sysmon_global.cpu_load.num_cpus == 0,
"Nominal Case: Not Idle String");

UtAssert_ZERO(vxworks_sysmon_global.cpu_load.per_core[0].avg_load);
UtAssert_UINT8_EQ(vxworks_sysmon_global.cpu_load.poll_core_no, 1);
}

void Test_Task_Nominal(void)
Expand All @@ -329,7 +328,6 @@ void Test_Task_Error(void)

vxworks_sysmon_Task();
UtAssert_True(vxworks_sysmon_global.cpu_load.should_run == false, "Error Case: Cannot Start Auxillary Clock");

}

/*
Expand All @@ -351,5 +349,4 @@ void UtTest_Setup(void)
ADD_TEST(Test_UpdateStat_Nominal);
ADD_TEST(Test_Task_Nominal);
ADD_TEST(Test_Task_Error);

}

0 comments on commit 8b005cc

Please sign in to comment.