From 7f25aff91d3c2c111ccc2607c23ccb6cc93d14ee Mon Sep 17 00:00:00 2001 From: Andrew Wood Date: Mon, 14 Aug 2023 22:33:06 +0100 Subject: [PATCH] Adjustments and annotations as per splint and flawfinder reports. --- src/include/options.h | 7 ++--- src/include/pv-internal.h | 4 +-- src/include/pv.h | 10 +++---- src/main/main.c | 36 ++++++++++++++++++++----- src/main/options.c | 56 +++++++++++++++++++++++++++++++++------ src/pv/state.c | 10 +++---- 6 files changed, 92 insertions(+), 31 deletions(-) diff --git a/src/include/options.h b/src/include/options.h index ba52677..4555b88 100644 --- a/src/include/options.h +++ b/src/include/options.h @@ -55,16 +55,17 @@ struct opts_s { /* structure describing run-time options */ /*@keep@*/ /*@null@*/ char *name; /* display name, if any */ /*@keep@*/ /*@null@*/ char *format; /* output format, if any */ /*@keep@*/ /*@null@*/ char *pidfile; /* PID file, if any */ - int argc; /* number of non-option arguments */ + unsigned int argc; /* number of non-option arguments */ /*@keep@*/ /*@null@*/ char **argv; /* array of non-option arguments */ + unsigned int argv_length; /* allocated array size */ }; /*@-exportlocal@*/ /* splint thinks opts_free is exported but not used - it is used. */ -extern /*@null@*/ /*@only@*/ opts_t opts_parse(int, char **); +extern /*@null@*/ /*@only@*/ opts_t opts_parse(unsigned int, char **); extern void opts_free(/*@only@*/ opts_t); - +extern bool opts_add_file(opts_t, char *); #ifdef __cplusplus } diff --git a/src/include/pv-internal.h b/src/include/pv-internal.h index 5de1c2a..fb31cc1 100644 --- a/src/include/pv-internal.h +++ b/src/include/pv-internal.h @@ -77,7 +77,7 @@ struct pvstate_s { /*************** * Input files * ***************/ - int input_file_count; /* number of input files */ + unsigned int input_file_count; /* number of input files */ const char **input_files; /* input files (0=first) */ /******************* @@ -106,7 +106,7 @@ struct pvstate_s { int watch_fd; /* fd to watch */ unsigned int width; /* screen width */ unsigned int height; /* screen height */ - const char *name; /* display name */ + /*@null@*/ const char *name; /* display name */ char default_format[PV_SIZEOF_DEFAULT_FORMAT]; /* default format string */ const char *format_string; /* output format string */ diff --git a/src/include/pv.h b/src/include/pv.h index fc6f953..ec1b415 100644 --- a/src/include/pv.h +++ b/src/include/pv.h @@ -79,7 +79,7 @@ extern size_t pv_strlcat(char *, const char *, size_t); /* * Create a new state structure, and return it, or 0 (NULL) on error. */ -extern pvstate_t pv_state_alloc(const char *); +extern /*@null@*/ /*@only@*/ pvstate_t pv_state_alloc(const char *); /* * Set the formatting string, given a set of old-style formatting options. @@ -90,7 +90,7 @@ extern void pv_state_set_format(pvstate_t state, bool progress, bool average_rate, bool bytes, bool bufpercent, unsigned int lastwritten, - const char *name); + /*@null@*/ const char *name); /* * Set the various options. @@ -119,9 +119,9 @@ extern void pv_state_name_set(pvstate_t, const char *); extern void pv_state_format_string_set(pvstate_t, const char *); extern void pv_state_watch_pid_set(pvstate_t, unsigned int); extern void pv_state_watch_fd_set(pvstate_t, int); -extern void pv_state_average_rate_window_set(pvstate_t, int); +extern void pv_state_average_rate_window_set(pvstate_t, unsigned int); -extern void pv_state_inputfiles(pvstate_t, int, const char **); +extern void pv_state_inputfiles(pvstate_t, unsigned int, const char **); /* * Work out whether we are in the foreground. @@ -166,7 +166,7 @@ extern void pv_sig_fini(pvstate_t); /* * Free a state structure, after which it can no longer be used. */ -extern void pv_state_free(pvstate_t); +extern void pv_state_free(/*@only@*/ pvstate_t); #ifdef ENABLE_DEBUGGING diff --git a/src/main/main.c b/src/main/main.c index 06b9dbd..a1f49f8 100644 --- a/src/main/main.c +++ b/src/main/main.c @@ -35,8 +35,8 @@ void pv_remote_fini(void); int main(int argc, char **argv) { struct termios t, t_save; - opts_t opts; - pvstate_t state; + /*@only@ */ opts_t opts = NULL; + /*@only@ */ pvstate_t state = NULL; bool t_saved, t_needs_reset; int retcode = 0; @@ -46,7 +46,7 @@ int main(int argc, char **argv) (void) textdomain(PACKAGE); #endif - opts = opts_parse(argc, argv); + opts = opts_parse(argc >= 0 ? (unsigned int) argc : 0, argv); if (NULL == opts) { debug("%s: %d", "exiting with status", 64); return 64; @@ -72,10 +72,17 @@ int main(int argc, char **argv) */ state = pv_state_alloc(opts->program_name); if (NULL == state) { + /*@-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", opts->program_name, _("state allocation failed"), strerror(errno)); opts_free(opts); debug("%s: %d", "exiting with status", 64); return 64; + /*@+mustfreefresh@ */ } /* @@ -85,10 +92,12 @@ int main(int argc, char **argv) FILE *pidfptr; pidfptr = fopen(opts->pidfile, "w"); if (NULL == pidfptr) { + /*@-mustfreefresh@ *//* see above */ fprintf(stderr, "%s: %s: %s\n", opts->program_name, opts->pidfile, strerror(errno)); pv_state_free(state); opts_free(opts); return 1; + /*@+mustfreefresh@ */ } fprintf(pidfptr, "%d\n", getpid()); if (0 != fclose(pidfptr)) { @@ -101,13 +110,19 @@ int main(int argc, char **argv) */ if (0 == opts->argc) { debug("%s", "no files given - adding fake argument `-'"); - opts->argv[opts->argc++] = "-"; + if (!opts_add_file(opts, "-")) { + pv_state_free(state); + opts_free(opts); + return 64; + } } /* * Put our list of files into the PV internal state. */ - pv_state_inputfiles(state, opts->argc, (const char **) (opts->argv)); + if (NULL != opts->argv) { + pv_state_inputfiles(state, opts->argc, (const char **) (opts->argv)); + } if (0 == opts->watch_pid) { /* @@ -200,8 +215,13 @@ int main(int argc, char **argv) pv_state_target_buffer_size_set(state, opts->buffer_size); pv_state_no_splice_set(state, opts->no_splice); pv_state_size_set(state, opts->size); - pv_state_name_set(state, opts->name); - pv_state_format_string_set(state, opts->format); + + if (NULL != opts->name) + pv_state_name_set(state, opts->name); + + if (NULL != opts->format) + pv_state_format_string_set(state, opts->format); + pv_state_watch_pid_set(state, opts->watch_pid); pv_state_watch_fd_set(state, opts->watch_fd); pv_state_average_rate_window_set(state, opts->average_rate_window); @@ -239,8 +259,10 @@ int main(int argc, char **argv) debug("%s", "saved terminal attributes"); t_saved = true; } else { + /*@-mustfreefresh@ *//* see above */ fprintf(stderr, "%s: %s: %s\n", opts->program_name, _("failed to read terminal attributes"), strerror(errno)); + /*@+mustfreefresh@ */ } } t_save = t; diff --git a/src/main/options.c b/src/main/options.c index c1c9d53..d079cad 100644 --- a/src/main/options.c +++ b/src/main/options.c @@ -39,7 +39,7 @@ char *xstrdup(const char *original) return NULL; } - length = strlen(original); /* flawfinder: ignore */ + length = strlen(original); /* flawfinder: ignore */ /* * flawfinder rationale: the original string is explicitly required * to be \0 terminated. @@ -84,18 +84,54 @@ void opts_free( /*@only@ */ opts_t opts) free(opts); } +/* + * Add a filename to the list of non-option arguments, returning false on + * error. The filename is not copied - the pointer is stored. + */ +bool opts_add_file(opts_t opts, char *filename) +{ + /*@-branchstate@ */ + if ((opts->argc >= opts->argv_length) || (NULL == opts->argv)) { + opts->argv_length = opts->argc + 10; + /*@-keeptrans@ */ + opts->argv = realloc(opts->argv, opts->argv_length * sizeof(char *)); + /*@+keeptrans@ */ + if (NULL == opts->argv) { + fprintf(stderr, "%s: %s\n", opts->program_name, strerror(errno)); + opts->argv_length = 0; + opts->argc = 0; + return false; + } + } + /*@+branchstate@ */ + + /* + * splint notes: we turned off "branchstate" above because depending + * on whether we have to extend the array, we change argv from + * "keep" to "only", which is also why we turned off "keeptrans"; + * there doesn't seem to be a clean way to tell splint that everyone + * else should not touch argv but we're allowed to reallocate it and + * so is opts_parse. + */ + + opts->argv[opts->argc++] = filename; + + return true; +} + /* * Parse the given command-line arguments into an opts_t object, handling * "help" and "version" options internally. * - * Returns an opts_t, or 0 on error. + * Returns an opts_t, or NULL on error. * * Note that the contents of *argv[] (i.e. the command line parameters) * aren't copied anywhere, just the pointers are copied, so make sure the * command line data isn't overwritten or argv[1] free()d or whatever. */ - /*@null@ *//*@only@ */ opts_t opts_parse(int argc, char **argv) + /*@null@ *//*@only@ */ +opts_t opts_parse(unsigned int argc, char **argv) { #ifdef HAVE_GETOPT_LONG /*@-nullassign@ */ @@ -189,6 +225,7 @@ void opts_free( /*@only@ */ opts_t opts) return NULL; /*@+mustfreefresh@ */ } + opts->argv_length = 1 + argc; numopts = 0; @@ -200,9 +237,9 @@ void opts_free( /*@only@ */ opts_t opts) do { #ifdef HAVE_GETOPT_LONG - c = getopt_long(argc, argv, short_options, long_options, &option_index); /* flawfinder: ignore */ + c = getopt_long((int) argc, argv, short_options, long_options, &option_index); /* flawfinder: ignore */ #else - c = getopt(argc, argv, short_options); /* flawfinder: ignore */ + c = getopt((int) argc, argv, short_options); /* flawfinder: ignore */ #endif /* * flawfinder rationale: we have to pass argv to getopt, and @@ -519,7 +556,7 @@ void opts_free( /*@only@ */ opts_t opts) /*@+mustfreefresh@ */ } - if (optind < argc) { + if (optind < (int) argc) { /*@-mustfreefresh@ *//* see above */ fprintf(stderr, "%s: %s\n", opts->program_name, _("cannot transfer files when watching file descriptors")); @@ -559,8 +596,11 @@ void opts_free( /*@only@ */ opts_t opts) /* * Store remaining command-line arguments. */ - while (optind < argc) { - opts->argv[opts->argc++] = argv[optind++]; + while (optind < (int) argc) { + if (!opts_add_file(opts, argv[optind++])) { + opts_free(opts); + return NULL; + } } return opts; diff --git a/src/pv/state.c b/src/pv/state.c index 2e23aa2..76adefe 100644 --- a/src/pv/state.c +++ b/src/pv/state.c @@ -109,10 +109,8 @@ void pv_state_free(pvstate_t state) /* * Set the formatting string, given a set of old-style formatting options. */ -void pv_state_set_format(pvstate_t state, bool progress, - bool timer, bool eta, - bool fineta, bool rate, - bool average_rate, bool bytes, bool bufpercent, unsigned int lastwritten, const char *name) +void pv_state_set_format(pvstate_t state, bool progress, bool timer, bool eta, bool fineta, bool rate, bool average_rate, bool bytes, bool bufpercent, unsigned int lastwritten, /*@null@ */ + const char *name) { #define PV_ADDFORMAT(x,y) if (x) { \ if (state->default_format[0] != '\0') \ @@ -263,7 +261,7 @@ 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, int val) +void pv_state_average_rate_window_set(pvstate_t state, unsigned int val) { if (val < 1) val = 1; @@ -281,7 +279,7 @@ void pv_state_average_rate_window_set(pvstate_t state, int val) /* * Set the array of input files. */ -void pv_state_inputfiles(pvstate_t state, int input_file_count, const char **input_files) +void pv_state_inputfiles(pvstate_t state, unsigned int input_file_count, const char **input_files) { state->input_file_count = input_file_count; state->input_files = input_files;