[lxc-devel] [PATCH 2/2 v3] Update absolute paths for overlay and aufs mounts

[Christian Brauner]

On Mon, Nov 02, 2015 at 03:12:02PM +0000, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrauner at gmail.com):
> > When using overlay and aufs mounts with lxc.mount.entry users have to specify
> > absolute paths for upperdir and workdir which will then get created
> > automatically by mount_entry_create_overlay_dirs() and
> > mount_entry_create_aufs_dirs() in conf.c. When we clone a container with
> > overlay or aufs lxc.mount.entry entries we need to update these absolute paths.
> > In order to do this we add the function update_ovl_paths() in
> > lxccontainer.c. The function updates the mounts in two locations:
> > 
> >         1) lxc_conf->mount_list
> > 
> > and
> > 
> >         2) lxc_conf->unexpanded_config (by calling clone_update_unexp_ovl_dir())
> > 
> > If we were to only update 2) we would end up with wrong upperdir and workdir
> > mounts as the absolute paths would still point to the container that serves as
> > the base for the clone. If we were to only update 1) we would end up with wrong
> > upperdir and workdir lxc.mount.entry entries in the clone's config as the
> > absolute paths in upperdir and workdir would still point to the container that
> > serves as the base for the clone. Updating both will get the job done.
> > 
> > NOTE: This function does not sanitize paths apart from removing trailing
> > slashes. (So when a user specifies //home//someone/// it will be cleaned to
> > //home//someone. This is the minimal path cleansing which is also done by
> > lxc_container_new().) But the mount_entry_create_overlay_dirs() and
> > mount_entry_create_aufs_dirs() functions both try to be extremely strict about
> > when to create upperdirs and workdirs. They will only accept sanitized paths,
> > i.e. they require /home/someone. I think this is a (safety) virtue and we
> > should consider sanitizing paths in general. In short: update_ovl_paths() does
> > update all absolute paths to the new container but
> > mount_entry_create_overlay_dirs() and mount_entry_create_aufs_dirs() will still
> > refuse to create upperdir and workdir when the updated path is unclean. This
> > happens easily when e.g. a user calls lxc-clone -o OLD -n NEW -P
> > //home//chb///.
> > 
> > Signed-off-by: Christian Brauner <christianvanbrauner at gmail.com>
> > ---
> >  src/lxc/lxccontainer.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 98 insertions(+)
> > 
> > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > index 42e23e7..7150103 100644
> > --- a/src/lxc/lxccontainer.c
> > +++ b/src/lxc/lxccontainer.c
> > @@ -2894,6 +2894,100 @@ static int create_file_dirname(char *path, struct lxc_conf *conf)
> >  	return ret;
> >  }
> >  
> > +/* When we clone a container with overlay lxc.mount.entry entries we need to
> > +*  update absolute paths for upper- and workdir. This update is done in two
> > +*  locations: lxc_conf->unexpanded_config and lxc_conf->mount_list. Both updates
> > +*  are done independent of each other since lxc_conf->mountlist may container
> > +*  more mount entries (e.g. from other included files) than
> > +*  lxc_conf->unexpanded_config . */
> > +static int update_ovl_paths(struct lxc_conf *lxc_conf, const char *lxc_path,
> > +			    const char *lxc_name, const char *newpath,
> > +			    const char *newname)
> > +{
> > +	char new_upper[MAXPATHLEN];
> > +	char new_work[MAXPATHLEN];
> > +	char old_upper[MAXPATHLEN];
> > +	char old_work[MAXPATHLEN];
> > +	char *cleanpath = NULL;
> > +	int i;
> > +	int fret = -1;
> > +	int ret = 0;
> > +	struct lxc_list *iterator;
> > +	const char *ovl_dirs[] = {"br", "upperdir", "workdir"};
> > +
> > +	cleanpath = strdup(newpath);
> > +	if (!cleanpath)
> > +		goto err;
> > +
> > +	remove_trailing_slashes(cleanpath);
> > +
> > +	/* We have to update lxc_conf->unexpanded_config separately from
> > +	*  lxc_conf->mount_list. */
> > +	for (i = 0; i < sizeof(ovl_dirs) / sizeof(ovl_dirs[0]); i++) {
> > +		if (!clone_update_unexp_ovl_paths(lxc_conf, lxc_path, newpath,
> > +						  lxc_name, newname,
> > +						  ovl_dirs[i]))
> > +			goto err;
> > +	}
> > +
> > +	ret = snprintf(old_work, MAXPATHLEN, "workdir=%s/%s", lxc_path, lxc_name);
> > +	if (ret < 0 || ret >= MAXPATHLEN)
> > +		goto err;
> > +
> > +	ret = snprintf(new_work, MAXPATHLEN, "workdir=%s/%s", cleanpath, newname);
> > +	if (ret < 0 || ret >= MAXPATHLEN)
> > +		goto err;
> > +
> > +	lxc_list_for_each(iterator, &lxc_conf->mount_list) {
> > +		char *mnt_entry = NULL;
> > +		char *new_mnt_entry = NULL;
> > +		char *tmp = NULL;
> > +		char *tmp_mnt_entry = NULL;
> > +		mnt_entry = iterator->elem;
> > +
> > +		if (strstr(mnt_entry, "overlay"))
> > +			tmp = "upperdir";
> > +		else if (strstr(mnt_entry, "aufs"))
> > +			tmp = "br";
> > +
> > +		if (!tmp)
> > +			continue;
> > +
> 
> The rest of this block needs to be un-indented by one level.
> 
> > +			ret = snprintf(old_upper, MAXPATHLEN, "%s=%s/%s", tmp, lxc_path, lxc_name);
> > +			if (ret < 0 || ret >= MAXPATHLEN)
> > +				goto err;
> > +
> > +			ret = snprintf(new_upper, MAXPATHLEN, "%s=%s/%s", tmp, cleanpath, newname);
> > +			if (ret < 0 || ret >= MAXPATHLEN)
> > +				goto err;
> > +
> > +			if (strstr(mnt_entry, old_upper)) {
> > +				tmp_mnt_entry = lxc_string_replace(old_upper, new_upper, mnt_entry);
> > +			}
> > +
> > +			if (strstr(mnt_entry, old_work)) {
> > +				if (tmp_mnt_entry)
> > +					new_mnt_entry = lxc_string_replace(old_work, new_work, tmp_mnt_entry);
> 
> Don't you want old_work replaced even if old_upper wasn't found?
> 
> (or are we requiring limitations on the old upper that guarantee that
> won't happen?)
I don't think we currently are. We should:

        if (strstr(mnt_entry, old_work)) {
                if (tmp_mnt_entry)
                        new_mnt_entry = lxc_string_replace(old_work, new_work, tmp_mnt_entry);
                else (tmp_mnt_entry)
			new_mnt_entry = lxc_string_replace(old_work, new_work, mnt_entry);
        }