From cd793ecebd87b2dcd9db6bb38bbf729e8957808d Mon Sep 17 00:00:00 2001 From: thor Date: Sun, 26 Apr 2020 14:04:33 +0000 Subject: [PATCH] getlopt: Machinery to avoid leaking strdup() memory. Hm, why are we using strdup, anyway? git-svn-id: svn://scm.orgis.org/mpg123/trunk@4657 35dc7657-300d-0410-a2e5-dc2837fedb53 --- NEWS | 2 ++ src/getlopt.c | 37 ++++++++++++++++++++---- src/getlopt.h | 13 ++++++--- src/mpg123-strip.c | 4 +-- src/mpg123.c | 70 +++++++++++++++++++++++----------------------- 5 files changed, 79 insertions(+), 47 deletions(-) diff --git a/NEWS b/NEWS index 91d9573c..6a1c6093 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,8 @@ It is an open question how that should be developed in relation to the external regression and compliance test suite. Maybe some really small example streams and decode tests will follow. +- Finally silenced memory checkers about leaking memory from getlopt() + (main code overwriting values without freeing strdup() strings). - AUTORS now in UTF-8;-) - CMake build files in ports/cmake, as an alternative to create MSVC project files and the like (thanks to Vitaly Kirsanov) diff --git a/src/getlopt.c b/src/getlopt.c index 36b0cc11..f56dec3a 100644 --- a/src/getlopt.c +++ b/src/getlopt.c @@ -33,7 +33,32 @@ topt *findopt (int islong, char *opt, topt *opts) return (0); } -static int performoption (int argc, char *argv[], topt *opt) +static void setcharoption(topt *opt, char *value) +{ + if(!opt->var) + { + merror("Option %s has no argument pointer!", opt->lname); + return; + } + if(opt->flags & GLO_VAR_MEM) + free(*((char**)opt->var)); + if(value) + { + *((char **) opt->var) = compat_strdup(value); + opt->flags |= GLO_VAR_MEM; + } else + opt->flags &= ~GLO_VAR_MEM; +} + +void getlopt_set_char(topt *opts, char *name, char *value) +{ + topt *opt = findopt(1, name, opts); + if(!opt) + return; + setcharoption(opt, value); +} + +static int performoption (int argc, char *argv[], topt *opt, topt *opts) { int result = GLO_CONTINUE; /* this really is not supposed to happen, so the exit may be justified to create asap ficing pressure */ @@ -50,7 +75,7 @@ static int performoption (int argc, char *argv[], topt *opt) if (opt->flags & GLO_CHAR) /* var is *char */ { debug1("char at %p", opt->var); - *((char *) opt->var) = (char) opt->value;\ + *((char *) opt->var) = (char) opt->value; } else if(opt->flags & GLO_LONG) { @@ -80,7 +105,7 @@ static int performoption (int argc, char *argv[], topt *opt) loptchr = 0; if (opt->var) { if (opt->flags & GLO_CHAR) /* var is *char */ - *((char **) opt->var) = compat_strdup(loptarg); /* valgrind claims lost memory here */ + setcharoption(opt, loptarg); else if(opt->flags & GLO_LONG) *((long *) opt->var) = atol(loptarg); else if(opt->flags & GLO_INT) @@ -95,7 +120,7 @@ static int performoption (int argc, char *argv[], topt *opt) #endif } if (opt->func) - opt->func(loptarg); + opt->func(loptarg, opts); debug4("result: %i (%p, %li, %i)", result, opt->var, opt->value, opt->sname); return (result); } @@ -120,7 +145,7 @@ int getsingleopt (int argc, char *argv[], topt *opts) if (!(opt = findopt(1, thisopt+2, opts))) return (GLO_UNKNOWN); else - return (performoption(argc, argv, opt)); + return (performoption(argc, argv, opt, opts)); } else { /* "--" == end of options */ loptind++; @@ -140,7 +165,7 @@ int getsingleopt (int argc, char *argv[], topt *opts) if (!opt) return (GLO_UNKNOWN); else - return (performoption(argc, argv, opt)); + return (performoption(argc, argv, opt, opts)); } int getlopt (int argc, char *argv[], topt *opts) diff --git a/src/getlopt.h b/src/getlopt.h index 3fca39cf..85014b6a 100644 --- a/src/getlopt.h +++ b/src/getlopt.h @@ -18,14 +18,16 @@ extern int loptind; /* index in argv[] */ extern int loptchr; /* index in argv[loptind] */ extern char *loptarg; /* points to argument if present, else to option */ -typedef struct { +struct topt; +typedef struct topt topt; +struct topt { char sname; /* short option name, can be 0 */ char *lname; /* long option name, can be 0 */ int flags; /* see below */ - void (*func)(char *); /* called if != 0 (after setting of var) */ + void (*func)(char *, topt *); /* called if != 0 (after setting of var) */ void *var; /* type is *long, *char or **char, see below */ long value; -} topt; +}; /* ThOr: make this clear; distict long from int (since this is != on my Alpha) and really use a flag for every case (spare the 0 case for .... no flag) */ @@ -34,6 +36,8 @@ for .... no flag) */ #define GLO_INT 4 #define GLO_LONG 8 #define GLO_DOUBLE 16 +/* This is set if getlopt allocates memory for var. */ +#define GLO_VAR_MEM 32 /* flags: * bit 0 = 0 - no argument @@ -64,7 +68,8 @@ for .... no flag) */ #define GLO_CONTINUE -3 int getlopt (int argc, char *argv[], topt *opts); - +// Helper to set a char parameter, avoiding memory leaks. +void getlopt_set_char(topt *opts, char *name, char *value); /* return values: * GLO_END (0) end of options * GLO_UNKNOWN (-1) unknown option *loptarg diff --git a/src/mpg123-strip.c b/src/mpg123-strip.c index be465233..97e4f0e2 100644 --- a/src/mpg123-strip.c +++ b/src/mpg123-strip.c @@ -46,12 +46,12 @@ static void usage(int err) exit(err); } -static void want_usage(char* bla) +static void want_usage(char* bla, topt *opts) { usage(0); } -static void set_verbose (char *arg) +static void set_verbose (char *arg, topt *opts) { param.verbose++; } diff --git a/src/mpg123.c b/src/mpg123.c index d9560f13..dfe0d0da 100644 --- a/src/mpg123.c +++ b/src/mpg123.c @@ -45,11 +45,11 @@ #include "debug.h" static void usage(int err); -static void want_usage(char* arg); +static void want_usage(char* arg, topt *); static void long_usage(int err); -static void want_long_usage(char* arg); +static void want_long_usage(char* arg, topt *); static void print_title(FILE* o); -static void give_version(char* arg); +static void give_version(char* arg, topt *); struct parameter param = { FALSE , /* aggressiv */ @@ -328,7 +328,7 @@ static void check_fatal_output(int code) } } -static void set_output_module( char *arg ) +static void set_output_module(char *arg, topt *opts) { unsigned int i; @@ -336,7 +336,7 @@ static void set_output_module( char *arg ) for(i=0; i< strlen( arg ); i++) { if (arg[i] == ':') { arg[i] = 0; - param.output_device = &arg[i+1]; + getlopt_set_char(opts, "audiodevice", &arg[i+1]); debug1("Setting output device: %s", param.output_device); break; } @@ -352,38 +352,38 @@ static void set_output_flag(int flag) else param.output_flags |= flag; } -static void set_output_h(char *a) +static void set_output_h(char *a, topt *opts) { set_output_flag(OUT123_HEADPHONES); } -static void set_output_s(char *a) +static void set_output_s(char *a, topt *opts) { set_output_flag(OUT123_INTERNAL_SPEAKER); } -static void set_output_l(char *a) +static void set_output_l(char *a, topt *opts) { set_output_flag(OUT123_LINE_OUT); } -static void set_output(char *arg) +static void set_output(char *arg, topt *opts) { /* If single letter, it's the legacy output switch for AIX/HP/Sun. If longer, it's module[:device] . If zero length, it's rubbish. */ if(strlen(arg) <= 1) switch(arg[0]) { - case 'h': set_output_h(arg); break; - case 's': set_output_s(arg); break; - case 'l': set_output_l(arg); break; + case 'h': set_output_h(arg, opts); break; + case 's': set_output_s(arg, opts); break; + case 'l': set_output_l(arg, opts); break; default: error1("\"%s\" is no valid output", arg); safe_exit(1); } - else set_output_module(arg); + else set_output_module(arg, opts); } -static void set_resample(char *arg) +static void set_resample(char *arg, topt *opts) { if(!strcasecmp("ntom", arg)) param.resample = 0; @@ -398,51 +398,51 @@ static void set_resample(char *arg) } } -static void set_verbose (char *arg) +static void set_verbose (char *arg, topt *opts) { param.verbose++; } -static void set_quiet (char *arg) +static void set_quiet (char *arg, topt *opts) { param.verbose=0; param.quiet=TRUE; } -static void set_out_wav(char *arg) +static void set_out_wav(char *arg, topt *opts) { param.output_module = "wav"; - param.output_device = arg; + getlopt_set_char(opts, "audiodevice", arg); } -void set_out_cdr(char *arg) +void set_out_cdr(char *arg, topt *opts) { param.output_module = "cdr"; - param.output_device = arg; + getlopt_set_char(opts, "audiodevice", arg); } -void set_out_au(char *arg) +void set_out_au(char *arg, topt *opts) { param.output_module = "au"; - param.output_device = arg; + getlopt_set_char(opts, "audiodevice", arg); } -void set_out_test(char *arg) +void set_out_test(char *arg, topt *opts) { param.output_module = "test"; - param.output_device = NULL; + getlopt_set_char(opts, "audiodevice", NULL); } -static void set_out_file(char *arg) +static void set_out_file(char *arg, topt *opts) { param.output_module = "raw"; - param.output_device = arg; + getlopt_set_char(opts, "audiodevice", arg); } -static void set_out_stdout(char *arg) +static void set_out_stdout(char *arg, topt *opts) { param.output_module = "raw"; - param.output_device = NULL; + getlopt_set_char(opts, "audiodevice", NULL); } #if !defined (HAVE_SCHED_SETSCHEDULER) && !defined (HAVE_WINDOWS_H) @@ -453,24 +453,24 @@ static void realtime_not_compiled(char *arg) #endif static int frameflag; /* ugly, but that's the way without hacking getlopt */ -static void set_frameflag(char *arg) +static void set_frameflag(char *arg, topt *opts) { /* Only one mono flag at a time! */ if(frameflag & MPG123_FORCE_MONO) param.flags &= ~MPG123_FORCE_MONO; param.flags |= frameflag; } -static void unset_frameflag(char *arg) +static void unset_frameflag(char *arg, topt *opts) { param.flags &= ~frameflag; } static int appflag; /* still ugly, but works */ -static void set_appflag(char *arg) +static void set_appflag(char *arg, topt *opts) { param.appflags |= appflag; } -static void list_output_modules(char *arg) +static void list_output_modules(char *arg, topt *opts) { char **names = NULL; char **descr = NULL; @@ -1556,7 +1556,7 @@ static void usage(int err) /* print syntax & exit */ safe_exit(err); } -static void want_usage(char* arg) +static void want_usage(char* arg, topt *opts) { usage(0); } @@ -1707,12 +1707,12 @@ static void long_usage(int err) safe_exit(err); } -static void want_long_usage(char* arg) +static void want_long_usage(char* arg, topt *opts) { long_usage(0); } -static void give_version(char* arg) +static void give_version(char* arg, topt *opts) { fprintf(stdout, PACKAGE_NAME" "PACKAGE_VERSION"\n"); safe_exit(0);