From c401ac3a98563f84e1957445f4c5643186e0e9d3 Mon Sep 17 00:00:00 2001 From: Nathan Scott Date: Tue, 17 Aug 2021 14:38:19 +1000 Subject: [PATCH 1/3] Ensure DynamicColumn hash lookups never see NULL pointers This cannot happen in these code locations, but for the purposes of static checkers like Coverity scan (and for future proofing), add two more guards on NULL hash table entry pointers. --- Action.c | 5 +++-- pcp/PCPDynamicColumn.c | 3 +++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/Action.c b/Action.c index 5d774587..6e804f68 100644 --- a/Action.c +++ b/Action.c @@ -174,8 +174,9 @@ static Htop_Reaction actionSetSortColumn(State* st) { char* name = NULL; if (fields[i] >= LAST_PROCESSFIELD) { DynamicColumn* column = Hashtable_get(dynamicColumns, fields[i]); - if (column) - name = xStrdup(column->caption ? column->caption : column->name); + if (column == NULL) + continue; + name = xStrdup(column->caption ? column->caption : column->name); } else { name = String_trim(Process_fields[fields[i]].name); } diff --git a/pcp/PCPDynamicColumn.c b/pcp/PCPDynamicColumn.c index 141978a7..4051e101 100644 --- a/pcp/PCPDynamicColumn.c +++ b/pcp/PCPDynamicColumn.c @@ -287,6 +287,9 @@ void PCPDynamicColumn_writeField(PCPDynamicColumn* this, const Process* proc, Ri int PCPDynamicColumn_compareByKey(const PCPProcess* p1, const PCPProcess* p2, ProcessField key) { const PCPDynamicColumn* column = Hashtable_get(p1->super.processList->dynamicColumns, key); + if (column == NULL) + return -1; + size_t metric = column->id; unsigned int type = PCPMetric_type(metric); From c7f634ec218da6bdb3bab9e9cd6fa7b2df3fcea8 Mon Sep 17 00:00:00 2001 From: Nathan Scott Date: Tue, 17 Aug 2021 14:41:55 +1000 Subject: [PATCH 2/3] PCP: ensure unsigned types used throughout CPU count detection This cannot be negative in these code locations, but for the purposes of static checking like Coverity scan make it clear and used the same unsigned type as ProcessList.h for the CPU count variable (matching PL activeCPUs and existingCPUs). --- pcp/PCPProcessList.c | 2 +- pcp/Platform.c | 8 ++++---- pcp/Platform.h | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pcp/PCPProcessList.c b/pcp/PCPProcessList.c index e4f9a5fd..3001d5e8 100644 --- a/pcp/PCPProcessList.c +++ b/pcp/PCPProcessList.c @@ -35,7 +35,7 @@ static void PCPProcessList_updateCPUcount(PCPProcessList* this) { unsigned int cpus = Platform_getMaxCPU(); if (cpus == pl->existingCPUs) return; - if (cpus <= 0) + if (cpus == 0) cpus = pl->activeCPUs; if (cpus <= 1) cpus = pl->activeCPUs = 1; diff --git a/pcp/Platform.c b/pcp/Platform.c index 97788569..d5a76b55 100644 --- a/pcp/Platform.c +++ b/pcp/Platform.c @@ -396,15 +396,15 @@ void Platform_getLoadAverage(double* one, double* five, double* fifteen) { } } -int Platform_getMaxCPU(void) { +unsigned int Platform_getMaxCPU(void) { if (pcp->ncpu) return pcp->ncpu; pmAtomValue value; - if (PCPMetric_values(PCP_HINV_NCPU, &value, 1, PM_TYPE_32) != NULL) - pcp->ncpu = value.l; + if (PCPMetric_values(PCP_HINV_NCPU, &value, 1, PM_TYPE_U32) != NULL) + pcp->ncpu = value.ul; else - pcp->ncpu = -1; + pcp->ncpu = 1; return pcp->ncpu; } diff --git a/pcp/Platform.h b/pcp/Platform.h index 9d0c8f53..dcb8dc98 100644 --- a/pcp/Platform.h +++ b/pcp/Platform.h @@ -54,7 +54,7 @@ typedef struct Platform_ { long long btime; /* boottime in seconds since the epoch */ char* release; /* uname and distro from this context */ int pidmax; /* maximum platform process identifier */ - int ncpu; /* maximum processor count configured */ + unsigned int ncpu; /* maximum processor count configured */ } Platform; extern ProcessField Platform_defaultFields[]; @@ -79,7 +79,7 @@ void Platform_getLoadAverage(double* one, double* five, double* fifteen); long long Platform_getBootTime(void); -int Platform_getMaxCPU(void); +unsigned int Platform_getMaxCPU(void); int Platform_getMaxPid(void); From d5ff5c48a889c1eab6e3792d3f5ca1566cf05491 Mon Sep 17 00:00:00 2001 From: Nathan Scott Date: Tue, 17 Aug 2021 15:42:10 +1000 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: BenBE --- Action.c | 2 +- pcp/PCPDynamicColumn.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Action.c b/Action.c index 6e804f68..a63b30e4 100644 --- a/Action.c +++ b/Action.c @@ -174,7 +174,7 @@ static Htop_Reaction actionSetSortColumn(State* st) { char* name = NULL; if (fields[i] >= LAST_PROCESSFIELD) { DynamicColumn* column = Hashtable_get(dynamicColumns, fields[i]); - if (column == NULL) + if (!column) continue; name = xStrdup(column->caption ? column->caption : column->name); } else { diff --git a/pcp/PCPDynamicColumn.c b/pcp/PCPDynamicColumn.c index 4051e101..3c4e6797 100644 --- a/pcp/PCPDynamicColumn.c +++ b/pcp/PCPDynamicColumn.c @@ -287,7 +287,7 @@ void PCPDynamicColumn_writeField(PCPDynamicColumn* this, const Process* proc, Ri int PCPDynamicColumn_compareByKey(const PCPProcess* p1, const PCPProcess* p2, ProcessField key) { const PCPDynamicColumn* column = Hashtable_get(p1->super.processList->dynamicColumns, key); - if (column == NULL) + if (!column) return -1; size_t metric = column->id;