From 896727b6646fc2d4b3ba17dee9c4f42b932a89f1 Mon Sep 17 00:00:00 2001 From: Andrew Wood Date: Sun, 10 Sep 2023 23:04:43 +0100 Subject: [PATCH] Addressed the issues raised by splint and flawfinder. --- src/include/pv-internal.h | 14 ++-- src/include/pv.h | 6 ++ src/main/options.c | 41 +---------- src/pv/state.c | 144 +++++++++++++++++++++++++++++--------- src/pv/string.c | 38 ++++++++++ 5 files changed, 164 insertions(+), 79 deletions(-) diff --git a/src/include/pv-internal.h b/src/include/pv-internal.h index 3a4f049..4231ee5 100644 --- a/src/include/pv-internal.h +++ b/src/include/pv-internal.h @@ -88,7 +88,7 @@ struct pvstate_s { * Input files * ***************/ unsigned int input_file_count; /* number of input files */ - const char **input_files; /* input files (0=first) */ + /*@only@*/ /*@null@*/ char **input_files; /* input files */ /******************* * Program control * @@ -120,14 +120,14 @@ struct pvstate_s { unsigned int height; /* screen height */ bool width_set_manually; /* width was set manually, not detected */ bool height_set_manually; /* height was set manually, not detected */ - /*@null@*/ char *name; /* display name */ + /*@only@*/ /*@null@*/ char *name; /* display name */ char default_format[PV_SIZEOF_DEFAULT_FORMAT]; /* default format string */ - char *format_string; /* output format string */ + /*@only@*/ /*@null@*/ char *format_string; /* output format string */ /****************** * Program status * ******************/ - const char *program_name; /* program name for error reporting */ + /*@only@*/ char *program_name; /* program name for error reporting */ char cwd[PV_SIZEOF_CWD]; /* current working directory for relative path */ int current_input_file; /* index of current file being read */ int exit_status; /* exit status to give (0=OK) */ @@ -160,15 +160,15 @@ struct pvstate_s { long double prev_trans; /* Keep track of progress over last intervals to compute current average rate. */ - pvhistory_t *history; /* state at previous intervals (circular buffer) */ - int history_len; /* total size */ + /*@null@*/ pvhistory_t *history; /* state at previous intervals (circular buffer) */ + unsigned int history_len; /* total size */ int history_interval; /* seconds between each history entry */ int history_first; int history_last; long double current_avg_rate; /* current average rate over last history intervals */ unsigned long long initial_offset; - char *display_buffer; + /*@only@*/ char *display_buffer; long display_buffer_size; int lastoutput_length; /* number of last-output bytes to show */ unsigned char lastoutput_buffer[PV_SIZEOF_LASTOUTPUT_BUFFER]; diff --git a/src/include/pv.h b/src/include/pv.h index 3ca43be..eaabf5c 100644 --- a/src/include/pv.h +++ b/src/include/pv.h @@ -78,6 +78,12 @@ extern int pv_snprintf(char *, size_t, const char *, ...); */ extern size_t pv_strlcat(char *, const char *, size_t); +/* + * Allocate and return a duplicate of a \0-terminated string, ensuring that + * the duplicate is also \0-terminated. Returns NULL on error. + */ +/*@null@ */ /*@only@ */ extern char *pv_strdup(const char *); + /* * Functions relating to elapsed time. */ diff --git a/src/main/options.c b/src/main/options.c index 93d039a..401b0bf 100644 --- a/src/main/options.c +++ b/src/main/options.c @@ -25,41 +25,6 @@ void display_help(void); void display_version(void); -/* - * Allocate a duplicate of a \0-terminated string. - */ -static /*@null@ */ - /*@only@ */ -char *xstrdup(const char *original) -{ - size_t length; - char *duplicate; - - if (NULL == original) { - errno = EINVAL; - return NULL; - } - - length = strlen(original); /* flawfinder: ignore */ - /* - * flawfinder rationale: the original string is explicitly required - * to be \0 terminated. - */ - duplicate = calloc(1, 1 + length); - if (NULL == duplicate) - return NULL; - - memcpy(duplicate, original, length); /* flawfinder: ignore */ - /* - * flawfinder rationale: the buffer is explicitly allocated to be - * large enough. - */ - - duplicate[length] = '\0'; - - return duplicate; -} - /* * Free an opts_t object. */ @@ -444,7 +409,7 @@ opts_t opts_parse(unsigned int argc, char **argv) opts->height_set_manually = opts->height == 0 ? false : true; break; case 'N': - opts->name = xstrdup(optarg); + opts->name = pv_strdup(optarg); if (NULL == opts->name) { fprintf(stderr, "%s: -N: %s\n", opts->program_name, strerror(errno)); opts_free(opts); @@ -484,7 +449,7 @@ opts_t opts_parse(unsigned int argc, char **argv) opts->remote = pv_getnum_ui(optarg); break; case 'P': - opts->pidfile = xstrdup(optarg); + opts->pidfile = pv_strdup(optarg); if (NULL == opts->pidfile) { fprintf(stderr, "%s: -P: %s\n", opts->program_name, strerror(errno)); opts_free(opts); @@ -492,7 +457,7 @@ opts_t opts_parse(unsigned int argc, char **argv) } break; case 'F': - opts->format = xstrdup(optarg); + opts->format = pv_strdup(optarg); if (NULL == opts->format) { fprintf(stderr, "%s: -F: %s\n", opts->program_name, strerror(errno)); opts_free(opts); diff --git a/src/pv/state.c b/src/pv/state.c index 45db642..45d839b 100644 --- a/src/pv/state.c +++ b/src/pv/state.c @@ -24,10 +24,17 @@ static void pv_alloc_history(pvstate_t state) free(state->history); state->history = NULL; - state->history = calloc(state->history_len, sizeof(state->history[0])); + state->history = calloc((size_t) (state->history_len), sizeof(state->history[0])); if (NULL == state->history) { + /*@-mustfreefresh@ */ + /* + * splint note: the gettext calls made by _() cause memory + * leak warnings, but in this case it's unavoidable, and + * mitigated by the fact we only translate each string once. + */ fprintf(stderr, "%s: %s: %s\n", state->program_name, _("history structure allocation failed"), strerror(errno)); + /*@+mustfreefresh@ */ return; } @@ -45,8 +52,19 @@ pvstate_t pv_state_alloc(const char *program_name) state = calloc(1, sizeof(*state)); if (NULL == state) return NULL; + memset(state, 0, sizeof(*state)); + + /* splint 3.1.2 thinks this is required for some reason. */ + if (NULL != state->program_name) { + free(state->program_name); + } + + state->program_name = pv_strdup(program_name); + if (NULL == state->program_name) { + free(state); + return NULL; + } - state->program_name = program_name; state->watch_pid = 0; state->watch_fd = -1; #ifdef HAVE_IPC @@ -88,6 +106,10 @@ void pv_state_free(pvstate_t state) if (0 == state) return; + if (NULL != state->program_name) + free(state->program_name); + state->program_name = NULL; + if (NULL != state->display_buffer) free(state->display_buffer); state->display_buffer = NULL; @@ -110,6 +132,18 @@ void pv_state_free(pvstate_t state) free(state->history); state->history = NULL; + if (NULL != state->input_files) { + unsigned int file_idx; + for (file_idx = 0; file_idx < state->input_file_count; file_idx++) { + /*@-unqualifiedtrans@ */ + free(state->input_files[file_idx]); + /*@+unqualifiedtrans@ */ + /* splint: see similar code below. */ + } + free(state->input_files); + state->input_files = NULL; + } + free(state); return; @@ -124,8 +158,8 @@ void pv_state_set_format(pvstate_t state, bool progress, bool timer, bool eta, b { #define PV_ADDFORMAT(x,y) if (x) { \ if (state->default_format[0] != '\0') \ - pv_strlcat(state->default_format, " ", sizeof(state->default_format)); \ - pv_strlcat(state->default_format, y, sizeof(state->default_format)); \ + (void) pv_strlcat(state->default_format, " ", sizeof(state->default_format)); \ + (void) pv_strlcat(state->default_format, y, sizeof(state->default_format)); \ } state->default_format[0] = '\0'; @@ -139,10 +173,15 @@ void pv_state_set_format(pvstate_t state, bool progress, bool timer, bool eta, b PV_ADDFORMAT(eta, "%e"); PV_ADDFORMAT(fineta, "%I"); if (lastwritten > 0) { - char buf[16]; + char buf[16]; /* flawfinder: ignore */ memset(buf, 0, sizeof(buf)); (void) pv_snprintf(buf, sizeof(buf), "%%%uA", lastwritten); PV_ADDFORMAT(lastwritten > 0, buf); + /* + * flawfinder rationale: large enough for string, zeroed + * before use, only written to by pv_snprintf() with the + * right buffer length. + */ } if (NULL != state->name) { @@ -151,7 +190,7 @@ void pv_state_set_format(pvstate_t state, bool progress, bool timer, bool eta, b } if (NULL != name) - state->name = strdup(name); + state->name = pv_strdup(name); state->reparse_display = 1; } @@ -165,47 +204,47 @@ void pv_state_force_set(pvstate_t state, bool val) void pv_state_cursor_set(pvstate_t state, bool val) { state->cursor = val; -}; +} void pv_state_numeric_set(pvstate_t state, bool val) { state->numeric = val; -}; +} void pv_state_wait_set(pvstate_t state, bool val) { state->wait = val; -}; +} void pv_state_delay_start_set(pvstate_t state, double val) { state->delay_start = val; -}; +} void pv_state_linemode_set(pvstate_t state, bool val) { state->linemode = val; -}; +} void pv_state_bits_set(pvstate_t state, bool bits) { state->bits = bits; -}; +} void pv_state_null_terminated_lines_set(pvstate_t state, bool val) { state->null_terminated_lines = val; -}; +} void pv_state_no_display_set(pvstate_t state, bool val) { state->no_display = val; -}; +} void pv_state_skip_errors_set(pvstate_t state, unsigned int val) { state->skip_errors = val; -}; +} void pv_state_error_skip_block_set(pvstate_t state, unsigned long long val) { @@ -215,60 +254,60 @@ void pv_state_error_skip_block_set(pvstate_t state, unsigned long long val) void pv_state_stop_at_size_set(pvstate_t state, bool val) { state->stop_at_size = val; -}; +} void pv_state_sync_after_write_set(pvstate_t state, bool val) { state->sync_after_write = val; -}; +} void pv_state_direct_io_set(pvstate_t state, bool val) { state->direct_io = val; state->direct_io_changed = true; -}; +} void pv_state_discard_input_set(pvstate_t state, bool val) { state->discard_input = val; -}; +} void pv_state_rate_limit_set(pvstate_t state, unsigned long long val) { state->rate_limit = val; -}; +} void pv_state_target_buffer_size_set(pvstate_t state, unsigned long long val) { state->target_buffer_size = val; -}; +} void pv_state_no_splice_set(pvstate_t state, bool val) { state->no_splice = val; -}; +} void pv_state_size_set(pvstate_t state, unsigned long long val) { state->size = val; -}; +} void pv_state_interval_set(pvstate_t state, double val) { state->interval = val; -}; +} void pv_state_width_set(pvstate_t state, unsigned int val, bool was_set_manually) { state->width = val; state->width_set_manually = was_set_manually; -}; +} void pv_state_height_set(pvstate_t state, unsigned int val, bool was_set_manually) { state->height = val; state->height_set_manually = was_set_manually; -}; +} void pv_state_name_set(pvstate_t state, /*@null@ */ const char *val) { @@ -277,8 +316,8 @@ void pv_state_name_set(pvstate_t state, /*@null@ */ const char *val) state->name = NULL; } if (NULL != val) - state->name = strdup(val); -}; + state->name = pv_strdup(val); +} void pv_state_format_string_set(pvstate_t state, /*@null@ */ const char *val) { @@ -287,18 +326,18 @@ void pv_state_format_string_set(pvstate_t state, /*@null@ */ const char *val) state->format_string = NULL; } if (NULL != val) - state->format_string = strdup(val); -}; + state->format_string = pv_strdup(val); +} void pv_state_watch_pid_set(pvstate_t state, unsigned int val) { state->watch_pid = val; -}; +} void pv_state_watch_fd_set(pvstate_t state, int val) { state->watch_fd = val; -}; +} void pv_state_average_rate_window_set(pvstate_t state, unsigned int val) { @@ -312,7 +351,7 @@ void pv_state_average_rate_window_set(pvstate_t state, unsigned int val) state->history_interval = 1; } pv_alloc_history(state); -}; +} /* @@ -320,8 +359,45 @@ void pv_state_average_rate_window_set(pvstate_t state, unsigned int val) */ void pv_state_inputfiles(pvstate_t state, unsigned int input_file_count, const char **input_files) { + unsigned int file_idx; + + if (NULL != state->input_files) { + for (file_idx = 0; file_idx < state->input_file_count; file_idx++) { + /*@-unqualifiedtrans@ */ + free(state->input_files[file_idx]); + /*@+unqualifiedtrans@ */ + /* + * TODO: find a way to tell splint the array + * contents are "only" and "null" as well as the + * array itself. + */ + } + free(state->input_files); + state->input_files = NULL; + state->input_file_count = 0; + } + state->input_files = calloc((size_t) (input_file_count + 1), sizeof(char *)); + if (NULL == state->input_files) { + /*@-mustfreefresh@ *//* see similar _() issue above */ + fprintf(stderr, "%s: %s: %s\n", state->program_name, _("file list allocation failed"), strerror(errno)); + /*@+mustfreefresh@ */ + return; + } + for (file_idx = 0; file_idx < input_file_count; file_idx++) { + /*@-nullstate@ */ + state->input_files[file_idx] = pv_strdup(input_files[file_idx]); + if (NULL == state->input_files[file_idx]) { + /*@-mustfreefresh@ *//* see similar _() issue above */ + fprintf(stderr, "%s: %s: %s\n", state->program_name, + _("file list allocation failed"), strerror(errno)); + /*@+mustfreefresh@ */ + return; + } + } state->input_file_count = input_file_count; - state->input_files = input_files; } +/*@+nullstate@*/ +/* splint: see unqualifiedtrans note by free() above. */ + /* EOF */ diff --git a/src/pv/string.c b/src/pv/string.c index ecaa0bc..03eec76 100644 --- a/src/pv/string.c +++ b/src/pv/string.c @@ -12,6 +12,7 @@ #include #include #include +#include /* @@ -113,4 +114,41 @@ size_t pv_strlcat(char *dst, const char *src, size_t dstsize) #endif } + +/* + * Allocate and return a duplicate of a \0-terminated string, ensuring that + * the duplicate is also \0-terminated. Returns NULL on error. + */ +/*@null@ */ +/*@only@ */ +char *pv_strdup(const char *original) +{ + size_t length; + char *duplicate; + + if (NULL == original) { + errno = EINVAL; + return NULL; + } + + length = strlen(original); /* flawfinder: ignore */ + /* + * flawfinder rationale: the original string is explicitly required + * to be \0 terminated. + */ + duplicate = calloc(1, 1 + length); + if (NULL == duplicate) + return NULL; + + memcpy(duplicate, original, length); /* flawfinder: ignore */ + /* + * flawfinder rationale: the buffer is explicitly allocated to be + * large enough. + */ + + duplicate[length] = '\0'; + + return duplicate; +} + /* EOF */