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

[Christian Brauner]

On Oct 16, 2015 19:11, "Serge Hallyn"  wrote:
>
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > On Fri, Oct 16, 2015 at 03:52:04PM +, Serge Hallyn wrote:
> > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > On Thu, Oct 15, 2015 at 08:32:25PM +, Serge Hallyn wrote:
> > > > > Quoting Christian Brauner (christianvanbrau...@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 
> > > > >
> > > > > 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?
> > > >
> > >
> > > True that the current check is bogus.  But I think you just want to
> > > use rootfs->mount instead of rootfs->path.  By the time this code
> > > hits we have converted whatever target path the user gave us into
> > > concat(rootfs->mount, process(target)) where process(x) will take
> > > off a leading $lxcpath/$lxcname/rootfs or rootfs->path.
> > >
> > > -serge
> >
> > It's not bogus. It just misses the single case where the container
itself is an
> > overlay container. :)
>
> Or blockd-dev-backed, or any case where lxc.rootfs is in a nonstandard
> location - but that's ok,
>
Right, I always forget the block-devs...

> > Let's merge this patch as it is.
>
> Yeah, that's fine.
>
> @Stgraber - please do apply this :)
>
> > I will test whether rootfs->mount really is what we want and send a
follow-up
> > patch that is based off of this one. (I'll probably send it tomorrow.)
>
> Great, thanks.
> ___
> lxc-devel mailing list
> lxc-devel@lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

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

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> On Fri, Oct 16, 2015 at 03:52:04PM +, Serge Hallyn wrote:
> > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > On Thu, Oct 15, 2015 at 08:32:25PM +, Serge Hallyn wrote:
> > > > Quoting Christian Brauner (christianvanbrau...@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 
> > > > 
> > > > 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?
> > > 
> > 
> > True that the current check is bogus.  But I think you just want to
> > use rootfs->mount instead of rootfs->path.  By the time this code
> > hits we have converted whatever target path the user gave us into
> > concat(rootfs->mount, process(target)) where process(x) will take
> > off a leading $lxcpath/$lxcname/rootfs or rootfs->path.
> > 
> > -serge
> 
> It's not bogus. It just misses the single case where the container itself is 
> an
> overlay container. :)

Or blockd-dev-backed, or any case where lxc.rootfs is in a nonstandard
location - but that's ok, 

> Let's merge this patch as it is.

Yeah, that's fine.

@Stgraber - please do apply this :)

> I will test whether rootfs->mount really is what we want and send a follow-up
> patch that is based off of this one. (I'll probably send it tomorrow.)

Great, thanks.
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

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

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> On Fri, Oct 16, 2015 at 05:11:02PM +, Serge Hallyn wrote:
> > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > On Fri, Oct 16, 2015 at 03:52:04PM +, Serge Hallyn wrote:
> > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > On Thu, Oct 15, 2015 at 08:32:25PM +, Serge Hallyn wrote:
> > > > > > Quoting Christian Brauner (christianvanbrau...@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 
> > > > > > 
> > > > > > 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?
> > > > > 
> > > > 
> > > > True that the current check is bogus.  But I think you just want to
> > > > use rootfs->mount instead of rootfs->path.  By the time this code
> > > > hits we have converted whatever target path the user gave us into
> > > > concat(rootfs->mount, process(target)) where process(x) will take
> > > > off a leading $lxcpath/$lxcname/rootfs or rootfs->path.
> 
> Assume we have a cloned container. Then according to my testing
> 
> rootfs->path
> 
> contains
> 
> 
> overlayfs:/path/to/the/original/containers/rootfs:/path/to/the/cloned/containers/rootfs
> 
> but
> 
> rootfs->mount
> 
> contains
> 
>/usr/lib/lxc/rootfs
> 
> or whatever directory is used for pivot_dir. Hence, rootfs->mount won't help 
> us
> with the right hand side of the strncmp() check since we want to ensure that 
> no
> upperdir or workdir folders are created under
> 
> /path/to/the/containers/rootfs
> 
> I still think that we will simply need to parse rootfs->path and extract
> /path/to/the/cloned/containers/rootfs. Thoughts?

Oh yeah this is a src not a target.  You're right.
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

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

[Christian Brauner]

On Fri, Oct 16, 2015 at 05:11:02PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > On Fri, Oct 16, 2015 at 03:52:04PM +, Serge Hallyn wrote:
> > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > On Thu, Oct 15, 2015 at 08:32:25PM +, Serge Hallyn wrote:
> > > > > Quoting Christian Brauner (christianvanbrau...@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 
> > > > > 
> > > > > 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?
> > > > 
> > > 
> > > True that the current check is bogus.  But I think you just want to
> > > use rootfs->mount instead of rootfs->path.  By the time this code
> > > hits we have converted whatever target path the user gave us into
> > > concat(rootfs->mount, process(target)) where process(x) will take
> > > off a leading $lxcpath/$lxcname/rootfs or rootfs->path.

Assume we have a cloned container. Then according to my testing

rootfs->path

contains


overlayfs:/path/to/the/original/containers/rootfs:/path/to/the/cloned/containers/rootfs

but

rootfs->mount

contains

   /usr/lib/lxc/rootfs

or whatever directory is used for pivot_dir. Hence, rootfs->mount won't help us
with the right hand side of the strncmp() check since we want to ensure that no
upperdir or workdir folders are created under

/path/to/the/containers/rootfs

I still think that we will simply need to parse rootfs->path and extract
/path/to/the/cloned/containers/rootfs. Thoughts?


signature.asc
Description: PGP signature
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

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

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> On Thu, Oct 15, 2015 at 08:32:25PM +, Serge Hallyn wrote:
> > Quoting Christian Brauner (christianvanbrau...@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 
> > 
> > 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?
> 

True that the current check is bogus.  But I think you just want to
use rootfs->mount instead of rootfs->path.  By the time this code
hits we have converted whatever target path the user gave us into
concat(rootfs->mount, process(target)) where process(x) will take
off a leading $lxcpath/$lxcname/rootfs or rootfs->path.

-serge
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

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

[Christian Brauner]

On Fri, Oct 16, 2015 at 03:52:04PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > On Thu, Oct 15, 2015 at 08:32:25PM +, Serge Hallyn wrote:
> > > Quoting Christian Brauner (christianvanbrau...@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 
> > > 
> > > 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?
> > 
> 
> True that the current check is bogus.  But I think you just want to
> use rootfs->mount instead of rootfs->path.  By the time this code
> hits we have converted whatever target path the user gave us into
> concat(rootfs->mount, process(target)) where process(x) will take
> off a leading $lxcpath/$lxcname/rootfs or rootfs->path.
> 
> -serge

It's not bogus. It just misses the single case where the container itself is an
overlay container. :)

Let's merge this patch as it is.

I will test whether rootfs->mount really is what we want and send a follow-up
patch that is based off of this one. (I'll probably send it tomorrow.)


signature.asc
Description: PGP signature
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

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

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@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 

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.

> ---
>  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. */

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

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@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 

Acked-by: Serge E. Hallyn 

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