From decf311a8054116780eaa6799e680be43fae94cd Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Sun, 14 Sep 2025 00:52:11 +0800 Subject: [PATCH] Improve C standards compliance and code quality This commit does the following: - Add void parameter lists for parameter-less functions - Fix pointer arithmetic - Initialize variables and buffers to prevent undefined behavior - Replace unsafe string operations with safe alternatives - Add early returns after error conditions - Make implicit comparisons explicit - Fix null pointer handling in memory allocation Change-Id: I2bf51bd2dd954bd9bcef2df6073e8a5509facea9 --- console.c | 27 ++++++++++++++------------- console.h | 4 ++-- harness.c | 22 ++++++++++------------ harness.h | 6 +++--- qtest.c | 19 ++++++++++--------- report.c | 21 ++++++++++++++------- scripts/pre-commit.hook | 3 +-- 7 files changed, 54 insertions(+), 48 deletions(-) diff --git a/console.c b/console.c index 701ea7bca..3f2011f57 100644 --- a/console.c +++ b/console.c @@ -65,10 +65,10 @@ static bool has_infile = false; static cmd_func_t quit_helpers[MAXQUIT]; static int quit_helper_cnt = 0; -static void init_in(); +static void init_in(void); static bool push_file(char *fname); -static void pop_file(); +static void pop_file(void); static bool interpret_cmda(int argc, char *argv[]); @@ -189,7 +189,7 @@ static bool force_quit(int argc, char *argv[]) return ok; } -static void record_error() +static void record_error(void) { err_cnt++; if (err_cnt >= err_limit) { @@ -403,7 +403,7 @@ static bool do_time(int argc, char *argv[]) } static bool use_linenoise = true; -static int web_fd; +static int web_fd = -1; static bool do_web(int argc, char *argv[]) { @@ -426,7 +426,7 @@ static bool do_web(int argc, char *argv[]) } /* Initialize interpreter */ -void init_cmd() +void init_cmd(void) { cmd_list = NULL; param_list = NULL; @@ -479,7 +479,7 @@ static bool push_file(char *fname) } /* Pop a file buffer from stack */ -static void pop_file() +static void pop_file(void) { if (buf_stack) { rio_t *rsave = buf_stack; @@ -490,7 +490,7 @@ static void pop_file() } /* Handling of input */ -static void init_in() +static void init_in(void) { buf_stack = NULL; } @@ -498,7 +498,7 @@ static void init_in() /* Read command from input file. * When hit EOF, close that file and return NULL */ -static char *readline() +static char *readline(void) { char c; char *lptr = linebuf; @@ -551,7 +551,7 @@ static char *readline() return linebuf; } -static bool cmd_done() +static bool cmd_done(void) { return !buf_stack || quit_flag; } @@ -607,7 +607,7 @@ static int cmd_select(int nfds, return 0; } -bool finish_cmd() +bool finish_cmd(void) { bool ok = true; if (!quit_flag) @@ -618,7 +618,8 @@ bool finish_cmd() static bool cmd_maybe(const char *target, const char *src) { - for (int i = 0; i < strlen(src); i++) { + size_t src_len = strlen(src); + for (size_t i = 0; i < src_len; i++) { if (target[i] == '\0') return false; if (src[i] != target[i]) @@ -638,8 +639,8 @@ void completion(const char *buf, line_completions_t *lc) if (strlen(plist->name) > 120) continue; - strcat(str, "option "); - strcat(str, plist->name); + strncat(str, "option ", sizeof(str) - 1); + strncat(str, plist->name, sizeof(str) - strlen(str) - 1); if (cmd_maybe(str, buf)) line_add_completion(lc, str); diff --git a/console.h b/console.h index 4bcb74de1..d91789af9 100644 --- a/console.h +++ b/console.h @@ -41,7 +41,7 @@ typedef struct __param_element { } param_element_t; /* Initialize interpreter */ -void init_cmd(); +void init_cmd(void); /* Add a new command */ void add_cmd(char *name, cmd_func_t operation, char *summary, char *parameter); @@ -62,7 +62,7 @@ void set_echo(bool on); /* Complete command interpretation */ /* Return true if no errors occurred */ -bool finish_cmd(); +bool finish_cmd(void); /* Run command loop. Non-null infile_name implies read commands from that file */ diff --git a/harness.c b/harness.c index d92345794..71386642a 100644 --- a/harness.c +++ b/harness.c @@ -69,7 +69,7 @@ typedef enum { /* Internal functions */ /* Should this allocation fail? */ -static bool fail_allocation() +static bool fail_allocation(void) { double weight = (double) random() / RAND_MAX; return (weight < 0.01 * fail_probability); @@ -83,10 +83,11 @@ static block_element_t *find_header(void *p) if (!p) { report_event(MSG_ERROR, "Attempting to free null block"); error_occurred = true; + return NULL; } block_element_t *b = - (block_element_t *) ((size_t) p - sizeof(block_element_t)); + (block_element_t *) ((uintptr_t) p - sizeof(block_element_t)); if (cautious_mode) { /* Make sure this is really an allocated block */ block_element_t *ab = allocated; @@ -119,7 +120,7 @@ static size_t *find_footer(block_element_t *b) { // cppcheck-suppress nullPointerRedundantCheck size_t *p = - (size_t *) ((size_t) b + b->payload_size + sizeof(block_element_t)); + (size_t *) ((uintptr_t) b + b->payload_size + sizeof(block_element_t)); return p; } @@ -150,18 +151,15 @@ static void *alloc(alloc_t alloc_type, size_t size) if (!new_block) { report_event(MSG_FATAL, "Couldn't allocate any more memory"); error_occurred = true; + return NULL; } - // cppcheck-suppress nullPointerRedundantCheck new_block->magic_header = MAGICHEADER; - // cppcheck-suppress nullPointerRedundantCheck new_block->payload_size = size; *find_footer(new_block) = MAGICFOOTER; void *p = (void *) &new_block->payload; - memset(p, !alloc_type * FILLCHAR, size); - // cppcheck-suppress nullPointerRedundantCheck + memset(p, (alloc_type == TEST_CALLOC) ? 0 : FILLCHAR, size); new_block->next = allocated; - // cppcheck-suppress nullPointerRedundantCheck new_block->prev = NULL; if (allocated) @@ -260,7 +258,7 @@ char *test_strdup(const char *s) return memcpy(new, s, len); } -size_t allocation_check() +size_t allocation_check(void) { return allocated_count; } @@ -283,8 +281,8 @@ void set_noallocate_mode(bool noallocate) noallocate_mode = noallocate; } -/* Return whether any errors have occurred since last time set error limit */ -bool error_check() +/* Return whether any errors have occurred since last time checked */ +bool error_check(void) { bool e = error_occurred; error_occurred = false; @@ -320,7 +318,7 @@ bool exception_setup(bool limit_time) } /* Call once past risky code */ -void exception_cancel() +void exception_cancel(void) { if (time_limited) { alarm(0); diff --git a/harness.h b/harness.h index 69b67d445..afd7d186f 100644 --- a/harness.h +++ b/harness.h @@ -19,7 +19,7 @@ char *test_strdup(const char *s); #ifdef INTERNAL /* Report number of allocated blocks */ -size_t allocation_check(); +size_t allocation_check(void); /* Probability of malloc failing, expressed as percent */ extern int fail_probability; @@ -37,7 +37,7 @@ void set_cautious_mode(bool cautious); void set_noallocate_mode(bool noallocate); /* Return whether any errors have occurred since last time checked */ -bool error_check(); +bool error_check(void); /* Prepare for a risky operation using setjmp. * Function returns true for initial return, false for error return @@ -45,7 +45,7 @@ bool error_check(); bool exception_setup(bool limit_time); /* Call once past risky code */ -void exception_cancel(); +void exception_cancel(void); /* Use longjmp to return to most recent exception setup. Include error message */ diff --git a/qtest.c b/qtest.c index b622d0816..bf9469dfc 100644 --- a/qtest.c +++ b/qtest.c @@ -913,7 +913,7 @@ static bool do_merge(int argc, char *argv[]) return ok && !error_check(); } -static bool is_circular() +static bool is_circular(void) { struct list_head *cur = current->q->next; struct list_head *fast = (cur) ? cur->next : NULL; @@ -1056,7 +1056,7 @@ static bool do_next(int argc, char *argv[]) return q_show(0); } -static void console_init() +static void console_init(void) { ADD_COMMAND(new, "Create new queue", ""); ADD_COMMAND(free, "Delete queue", ""); @@ -1125,7 +1125,7 @@ static void sigalrm_handler(int sig) "code is too inefficient"); } -static void q_init() +static void q_init(void) { fail_count = 0; INIT_LIST_HEAD(&chain.head); @@ -1259,7 +1259,7 @@ bool commit_exists(const char *commit_hash) char buffer[1024]; while (fgets(buffer, sizeof(buffer), stream)) { /* Compare the first 40 characters of each line with commit_hash */ - if (!strncmp(buffer, commit_hash, 40)) { + if (strncmp(buffer, commit_hash, 40) == 0) { found = true; break; } @@ -1294,19 +1294,20 @@ static bool check_commitlog(void) } #define GIT_HOOK ".git/hooks/" -static bool sanity_check() +static bool sanity_check(void) { struct stat buf; /* Directory .git not found */ - if (stat(".git", &buf)) { + if (stat(".git", &buf) != 0) { fprintf(stderr, "FATAL: You should run qtest in the directory containing valid " "git workspace.\n"); return false; } /* Expected pre-commit and pre-push hooks not found */ - if (stat(GIT_HOOK "commit-msg", &buf) || - stat(GIT_HOOK "pre-commit", &buf) || stat(GIT_HOOK "pre-push", &buf)) { + if (stat(GIT_HOOK "commit-msg", &buf) != 0 || + stat(GIT_HOOK "pre-commit", &buf) != 0 || + stat(GIT_HOOK "pre-push", &buf) != 0) { fprintf(stderr, "FATAL: Git hooks are not properly installed.\n"); /* Attempt to install Git hooks */ @@ -1323,7 +1324,7 @@ static bool sanity_check() return false; } /* GitHub Actions checkouts do not include the complete git history. */ - if (stat("/home/runner/work", &buf)) { + if (stat("/home/runner/work", &buf) != 0) { #define COPYRIGHT_COMMIT_SHA1 "50c5ac53d31adf6baac4f8d3db6b3ce2215fee40" if (!commit_exists(COPYRIGHT_COMMIT_SHA1)) { fprintf( diff --git a/report.c b/report.c index fb964dd40..7c37f254b 100644 --- a/report.c +++ b/report.c @@ -30,7 +30,7 @@ static char fail_buf[1024] = "FATAL Error. Exiting\n"; static volatile int ret = 0; /* Default fatal function */ -static void default_fatal_fun() +static void default_fatal_fun(void) { ret = write(STDOUT_FILENO, fail_buf, strlen(fail_buf) + 1); if (logfile) @@ -38,7 +38,7 @@ static void default_fatal_fun() } /* Optional function to call when fatal error encountered */ -static void (*fatal_fun)() = default_fatal_fun; +static void (*fatal_fun)(void) = default_fatal_fun; void set_verblevel(int level) { @@ -102,7 +102,7 @@ void report(int level, char *fmt, ...) if (!verbfile) init_files(stdout, stdout); - char buffer[BUF_SIZE]; + char buffer[BUF_SIZE] = {0}; if (level <= verblevel) { va_list ap; va_start(ap, fmt); @@ -135,7 +135,7 @@ void report_noreturn(int level, char *fmt, ...) if (!verbfile) init_files(stdout, stdout); - char buffer[BUF_SIZE]; + char buffer[BUF_SIZE] = {0}; if (level <= verblevel) { va_list ap; va_start(ap, fmt); @@ -263,14 +263,17 @@ char *strsave_or_fail(const char *s, const char *fun_name) peak_bytes = MAX(peak_bytes, current_bytes); last_peak_bytes = MAX(last_peak_bytes, current_bytes); + // cppcheck-suppress returnDanglingLifetime return strncpy(ss, s, len + 1); } /* Free block, as from malloc, realloc, or strsave */ void free_block(void *b, size_t bytes) { - if (!b) + if (!b) { report_event(MSG_ERROR, "Attempting to free null block"); + return; + } free(b); free_cnt++; @@ -281,8 +284,10 @@ void free_block(void *b, size_t bytes) /* Free array, as from calloc */ void free_array(void *b, size_t cnt, size_t bytes) { - if (!b) + if (!b) { report_event(MSG_ERROR, "Attempting to free null block"); + return; + } free(b); free_cnt++; @@ -293,8 +298,10 @@ void free_array(void *b, size_t cnt, size_t bytes) /* Free string saved by strsave_or_fail */ void free_string(char *s) { - if (!s) + if (!s) { report_event(MSG_ERROR, "Attempting to free null block"); + return; + } free_block((void *) s, strlen(s) + 1); } diff --git a/scripts/pre-commit.hook b/scripts/pre-commit.hook index 47ae357ab..951943f40 100755 --- a/scripts/pre-commit.hook +++ b/scripts/pre-commit.hook @@ -85,7 +85,7 @@ cppcheck_suppressions() { # Array for additional cppcheck options (non-suppressions) local -a other_flags=( - "--inline-suppr harness.c --inline-suppr tools/fmtscan.c" + "--inline-suppr" ) local out="" @@ -376,7 +376,6 @@ if [ $RETURN -eq 0 ]; then printf "\n${GREEN}All checks passed!${NC}\n" else printf "\n${RED}Pre-commit checks failed. Please fix the issues above.${NC}\n" - printf "${YELLOW}Tip: Use 'git commit --no-verify' to bypass these checks (not recommended).${NC}\n" fi exit $RETURN