[lxc-devel] [PATCH] Make mount_entry_create_*_dirs() more robust

Christian Brauner christianvanbrauner at gmail.com
Thu Oct 15 22:33:00 UTC 2015


On Thu, Oct 15, 2015 at 08:32:25PM +0000, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrauner at gmail.com):
> > The mount_entry_create_*_dirs() functions currently assume that the rootfs of
> > the container is actually named "rootfs". This has the consequence that
> > 
> > 	del = strstr(lxcpath, "/rootfs");
> > 	if (!del) {
> > 		free(lxcpath);
> > 		lxc_free_array((void **)opts, free);
> > 		return -1;
> > 	}
> > 	*del = '\0';
> > 
> > will return NULL when the rootfs of a container is not actually named "rootfs".
> > This means the we return -1 and do not create the necessary upperdir/workdir
> > directories required for the overlay/aufs mount to work. Hence, let's not make
> > that assumption. We now pass lxc_path and lxc_name to
> > mount_entry_create_*_dirs() and create the path directly. To prevent failure we
> > also have mount_entry_create_*_dirs() check that lxc_name and lxc_path are not
> > empty when they are passed in.
> > 
> > Signed-off-by: Christian Brauner <christianvanbrauner at gmail.com>
> 
> Yeah this was bugging me a few years ago.  Overall the patch looks fine
> to me, I'm running a full testsuite to ease my mind about it.  Will ack
> after that passes and I look over it again.

We should also consider parsing path->rootfs when the container is an overlay or
aufs backed container. Because in this case the right hand side of the check:

		if ((strncmp(upperdir, lxcpath, dirlen) == 0) && (strncmp(upperdir, rootfs->path, rootfslen) != 0))

will be trivially true since path->rootfs will e.g. be overlayfs:/path1:path2.
Parsing path->rootfs to extract path2 before doing the second check would be
safer... Thoughts?

> 
> > ---
> >  src/lxc/conf.c | 91 ++++++++++++++++++++++++++--------------------------------
> >  1 file changed, 41 insertions(+), 50 deletions(-)
> > 
> > diff --git a/src/lxc/conf.c b/src/lxc/conf.c
> > index 0e3421b..16a62f8 100644
> > --- a/src/lxc/conf.c
> > +++ b/src/lxc/conf.c
> > @@ -1816,20 +1816,22 @@ static void cull_mntent_opt(struct mntent *mntent)
> >  }
> >  
> >  static int mount_entry_create_overlay_dirs(const struct mntent *mntent,
> > -					   const struct lxc_rootfs *rootfs)
> > +					   const struct lxc_rootfs *rootfs,
> > +					   const char *lxc_name,
> > +					   const char *lxc_path)
> >  {
> > -	char *del = NULL;
> > -	char *lxcpath = NULL;
> > +	char lxcpath[MAXPATHLEN];
> >  	char *upperdir = NULL;
> >  	char *workdir = NULL;
> >  	char **opts = NULL;
> > +	int ret = 0;
> >  	size_t arrlen = 0;
> >  	size_t dirlen = 0;
> >  	size_t i;
> >  	size_t len = 0;
> >  	size_t rootfslen = 0;
> >  
> > -	if (!rootfs->path)
> > +	if (!rootfs->path || !lxc_name || !lxc_path)
> >  		return -1;
> >  
> >  	opts = lxc_string_split(mntent->mnt_opts, ',');
> > @@ -1845,19 +1847,11 @@ static int mount_entry_create_overlay_dirs(const struct mntent *mntent,
> >  			workdir = opts[i] + len;
> >  	}
> >  
> > -	lxcpath = strdup(rootfs->path);
> > -	if (!lxcpath) {
> > -		lxc_free_array((void **)opts, free);
> > -		return -1;
> > -	}
> > -
> > -	del = strstr(lxcpath, "/rootfs");
> > -	if (!del) {
> > -		free(lxcpath);
> > +	ret = snprintf(lxcpath, MAXPATHLEN, "%s/%s", lxc_path, lxc_name);
> > +	if (ret < 0 || ret >= MAXPATHLEN) {
> >  		lxc_free_array((void **)opts, free);
> >  		return -1;
> >  	}
> > -	*del = '\0';
> >  
> >  	dirlen = strlen(lxcpath);
> >  	rootfslen = strlen(rootfs->path);
> > @@ -1877,25 +1871,26 @@ static int mount_entry_create_overlay_dirs(const struct mntent *mntent,
> >  				WARN("Failed to create workdir");
> >  			}
> >  
> > -	free(lxcpath);
> >  	lxc_free_array((void **)opts, free);
> >  	return 0;
> >  }
> >  
> >  static int mount_entry_create_aufs_dirs(const struct mntent *mntent,
> > -					const struct lxc_rootfs *rootfs)
> > +					const struct lxc_rootfs *rootfs,
> > +					const char *lxc_name,
> > +					const char *lxc_path)
> >  {
> > -	char *del = NULL;
> > -	char *lxcpath = NULL;
> > +	char lxcpath[MAXPATHLEN];
> >  	char *scratch = NULL;
> >  	char *tmp = NULL;
> >  	char *upperdir = NULL;
> >  	char **opts = NULL;
> > +	int ret = 0;
> >  	size_t arrlen = 0;
> >  	size_t i;
> >  	size_t len = 0;
> >  
> > -	if (!rootfs->path)
> > +	if (!rootfs->path || !lxc_name || !lxc_path)
> >  		return -1;
> >  
> >  	opts = lxc_string_split(mntent->mnt_opts, ',');
> > @@ -1919,19 +1914,11 @@ static int mount_entry_create_aufs_dirs(const struct mntent *mntent,
> >  		return -1;
> >  	}
> >  
> > -	lxcpath = strdup(rootfs->path);
> > -	if (!lxcpath) {
> > -		lxc_free_array((void **)opts, free);
> > -		return -1;
> > -	}
> > -
> > -	del = strstr(lxcpath, "/rootfs");
> > -	if (!del) {
> > -		free(lxcpath);
> > +	ret = snprintf(lxcpath, MAXPATHLEN, "%s/%s", lxc_path, lxc_name);
> > +	if (ret < 0 || ret >= MAXPATHLEN) {
> >  		lxc_free_array((void **)opts, free);
> >  		return -1;
> >  	}
> > -	*del = '\0';
> >  
> >  	/* We neither allow users to create upperdirs outside the containerdir
> >  	 * nor inside the rootfs. The latter might be debatable. */
> > @@ -1940,23 +1927,24 @@ static int mount_entry_create_aufs_dirs(const struct mntent *mntent,
> >  			WARN("Failed to create upperdir");
> >  		}
> >  
> > -	free(lxcpath);
> >  	lxc_free_array((void **)opts, free);
> >  	return 0;
> >  }
> >  
> > +
> >  static int mount_entry_create_dir_file(const struct mntent *mntent,
> > -				       const char* path, const struct lxc_rootfs *rootfs)
> > +				       const char* path, const struct lxc_rootfs *rootfs,
> > +				       const char *lxc_name, const char *lxc_path)
> >  {
> >  	char *pathdirname = NULL;
> >  	int ret = 0;
> >  	FILE *pathfile = NULL;
> >  
> >  	if (strncmp(mntent->mnt_type, "overlay", 7) == 0) {
> > -		if (mount_entry_create_overlay_dirs(mntent, rootfs) < 0)
> > +		if (mount_entry_create_overlay_dirs(mntent, rootfs, lxc_name, lxc_path) < 0)
> >  			return -1;
> >  	} else if (strncmp(mntent->mnt_type, "aufs", 4) == 0) {
> > -		if (mount_entry_create_aufs_dirs(mntent, rootfs) < 0)
> > +		if (mount_entry_create_aufs_dirs(mntent, rootfs, lxc_name, lxc_path) < 0)
> >  			return -1;
> >  	}
> >  
> > @@ -1986,14 +1974,15 @@ static int mount_entry_create_dir_file(const struct mntent *mntent,
> >  }
> >  
> >  static inline int mount_entry_on_generic(struct mntent *mntent,
> > -                 const char* path, const struct lxc_rootfs *rootfs)
> > +                 const char* path, const struct lxc_rootfs *rootfs,
> > +		 const char *lxc_name, const char *lxc_path)
> >  {
> >  	unsigned long mntflags;
> >  	char *mntdata;
> >  	int ret;
> >  	bool optional = hasmntopt(mntent, "optional") != NULL;
> >  
> > -	ret = mount_entry_create_dir_file(mntent, path, rootfs);
> > +	ret = mount_entry_create_dir_file(mntent, path, rootfs, lxc_name, lxc_path);
> >  
> >  	if (ret < 0)
> >  		return optional ? 0 : -1;
> > @@ -2015,12 +2004,13 @@ static inline int mount_entry_on_generic(struct mntent *mntent,
> >  
> >  static inline int mount_entry_on_systemfs(struct mntent *mntent)
> >  {
> > -  return mount_entry_on_generic(mntent, mntent->mnt_dir, NULL);
> > +  return mount_entry_on_generic(mntent, mntent->mnt_dir, NULL, NULL, NULL);
> >  }
> >  
> >  static int mount_entry_on_absolute_rootfs(struct mntent *mntent,
> >  					  const struct lxc_rootfs *rootfs,
> > -					  const char *lxc_name)
> > +					  const char *lxc_name,
> > +					  const char *lxc_path)
> >  {
> >  	char *aux;
> >  	char path[MAXPATHLEN];
> > @@ -2062,11 +2052,13 @@ skipabs:
> >  		return -1;
> >  	}
> >  
> > -	return mount_entry_on_generic(mntent, path, rootfs);
> > +	return mount_entry_on_generic(mntent, path, rootfs, lxc_name, lxc_path);
> >  }
> >  
> >  static int mount_entry_on_relative_rootfs(struct mntent *mntent,
> > -					  const struct lxc_rootfs *rootfs)
> > +					  const struct lxc_rootfs *rootfs,
> > +					  const char *lxc_name,
> > +					  const char *lxc_path)
> >  {
> >  	char path[MAXPATHLEN];
> >  	int ret;
> > @@ -2078,11 +2070,11 @@ static int mount_entry_on_relative_rootfs(struct mntent *mntent,
> >  		return -1;
> >  	}
> >  
> > -	return mount_entry_on_generic(mntent, path, rootfs);
> > +	return mount_entry_on_generic(mntent, path, rootfs, lxc_name, lxc_path);
> >  }
> >  
> >  static int mount_file_entries(const struct lxc_rootfs *rootfs, FILE *file,
> > -	const char *lxc_name)
> > +	const char *lxc_name, const char *lxc_path)
> >  {
> >  	struct mntent mntent;
> >  	char buf[4096];
> > @@ -2098,13 +2090,12 @@ static int mount_file_entries(const struct lxc_rootfs *rootfs, FILE *file,
> >  
> >  		/* We have a separate root, mounts are relative to it */
> >  		if (mntent.mnt_dir[0] != '/') {
> > -			if (mount_entry_on_relative_rootfs(&mntent,
> > -							   rootfs))
> > +			if (mount_entry_on_relative_rootfs(&mntent, rootfs, lxc_name, lxc_path))
> >  				goto out;
> >  			continue;
> >  		}
> >  
> > -		if (mount_entry_on_absolute_rootfs(&mntent, rootfs, lxc_name))
> > +		if (mount_entry_on_absolute_rootfs(&mntent, rootfs, lxc_name, lxc_path))
> >  			goto out;
> >  	}
> >  
> > @@ -2116,7 +2107,7 @@ out:
> >  }
> >  
> >  static int setup_mount(const struct lxc_rootfs *rootfs, const char *fstab,
> > -	const char *lxc_name)
> > +	const char *lxc_name, const char *lxc_path)
> >  {
> >  	FILE *file;
> >  	int ret;
> > @@ -2130,7 +2121,7 @@ static int setup_mount(const struct lxc_rootfs *rootfs, const char *fstab,
> >  		return -1;
> >  	}
> >  
> > -	ret = mount_file_entries(rootfs, file, lxc_name);
> > +	ret = mount_file_entries(rootfs, file, lxc_name, lxc_path);
> >  
> >  	endmntent(file);
> >  	return ret;
> > @@ -2158,7 +2149,7 @@ FILE *write_mount_file(struct lxc_list *mount)
> >  }
> >  
> >  static int setup_mount_entries(const struct lxc_rootfs *rootfs, struct lxc_list *mount,
> > -	const char *lxc_name)
> > +	const char *lxc_name, const char *lxc_path)
> >  {
> >  	FILE *file;
> >  	int ret;
> > @@ -2167,7 +2158,7 @@ static int setup_mount_entries(const struct lxc_rootfs *rootfs, struct lxc_list
> >  	if (!file)
> >  		return -1;
> >  
> > -	ret = mount_file_entries(rootfs, file, lxc_name);
> > +	ret = mount_file_entries(rootfs, file, lxc_name, lxc_path);
> >  
> >  	fclose(file);
> >  	return ret;
> > @@ -3891,12 +3882,12 @@ int lxc_setup(struct lxc_handler *handler)
> >  		return -1;
> >  	}
> >  
> > -	if (setup_mount(&lxc_conf->rootfs, lxc_conf->fstab, name)) {
> > +	if (setup_mount(&lxc_conf->rootfs, lxc_conf->fstab, name, lxcpath)) {
> >  		ERROR("failed to setup the mounts for '%s'", name);
> >  		return -1;
> >  	}
> >  
> > -	if (!lxc_list_empty(&lxc_conf->mount_list) && setup_mount_entries(&lxc_conf->rootfs, &lxc_conf->mount_list, name)) {
> > +	if (!lxc_list_empty(&lxc_conf->mount_list) && setup_mount_entries(&lxc_conf->rootfs, &lxc_conf->mount_list, name, lxcpath)) {
> >  		ERROR("failed to setup the mount entries for '%s'", name);
> >  		return -1;
> >  	}
> > -- 
> > 2.6.1
> > 
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel


More information about the lxc-devel mailing list