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

@@ -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)

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)

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

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++;
 }

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);