[lxc-devel] [PATCH] Set default paths for unprivileged use

[Serge Hallyn]

Quoting Stéphane Graber (stgraber at ubuntu.com):
> On Thu, Dec 19, 2013 at 05:32:57PM -0600, Serge Hallyn wrote:
> > Quoting Stéphane Graber (stgraber at ubuntu.com):
> > > When running unprivileged (euid != 0), LXC will now use the following paths:
> > >  - Default lxc path: ~/.local/share/lxc/
> > >  - Default config path: ~/.config/lxc/lxc.conf
> > > 
> > > Those two paths are based on standard XDG paths (though ignoring all the
> > > possible override paths for now at least) and so probably don't need to
> > > be configurable at build time.
> > > 
> > > Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
> > 
> > Two comments on freeing user_lxc_path:
> > 
> > > ---
> > >  src/lxc/utils.c | 37 ++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 34 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/lxc/utils.c b/src/lxc/utils.c
> > > index 785f3e6..9fd91b7 100644
> > > --- a/src/lxc/utils.c
> > > +++ b/src/lxc/utils.c
> > > @@ -39,6 +39,7 @@
> > >  #include <sys/types.h>
> > >  #include <sys/wait.h>
> > >  #include <assert.h>
> > > +#include <wordexp.h>
> > >  
> > >  #ifndef HAVE_GETLINE
> > >  #ifdef HAVE_FGETLN
> > > @@ -248,11 +249,30 @@ const char *lxc_global_config_value(const char *option_name)
> > >  		{ "lvm_vg",          DEFAULT_VG      },
> > >  		{ "lvm_thin_pool",   DEFAULT_THIN_POOL },
> > >  		{ "zfsroot",         DEFAULT_ZFSROOT },
> > > -		{ "lxcpath",         LXCPATH         },
> > > +		{ "lxcpath",         NULL            },
> > >  		{ "cgroup.pattern",  DEFAULT_CGROUP_PATTERN },
> > >  		{ "cgroup.use",      NULL            },
> > >  		{ NULL, NULL },
> > >  	};
> > > +
> > > +	char *user_config_path;
> > > +	char *user_lxc_path;
> > > +	wordexp_t exp;
> > > +
> > > +	if (geteuid() > 0) {
> > > +		wordexp("~/.config/lxc/lxc.conf", &exp, 0);
> > > +		user_config_path = strdup(exp.we_wordv[0]);
> > > +		wordfree(&exp);
> > > +
> > > +		wordexp("~/.local/share/lxc/", &exp, 0);
> > > +		user_lxc_path = strdup(exp.we_wordv[0]);
> > > +		wordfree(&exp);
> > > +	}
> > > +	else {
> > > +		user_config_path = strdup(LXC_GLOBAL_CONF);
> > > +		user_lxc_path = strdup(LXCPATH);
> > > +	}
> > > +
> > >  	/* placed in the thread local storage pool */
> > >  	static __thread const char *values[sizeof(options) / sizeof(options[0])] = { 0 };
> > >  	const char *(*ptr)[2];
> > > @@ -266,17 +286,23 @@ const char *lxc_global_config_value(const char *option_name)
> > >  			break;
> > >  	}
> > >  	if (!(*ptr)[0]) {
> > > +		free(user_config_path);
> > > +		free(user_lxc_path);
> > >  		errno = EINVAL;
> > >  		return NULL;
> > >  	}
> > >  
> > >  	if (values[i]) {
> > > +		free(user_config_path);
> > > +		free(user_lxc_path);
> > >  		value = values[i];
> > > +
> > >  		return value;
> > >  	}
> > >  
> > >  	process_lock();
> > > -	fin = fopen_cloexec(LXC_GLOBAL_CONF, "r");
> > > +	fin = fopen_cloexec(user_config_path, "r");
> > > +	free(user_config_path);
> > >  	process_unlock();
> > >  	if (fin) {
> > >  		while (fgets(buf, 1024, fin)) {
> > > @@ -315,7 +341,12 @@ const char *lxc_global_config_value(const char *option_name)
> > >  		}
> > >  	}
> > >  	/* could not find value, use default */
> > > -	values[i] = (*ptr)[1];
> > > +	if (strcmp(option_name, "lxcpath") == 0)
> > > +		values[i] = user_lxc_path;
> > > +	else {
> > > +		free(user_config_path);
> > 
> > Did you mean to free user_lxc_path here?
> 
> I did indeed, user_config_path is always freed right after fopen.
> 
> > 
> > > +		values[i] = (*ptr)[1];
> > > +	}
> > >  	/* special case: if default value is NULL,
> > >  	 * and there is no config, don't view that
> > >  	 * as an error... */
> > 
> > Below here is an out: path.  I don't think you're freeing
> > user_lxc_path on that codepath.
> 
> So with the free above corrected to free user_config_path, it'll either
> have been already freed or be returned so shouldn't need any further
> freeing.

It only is returned or freed if you go through the loop not
finding what you were looking for.  If you find what you
were looking for in the loop reading through the conffile,
then you jump past the '/* could not find value, use default */'
block where you either assign or free lxcpath.