From cb61865bb93999e02852438d53c06def3d8d623b Mon Sep 17 00:00:00 2001 From: Nathan Scott Date: Sat, 30 Apr 2022 13:50:25 +1000 Subject: Add array bounds checking for the Process_fields array (covscan) Coverity scan reports there may be a code path that would cause an overrun in the (relatively new) ScreenSettings code where it evaluates default sort direction. Add bounds check and default to descending instead of a potentially invalid array access. --- Settings.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Settings.c b/Settings.c index 006be97f..a825b56a 100644 --- a/Settings.c +++ b/Settings.c @@ -271,12 +271,18 @@ static void ScreenSettings_readFields(ScreenSettings* ss, Hashtable* columns, co ScreenSettings* Settings_newScreen(Settings* this, const ScreenDefaults* defaults) { int sortKey = defaults->sortKey ? toFieldIndex(this->dynamicColumns, defaults->sortKey) : PID; + int sortDesc; + if (sortKey >= 0 && sortKey < LAST_PROCESSFIELD) + sortDesc = Process_fields[sortKey].defaultSortDesc; + else + sortDesc = 1; + ScreenSettings* ss = xMalloc(sizeof(ScreenSettings)); *ss = (ScreenSettings) { .name = xStrdup(defaults->name), .fields = xCalloc(LAST_PROCESSFIELD, sizeof(ProcessField)), .flags = 0, - .direction = (Process_fields[sortKey].defaultSortDesc) ? -1 : 1, + .direction = sortDesc ? -1 : 1, .treeDirection = 1, .sortKey = sortKey, .treeSortKey = PID, -- cgit v1.2.3 From cde72dd0b0bcc971ffa0a343e4a5177eed1b8765 Mon Sep 17 00:00:00 2001 From: Nathan Scott Date: Sat, 30 Apr 2022 13:55:56 +1000 Subject: Remove redundant null checks on Settings_write (covscan) Coverity scan reports that there is dead code in Settings_write checking for nulls that have already been dereferenced on every code path leading to the check. This is likely a hangover from times when the screens pointer was only conditionally allocated - they're not needed anymore. --- Settings.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/Settings.c b/Settings.c index a825b56a..0b8050d8 100644 --- a/Settings.c +++ b/Settings.c @@ -631,19 +631,17 @@ int Settings_write(const Settings* this, bool onCrash) { printSettingInteger("tree_view_always_by_pid", this->screens[0]->treeViewAlwaysByPID); printSettingInteger("all_branches_collapsed", this->screens[0]->allBranchesCollapsed); - if (this->screens && this->screens[0]) { - for (unsigned int i = 0; i < this->nScreens; i++) { - ScreenSettings* ss = this->screens[i]; - fprintf(fd, "screen:%s=", ss->name); - writeFields(fd, ss->fields, this->dynamicColumns, true, separator); - printSettingString(".sort_key", toFieldName(this->dynamicColumns, ss->sortKey)); - printSettingString(".tree_sort_key", toFieldName(this->dynamicColumns, ss->treeSortKey)); - printSettingInteger(".tree_view", ss->treeView); - printSettingInteger(".tree_view_always_by_pid", ss->treeViewAlwaysByPID); - printSettingInteger(".sort_direction", ss->direction); - printSettingInteger(".tree_sort_direction", ss->treeDirection); - printSettingInteger(".all_branches_collapsed", ss->allBranchesCollapsed); - } + for (unsigned int i = 0; i < this->nScreens; i++) { + ScreenSettings* ss = this->screens[i]; + fprintf(fd, "screen:%s=", ss->name); + writeFields(fd, ss->fields, this->dynamicColumns, true, separator); + printSettingString(".sort_key", toFieldName(this->dynamicColumns, ss->sortKey)); + printSettingString(".tree_sort_key", toFieldName(this->dynamicColumns, ss->treeSortKey)); + printSettingInteger(".tree_view", ss->treeView); + printSettingInteger(".tree_view_always_by_pid", ss->treeViewAlwaysByPID); + printSettingInteger(".sort_direction", ss->direction); + printSettingInteger(".tree_sort_direction", ss->treeDirection); + printSettingInteger(".all_branches_collapsed", ss->allBranchesCollapsed); } #undef printSettingString -- cgit v1.2.3 From 10b541b5e4ccd59dc4a6301b58d97a37a90b4228 Mon Sep 17 00:00:00 2001 From: Nathan Scott Date: Sun, 1 May 2022 16:21:13 +1000 Subject: Update Settings_newScreen with single-line sortKey checking. Co-authored-by: BenBE --- Settings.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Settings.c b/Settings.c index 0b8050d8..a6303741 100644 --- a/Settings.c +++ b/Settings.c @@ -271,11 +271,7 @@ static void ScreenSettings_readFields(ScreenSettings* ss, Hashtable* columns, co ScreenSettings* Settings_newScreen(Settings* this, const ScreenDefaults* defaults) { int sortKey = defaults->sortKey ? toFieldIndex(this->dynamicColumns, defaults->sortKey) : PID; - int sortDesc; - if (sortKey >= 0 && sortKey < LAST_PROCESSFIELD) - sortDesc = Process_fields[sortKey].defaultSortDesc; - else - sortDesc = 1; + int sortDesc = (sortKey >= 0 && sortKey < LAST_PROCESSFIELD) ? Process_fields[sortKey].defaultSortDesc : 1; ScreenSettings* ss = xMalloc(sizeof(ScreenSettings)); *ss = (ScreenSettings) { -- cgit v1.2.3