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