(patch) memory leak in loadparm.c

[Dave Dykstra]

Isn't there some solution that doesn't have to explicitly list every
variable name?  I think that's asking for future bugs; just because there's
an instruction in a comment doesn't mean people will remember to do what
it says when they add a new variable.

- Dave Dykstra


On Fri, Dec 28, 2001 at 07:30:33AM -0500, Colin Walters wrote:
> Hi, I spent a little bit of time trying to debug a segfault in rsync
> 2.5.0, and after discovering it was in loadparm.c, I noticed that the
> bug had been worked around in the current CVS by removing a free()
> statement.
> 
> That fix seems less than optimal; here's a patch which should fix the
> memory leak.
> 
> 

> --- loadparm.c.~1.40.~	Sun Dec  2 03:16:15 2001
> +++ loadparm.c	Fri Dec 28 07:22:25 2001
> @@ -108,7 +108,8 @@
>  
>  
>  /* 
> - * This structure describes a single service. 
> + * This structure describes a single service.  If you update this, be
> + * sure to update init_globals below!
>   */
>  typedef struct
>  {
> @@ -179,6 +180,7 @@
>  static int iNumServices = 0;
>  static int iServiceIndex = 0;
>  static BOOL bInGlobalSection = True;
> +static BOOL bsDefaultInitialized = False;
>  
>  #define NUMPARAMETERS (sizeof(parm_table) / sizeof(struct parm_struct))
>  
> @@ -297,6 +299,28 @@
>  #ifdef LOG_DAEMON
>  	Globals.syslog_facility = LOG_DAEMON;
>  #endif
> +  if (!bsDefaultInitialized) {
> +    bsDefaultInitialized = True;
> +#define maybe_init(x) if (sDefault.x != NULL) sDefault.x = strdup(sDefault.x)
> +    maybe_init(name);
> +    maybe_init(path);
> +    maybe_init(comment);
> +    maybe_init(lock_file);
> +    maybe_init(uid);
> +    maybe_init(gid);
> +    maybe_init(hosts_allow);
> +    maybe_init(hosts_deny);
> +    maybe_init(auth_users);
> +    maybe_init(secrets_file);
> +    maybe_init(exclude);
> +    maybe_init(exclude_from);
> +    maybe_init(include);
> +    maybe_init(include_from);
> +    maybe_init(log_format);
> +    maybe_init(refuse_options);
> +    maybe_init(dont_compress);
> +#undef maybe_init
> +  }
>  }
>  
>  /***************************************************************************
> @@ -386,16 +410,9 @@
>  
>  
>  /**
> - * Assign a copy of @p v to @p *s.  Handles NULL strings.  @p *v must
> - * be initialized when this is called, either to NULL or a malloc'd
> - * string.
> - *
> - * @fixme There is a small leak here in that sometimes the existing
> - * value will be dynamically allocated, and the old copy is lost.
> - * However, we can't always deallocate the old value, because in the
> - * case of sDefault, it points to a static string.  It would be nice
> - * to have either all-strdup'd values, or to never need to free
> - * memory.
> + * Assign a copy of @p v to @p *s, freeing any existing values and
> + * handling NULL strings.  @p *v must be initialized when this is
> + * called, either to NULL or a malloc'd string.
>   **/
>  static void string_set(char **s, const char *v)
>  {
> @@ -403,6 +420,8 @@
>  		*s = NULL;
>  		return;
>  	}
> +	if (*s)
> +		free(*s);
>  	*s = strdup(v);
>  	if (!*s)
>  		exit_cleanup(RERR_MALLOC);