Re: [lxc-devel] (Mount) namespaces cleanup

[Wolfgang Bumiller]

On Fri, Sep 04, 2015 at 06:09:36PM +, Serge Hallyn wrote:
> > I'm assuming the cleanup is left to the kernel for when the last
> > reference to the namespace disappears. However, this can be
> 
> Yes.
> 
> > problematic in some cases. For instance with an NFS mount, which can
> > apparently hang indefinitely.
> > 
> > So I'd like to know what the recommended procedure there is. One thing
> > that came to mind is adding a `cleanup` hook. Currently there's only a
> > post-stop hook which is run in the host's namespace after stopping. A
> > cleanup hook would ideally be run after stopping the processes in the
> > container but *inside* the container's namespaces. (Maybe also a
> > pre-stop hook to be run inside the container before killing the
> > processes?)
> 
> cleanup hook could be useful.  What exactly would you do there to solve
> your problem?

The idea is to manually unmount the storages we want to manage from the
outside, so that if an unmount hangs we actually notice it (because the
unmount-process would hang with it, and we'd see that as we'd be waiting
for it to finish.)

Dead mounts aren't the only issue though, timing is, too. Here's an
example: We have two nodes in a cluster and want to migrate a container
from one to the other. So we stop the container, now let's assume the
data is on shared storage, so we (up until now) assumed we can simply
start it on the other node, however, there's no guarantee that we can
already do this: like if the network filesystem is slow, and is thus
still in the process of syncing, we'd be trying to mount the data on
node B before node A is finished syncing.

In the cleanup hook we'd basically just wait for the mounts, and thus
either delay the start of the container on the other node, or throw an
error after a time out and prevent the start on the other node.

The other idea we had would be to keep the filesystems visible on the
host. That way the host can make sure they're unmounted after the
container is stopped. Since the container AFAIK is an MS_SLAVE mount
this would propagate to the container if it's still alive for some other
reason.

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

Re: [lxc-devel] (Mount) namespaces cleanup

[Serge Hallyn]

Quoting Wolfgang Bumiller (w.bumil...@proxmox.com):
> On Fri, Sep 04, 2015 at 06:09:36PM +, Serge Hallyn wrote:
> > > I'm assuming the cleanup is left to the kernel for when the last
> > > reference to the namespace disappears. However, this can be
> > 
> > Yes.
> > 
> > > problematic in some cases. For instance with an NFS mount, which can
> > > apparently hang indefinitely.
> > > 
> > > So I'd like to know what the recommended procedure there is. One thing
> > > that came to mind is adding a `cleanup` hook. Currently there's only a
> > > post-stop hook which is run in the host's namespace after stopping. A
> > > cleanup hook would ideally be run after stopping the processes in the
> > > container but *inside* the container's namespaces. (Maybe also a
> > > pre-stop hook to be run inside the container before killing the
> > > processes?)
> > 
> > cleanup hook could be useful.  What exactly would you do there to solve
> > your problem?
> 
> The idea is to manually unmount the storages we want to manage from the
> outside, so that if an unmount hangs we actually notice it (because the
> unmount-process would hang with it, and we'd see that as we'd be waiting
> for it to finish.)
> 
> Dead mounts aren't the only issue though, timing is, too. Here's an
> example: We have two nodes in a cluster and want to migrate a container
> from one to the other. So we stop the container, now let's assume the
> data is on shared storage, so we (up until now) assumed we can simply
> start it on the other node, however, there's no guarantee that we can
> already do this: like if the network filesystem is slow, and is thus
> still in the process of syncing, we'd be trying to mount the data on
> node B before node A is finished syncing.
> 
> In the cleanup hook we'd basically just wait for the mounts, and thus
> either delay the start of the container on the other node, or throw an
> error after a time out and prevent the start on the other node.
> 
> The other idea we had would be to keep the filesystems visible on the
> host. That way the host can make sure they're unmounted after the
> container is stopped. Since the container AFAIK is an MS_SLAVE mount
> this would propagate to the container if it's still alive for some other
> reason.

The container can easily undo the MS_SLAVE, so while this may be good
enough to be useful, it's not bullet-proof.
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] conf.c:instantiate_veth() rewrite

[Serge Hallyn]

Quoting Stephane Nguyen (stephm...@yahoo.es):
> Hi
> 
> It looks like conf.c:instanciate_veth() was rewritten; the result is
> that lxc-start is failing when setting up the veth pair MTUs. I rewrote
> a very small section of the code and it seems to be working fine now:
> 
> 1) Move the following block (unchanged) before the block that's
> getting/setting the MTU
> < netdev->ifindex = if_nametoindex(veth2);
> < if (!netdev->ifindex) {
> < ERROR("failed to retrieve the index for %s", veth2);
> < goto out_delete;
> 
> 2) Replace
> < mtu = netdev_get_mtu(if_nametoindex(netdev->link));
> 
> by
> 
> > mtu = netdev_get_mtu(netdev->ifindex);
> 
> The whole code diff is as follows:
> 
> # diff lxc-master/src/lxc/conf.c lxc-steph/src/lxc/conf.c
> 2602a2603,2608
> > netdev->ifindex = if_nametoindex(veth2);
> > if (!netdev->ifindex) {
> > ERROR("failed to retrieve the index for %s", veth2);
> > goto out_delete;
> > }
> > 
> 2606c2612
> < mtu = netdev_get_mtu(if_nametoindex(netdev->link));
> ---
> > mtu = netdev_get_mtu(netdev->ifindex);
> 2627,2632d2632
> < }
> < 
> < netdev->ifindex = if_nametoindex(veth2);
> < if (!netdev->ifindex) {
> < ERROR("failed to retrieve the index for %s", veth2);
> < goto out_delete;
> 
> 
> Was there a reason why the interface index was searched after the MTU
> get/set which actually depends on the interface index?

Could you send this as a signed-off patch or a github pull request?
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 2/6] Delete string from array Add function to delete a string from a non-null terminated buffer

[Serge Hallyn]

Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> 
> I'm probably wrong, but
> 
> 1. if the buffer is non-null-terminated, then can't the memmove of
>   strlen(del) - needlelen + 1 end up in a segv?
> 2. the comment should probably mention newlines.  If needle doesn't
>   include newline, and the string is a policy, we'll end up
>   with extra newlines.  Which may not matter, unless we do
>   thousands of operations on a container...

In fact, if it is not null-terminated, you can't do the strlen(del)
safely, can you?

> > Signed-off-by: Christian Brauner 
> > 
> >  100.0% src/lxc/
> > diff --git a/src/lxc/utils.c b/src/lxc/utils.c
> > index 7ced314..5940542 100644
> > --- a/src/lxc/utils.c
> > +++ b/src/lxc/utils.c
> > @@ -1466,3 +1466,17 @@ err:
> > close(fd);
> > return ret;
> >  }
> > +
> > +bool lxc_delete_string_in_array(char *haystack, size_t haystacklen,
> > +   const char *needle, size_t needlelen)
> > +{
> > +   char *del = NULL;
> > +   bool bret = false;
> > +
> > +   if ((del = memmem(haystack, haystacklen, needle, needlelen))) {
> > +   memmove(del, del + needlelen, strlen(del) - needlelen + 1);
> > +   bret = true;
> > +   }
> > +   return bret;
> > +}
> > +
> > diff --git a/src/lxc/utils.h b/src/lxc/utils.h
> > index ee12dde..715d125 100644
> > --- a/src/lxc/utils.h
> > +++ b/src/lxc/utils.h
> > @@ -243,6 +243,9 @@ extern char *lxc_append_paths(const char *first, const 
> > char *second);
> >  extern bool lxc_string_in_list(const char *needle, const char *haystack, 
> > char sep);
> >  extern char **lxc_string_split(const char *string, char sep);
> >  extern char **lxc_string_split_and_trim(const char *string, char sep);
> > +/* Delete a string from a non-null terminated buffer. */
> > +bool lxc_delete_string_in_array(char *haystack, size_t haystacklen,
> > +   const char *needle, size_t needlelen);
> >  
> >  /* some simple array manipulation utilities */
> >  typedef void (*lxc_free_fn)(void *);
> > -- 
> > 2.5.1
> > 
> ___
> 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 4/6] Add bdev_destroy() to bdev.c and bdev.h static do_bdev_destroy() in lxccontainer.c becomes public bdev_destroy()

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> Signed-off-by: Christian Brauner 
> 
>  100.0% src/lxc/
> diff --git a/src/lxc/bdev.c b/src/lxc/bdev.c
> index ada3958..475d878 100644
> --- a/src/lxc/bdev.c
> +++ b/src/lxc/bdev.c
> @@ -3614,3 +3614,21 @@ bool rootfs_is_blockdev(struct lxc_conf *conf)
>   return true;
>   return false;
>  }
> +
> +bool bdev_destroy(struct lxc_conf *conf)
> +{
> + struct bdev *r;
> + bool ret = false;
> +
> + r = bdev_init(conf, conf->rootfs.path, conf->rootfs.mount, NULL);
> + if (!r)
> + return ret;
> +
> + if (r->ops->destroy(r) < 0)

You need to bdev_put here.

> + return ret;
> + bdev_put(r);
> +
> + ret = true;
> + return ret;
> +}
> +
> diff --git a/src/lxc/bdev.h b/src/lxc/bdev.h
> index 428b3b7..b034bcb 100644
> --- a/src/lxc/bdev.h
> +++ b/src/lxc/bdev.h
> @@ -123,6 +123,7 @@ struct bdev *bdev_copy(struct lxc_container *c0, const 
> char *cname,
>  struct bdev *bdev_create(const char *dest, const char *type,
>   const char *cname, struct bdev_specs *specs);
>  void bdev_put(struct bdev *bdev);
> +bool bdev_destroy(struct lxc_conf *conf);
>  
>  /*
>   * these are really for qemu-nbd support, as container shutdown
> -- 
> 2.5.1
> 
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 6/6] Enable lxc_fini() to destroy container on shutdown This works for any bdev-type but is only used for overlayfs and aufs now

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> Now we can e.g. implement ephemeral containers in a consistent way.
> 
> Signed-off-by: Christian Brauner 
> 
>  100.0% src/lxc/
> diff --git a/src/lxc/start.c b/src/lxc/start.c
> index ffb8d12..1179d2c 100644
> --- a/src/lxc/start.c
> +++ b/src/lxc/start.c
> @@ -83,6 +83,11 @@ const struct ns_info ns_info[LXC_NS_MAX] = {
>   [LXC_NS_NET] = {"net", CLONE_NEWNET}
>  };
>  
> +static int bdev_destroy_wrapper(void *data);
> +static int lxc_rmdir_onedev_wrapper(void *data);
> +static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
> + const char *name);
> +
>  static void print_top_failing_dir(const char *path)
>  {
>   size_t len = strlen(path);
> @@ -495,6 +500,13 @@ void lxc_fini(const char *name, struct lxc_handler 
> *handler)
>   close(handler->ttysock[0]);
>   close(handler->ttysock[1]);
>   }
> + if (handler->conf->ephemeral > 0) {

This doesn't match what was advertised - < 0 should also mean
ephemeral.  (I really think a boolean makes the most sense,
enforced in the conffile as must be 0|1.)

> + /* Only destroy when rootfs is overlayfs or aufs for now. The
> +  * function however, works for other bdev types as well. */

What is the reason for that limitation?

> + if ((strncmp(handler->conf->rootfs.path, "overlayfs:", 10) == 
> 0) ||
> +(strncmp(handler->conf->rootfs.path, "aufs:", 5) == 0))
> + lxc_destroy_container_on_signal(handler, name);
> + }
>   cgroup_destroy(handler);
>   free(handler);
>  }
> @@ -1291,3 +1303,61 @@ int lxc_start(const char *name, char *const argv[], 
> struct lxc_conf *conf,
>   conf->need_utmp_watch = 1;
>   return __lxc_start(name, conf, _ops, _arg, lxcpath, 
> backgrounded);
>  }
> +
> +static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
> + const char *name)
> +{
> + char destroy[MAXPATHLEN];
> + bool bret = true;
> + int ret = 0;
> + if (handler->conf && handler->conf->rootfs.path && 
> handler->conf->rootfs.mount) {
> + if (am_unpriv())
> + ret = userns_exec_1(handler->conf, 
> bdev_destroy_wrapper, handler->conf);
> + else
> + bret = bdev_destroy(handler->conf);
> + if (ret < 0 || !bret) {
> + ERROR("Error destroying rootfs for %s", name);
> + } else {
> + INFO("Destroyed rootfs for %s", name);
> + }
> + }
> + ret = snprintf(destroy, MAXPATHLEN, "%s/%s", handler->lxcpath, name);
> + if (ret < 0 || ret >= MAXPATHLEN)
> + ERROR("Error creating string");
> + if (am_unpriv())
> + ret = userns_exec_1(handler->conf, lxc_rmdir_onedev_wrapper, 
> destroy);
> + else
> + ret = lxc_rmdir_onedev(destroy, NULL);
> + if (ret < 0) {
> + ERROR("Error destroying container directory for %s", name);
> + } else {
> + INFO("Destroyed directory for %s", name);
> + }
> +}
> +
> +static int bdev_destroy_wrapper(void *data)

The one in lxccontainers.c should somehow be shared.

> +{
> + struct lxc_conf *conf = data;
> +
> + if (setgid(0) < 0) {
> + ERROR("Failed to setgid to 0");
> + return -1;
> + }
> + if (setgroups(0, NULL) < 0)
> + WARN("Failed to clear groups");
> + if (setuid(0) < 0) {
> + ERROR("Failed to setuid to 0");
> + return -1;
> + }
> + if (!bdev_destroy(conf))
> + return -1;
> + else
> + return 0;
> +}
> +
> +static int lxc_rmdir_onedev_wrapper(void *data)

Hm, I guess it's easier just to keep this one static in 
the two places like you're doing.

> +{
> + char *arg = (char *) data;
> + return lxc_rmdir_onedev(arg, NULL);
> +}
> +
> -- 
> 2.5.1
> 
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] Do not use strlen() on non-null terminated buffer

[Christian Brauner]

On Tue, Sep 08, 2015 at 02:36:20AM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > Signed-off-by: Christian Brauner 
> 
> Thanks, this looks good, but I'd like to give it another
> look with a fresh pair of eyes in the morning.
> 
> (and maybe write a testcase)

Thanks, Serge! If this solution does not satisfy in the end I have another
solution available which I coded before deciding for the mmap()-based solution:
Using read() + calloc() to read in the whole file, then removing the line from
the buffer using strstr() and writing the buffer back to the file with write().
In this case we wouldn't need to fiddle with non-null terminated buffers.

Christian

> 
> > ---
> >  src/lxc/lxccontainer.c | 31 +--
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > index 932d658..fb99892 100644
> > --- a/src/lxc/lxccontainer.c
> > +++ b/src/lxc/lxccontainer.c
> > @@ -1989,7 +1989,7 @@ static bool mod_rdep(struct lxc_container *c0, struct 
> > lxc_container *c, bool inc
> > char newpath[MAXPATHLEN];
> > int fd, ret, n = 0, v = 0;
> > bool bret = false;
> > -   size_t len;
> > +   size_t len, difflen;
> >  
> > if (container_disk_lock(c0))
> > return false;
> > @@ -2072,19 +2072,22 @@ static bool mod_rdep(struct lxc_container *c0, 
> > struct lxc_container *c, bool inc
> >  
> > /* mmap()ed memory is only \0-terminated when it is not
> >  * a multiple of a pagesize. Hence, we'll use memmem(). 
> > */
> > -   if ((del = memmem(buf, fbuf.st_size, newpath, len))) {
> > -   /* remove container entry */
> > -   memmove(del, del + len, strlen(del) - len + 1);
> > -
> > -   munmap(buf, fbuf.st_size);
> > -
> > -   if (ftruncate(fd, fbuf.st_size - len) < 0) {
> > -   SYSERROR("Failed to truncate file %s", 
> > path);
> > -   close(fd);
> > -   goto out;
> > -   }
> > -   } else {
> > -   munmap(buf, fbuf.st_size);
> > +if ((del = memmem(buf, fbuf.st_size, newpath, 
> > len))) {
> > +/* remove container entry */
> > +if (del != buf + fbuf.st_size - len) {
> > +difflen = fbuf.st_size - (del-buf);
> > +memmove(del, del + len, 
> > strnlen(del, difflen) - len);
> > +}
> > +
> > +munmap(buf, fbuf.st_size);
> > +
> > +if (ftruncate(fd, fbuf.st_size - len) < 0) 
> > {
> > +SYSERROR("Failed to truncate file 
> > %s", path);
> > +close(fd);
> > +goto out;
> > +}
> > +} else {
> > +munmap(buf, fbuf.st_size);
> > }
> >  
> > close(fd);
> > -- 
> > 2.5.1
> > 


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 5/6] Destroy bdevs using new bdev_destroy() from bdev.h

[Christian Brauner]

On Mon, Sep 07, 2015 at 05:10:27PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > Signed-off-by: Christian Brauner 
> > 
> >  100.0% src/lxc/
> > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > index 2103437..9f22fdc 100644
> > --- a/src/lxc/lxccontainer.c
> > +++ b/src/lxc/lxccontainer.c
> > @@ -2213,21 +2213,6 @@ static int lxc_rmdir_onedev_wrapper(void *data)
> > return lxc_rmdir_onedev(arg, "snaps");
> >  }
> >  
> > -static int do_bdev_destroy(struct lxc_conf *conf)
> > -{
> > -   struct bdev *r;
> > -   int ret = 0;
> > -
> > -   r = bdev_init(conf, conf->rootfs.path, conf->rootfs.mount, NULL);
> > -   if (!r)
> > -   return -1;
> > -
> > -   if (r->ops->destroy(r) < 0)
> > -   ret = -1;
> > -   bdev_put(r);
> > -   return ret;
> > -}
> > -
> >  static int bdev_destroy_wrapper(void *data)
> >  {
> > struct lxc_conf *conf = data;
> > @@ -2242,13 +2227,16 @@ static int bdev_destroy_wrapper(void *data)
> > ERROR("Failed to setuid to 0");
> > return -1;
> > }
> > -   return do_bdev_destroy(conf);
> > +   if (!bdev_destroy(conf))
> > +   return -1;
> > +   else
> > +   return 0;
> >  }
> >  
> >  static bool container_destroy(struct lxc_container *c)
> >  {
> > bool bret = false;
> > -   int ret;
> > +   int ret = 0;
> > struct lxc_conf *conf;
> >  
> > if (!c || !do_lxcapi_is_defined(c))
> > @@ -2304,8 +2292,8 @@ static bool container_destroy(struct lxc_container *c)
> > if (am_unpriv())
> > ret = userns_exec_1(conf, bdev_destroy_wrapper, conf);
> > else
> > -   ret = do_bdev_destroy(conf);
> > -   if (ret < 0) {
> > +   bret = bdev_destroy(conf);
> > +   if (ret < 0 || !bret) {
> 
> I don't see how this can work - if the container is unprivileged, bret
> will remain false as initialized, so an error will always be raised.

This has been fixed.

> 
> I think it's better to have a single return value from this if/else
> block.  You could just do
> 
>   bret = userns_exec_1(conf, bdev_destroy_wrapper, conf) ? 0 : -1;

Yes, this is the solution we should use. But I think you meant:

ret = bdev_destroy(conf) ? 0 : -1;

> 
> but it's probably nicer to just add a short static wrapper function
> returning bool, something like

Not possible, since userns_exec_1() expects a callback function pointer of type
int (*fn)(void *)

> 
> static bool do_destroy_container(struct lxc_container *conf) {
>   if (am_unpriv()) {
>   if (userns_exec_1(conf, bdev_destroy_wrapper, conf) < 0)
>   return false;
>   return true;
>   }
>   return bdev_destroy(conf);
> }
> 
> > ERROR("Error destroying rootfs for %s", c->name);
> > goto out;
> > }
> > -- 
> > 2.5.1
> > 


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] Do not use strlen() on non-null terminated buffer

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> Signed-off-by: Christian Brauner 

Thanks, this looks good, but I'd like to give it another
look with a fresh pair of eyes in the morning.

(and maybe write a testcase)

> ---
>  src/lxc/lxccontainer.c | 31 +--
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index 932d658..fb99892 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -1989,7 +1989,7 @@ static bool mod_rdep(struct lxc_container *c0, struct 
> lxc_container *c, bool inc
>   char newpath[MAXPATHLEN];
>   int fd, ret, n = 0, v = 0;
>   bool bret = false;
> - size_t len;
> + size_t len, difflen;
>  
>   if (container_disk_lock(c0))
>   return false;
> @@ -2072,19 +2072,22 @@ static bool mod_rdep(struct lxc_container *c0, struct 
> lxc_container *c, bool inc
>  
>   /* mmap()ed memory is only \0-terminated when it is not
>* a multiple of a pagesize. Hence, we'll use memmem(). 
> */
> - if ((del = memmem(buf, fbuf.st_size, newpath, len))) {
> - /* remove container entry */
> - memmove(del, del + len, strlen(del) - len + 1);
> -
> - munmap(buf, fbuf.st_size);
> -
> - if (ftruncate(fd, fbuf.st_size - len) < 0) {
> - SYSERROR("Failed to truncate file %s", 
> path);
> - close(fd);
> - goto out;
> - }
> - } else {
> - munmap(buf, fbuf.st_size);
> +if ((del = memmem(buf, fbuf.st_size, newpath, len))) 
> {
> +/* remove container entry */
> +if (del != buf + fbuf.st_size - len) {
> +difflen = fbuf.st_size - (del-buf);
> +memmove(del, del + len, strnlen(del, 
> difflen) - len);
> +}
> +
> +munmap(buf, fbuf.st_size);
> +
> +if (ftruncate(fd, fbuf.st_size - len) < 0) {
> +SYSERROR("Failed to truncate file 
> %s", path);
> +close(fd);
> +goto out;
> +}
> +} else {
> +munmap(buf, fbuf.st_size);
>   }
>  
>   close(fd);
> -- 
> 2.5.1
> 
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 6/6] Enable lxc_fini() to destroy container on shutdown This works for any bdev-type but is only used for overlayfs and aufs now

[Christian Brauner]

On Mon, Sep 07, 2015 at 05:25:03PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > Now we can e.g. implement ephemeral containers in a consistent way.
> > 
> > Signed-off-by: Christian Brauner 
> > 
> >  100.0% src/lxc/
> > diff --git a/src/lxc/start.c b/src/lxc/start.c
> > index ffb8d12..1179d2c 100644
> > --- a/src/lxc/start.c
> > +++ b/src/lxc/start.c
> > @@ -83,6 +83,11 @@ const struct ns_info ns_info[LXC_NS_MAX] = {
> > [LXC_NS_NET] = {"net", CLONE_NEWNET}
> >  };
> >  
> > +static int bdev_destroy_wrapper(void *data);
> > +static int lxc_rmdir_onedev_wrapper(void *data);
> > +static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
> > +   const char *name);
> > +
> >  static void print_top_failing_dir(const char *path)
> >  {
> > size_t len = strlen(path);
> > @@ -495,6 +500,13 @@ void lxc_fini(const char *name, struct lxc_handler 
> > *handler)
> > close(handler->ttysock[0]);
> > close(handler->ttysock[1]);
> > }
> > +   if (handler->conf->ephemeral > 0) {
> 
> This doesn't match what was advertised - < 0 should also mean
> ephemeral.  (I really think a boolean makes the most sense,
> enforced in the conffile as must be 0|1.)

Ok.

> 
> > +   /* Only destroy when rootfs is overlayfs or aufs for now. The
> > +* function however, works for other bdev types as well. */
> 
> What is the reason for that limitation?

So far we only seem to use it for overlayfs and aufs based containers and I
didn't want to be hasty making it usable for any kind of container.

> 
> > +   if ((strncmp(handler->conf->rootfs.path, "overlayfs:", 10) == 
> > 0) ||
> > +  (strncmp(handler->conf->rootfs.path, "aufs:", 5) == 0))
> > +   lxc_destroy_container_on_signal(handler, name);
> > +   }
> > cgroup_destroy(handler);
> > free(handler);
> >  }
> > @@ -1291,3 +1303,61 @@ int lxc_start(const char *name, char *const argv[], 
> > struct lxc_conf *conf,
> > conf->need_utmp_watch = 1;
> > return __lxc_start(name, conf, _ops, _arg, lxcpath, 
> > backgrounded);
> >  }
> > +
> > +static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
> > +   const char *name)
> > +{
> > +   char destroy[MAXPATHLEN];
> > +   bool bret = true;
> > +   int ret = 0;
> > +   if (handler->conf && handler->conf->rootfs.path && 
> > handler->conf->rootfs.mount) {
> > +   if (am_unpriv())
> > +   ret = userns_exec_1(handler->conf, 
> > bdev_destroy_wrapper, handler->conf);
> > +   else
> > +   bret = bdev_destroy(handler->conf);
> > +   if (ret < 0 || !bret) {
> > +   ERROR("Error destroying rootfs for %s", name);
> > +   } else {
> > +   INFO("Destroyed rootfs for %s", name);
> > +   }
> > +   }
> > +   ret = snprintf(destroy, MAXPATHLEN, "%s/%s", handler->lxcpath, name);
> > +   if (ret < 0 || ret >= MAXPATHLEN)
> > +   ERROR("Error creating string");
> > +   if (am_unpriv())
> > +   ret = userns_exec_1(handler->conf, lxc_rmdir_onedev_wrapper, 
> > destroy);
> > +   else
> > +   ret = lxc_rmdir_onedev(destroy, NULL);
> > +   if (ret < 0) {
> > +   ERROR("Error destroying container directory for %s", name);
> > +   } else {
> > +   INFO("Destroyed directory for %s", name);
> > +   }
> > +}
> > +
> > +static int bdev_destroy_wrapper(void *data)
> 
> The one in lxccontainers.c should somehow be shared.
We can put it in bdev.c and bdev.h

> 
> > +{
> > +   struct lxc_conf *conf = data;
> > +
> > +   if (setgid(0) < 0) {
> > +   ERROR("Failed to setgid to 0");
> > +   return -1;
> > +   }
> > +   if (setgroups(0, NULL) < 0)
> > +   WARN("Failed to clear groups");
> > +   if (setuid(0) < 0) {
> > +   ERROR("Failed to setuid to 0");
> > +   return -1;
> > +   }
> > +   if (!bdev_destroy(conf))
> > +   return -1;
> > +   else
> > +   return 0;
> > +}
> > +
> > +static int lxc_rmdir_onedev_wrapper(void *data)
> 
> Hm, I guess it's easier just to keep this one static in 
> the two places like you're doing.

Ok.

> 
> > +{
> > +   char *arg = (char *) data;
> > +   return lxc_rmdir_onedev(arg, NULL);
> > +}
> > +
> > -- 
> > 2.5.1
> > 


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 5/6] Destroy bdevs using new bdev_destroy() from bdev.h

[Christian Brauner]

On Mon, Sep 07, 2015 at 05:10:27PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > Signed-off-by: Christian Brauner 
> > 
> >  100.0% src/lxc/
> > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > index 2103437..9f22fdc 100644
> > --- a/src/lxc/lxccontainer.c
> > +++ b/src/lxc/lxccontainer.c
> > @@ -2213,21 +2213,6 @@ static int lxc_rmdir_onedev_wrapper(void *data)
> > return lxc_rmdir_onedev(arg, "snaps");
> >  }
> >  
> > -static int do_bdev_destroy(struct lxc_conf *conf)
> > -{
> > -   struct bdev *r;
> > -   int ret = 0;
> > -
> > -   r = bdev_init(conf, conf->rootfs.path, conf->rootfs.mount, NULL);
> > -   if (!r)
> > -   return -1;
> > -
> > -   if (r->ops->destroy(r) < 0)
> > -   ret = -1;
> > -   bdev_put(r);
> > -   return ret;
> > -}
> > -
> >  static int bdev_destroy_wrapper(void *data)
> >  {
> > struct lxc_conf *conf = data;
> > @@ -2242,13 +2227,16 @@ static int bdev_destroy_wrapper(void *data)
> > ERROR("Failed to setuid to 0");
> > return -1;
> > }
> > -   return do_bdev_destroy(conf);
> > +   if (!bdev_destroy(conf))
> > +   return -1;
> > +   else
> > +   return 0;
> >  }
> >  
> >  static bool container_destroy(struct lxc_container *c)
> >  {
> > bool bret = false;
> > -   int ret;
> > +   int ret = 0;
> > struct lxc_conf *conf;
> >  
> > if (!c || !do_lxcapi_is_defined(c))
> > @@ -2304,8 +2292,8 @@ static bool container_destroy(struct lxc_container *c)
> > if (am_unpriv())
> > ret = userns_exec_1(conf, bdev_destroy_wrapper, conf);
> > else
> > -   ret = do_bdev_destroy(conf);
> > -   if (ret < 0) {
> > +   bret = bdev_destroy(conf);
> > +   if (ret < 0 || !bret) {
> 
> I don't see how this can work - if the container is unprivileged, bret
> will remain false as initialized, so an error will always be raised.
> 
> I think it's better to have a single return value from this if/else
> block.  You could just do
> 
>   bret = userns_exec_1(conf, bdev_destroy_wrapper, conf) ? 0 : -1;
> 
> but it's probably nicer to just add a short static wrapper function
> returning bool, something like
> 
> static bool do_destroy_container(struct lxc_container *conf) {
>   if (am_unpriv()) {
>   if (userns_exec_1(conf, bdev_destroy_wrapper, conf) < 0)
>   return false;
>   return true;
>   }
>   return bdev_destroy(conf);
Sorry, I get it. Will do!

> }
> 
> > ERROR("Error destroying rootfs for %s", c->name);
> > goto out;
> > }
> > -- 
> > 2.5.1
> > 


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 6/6] Enable lxc_fini() to destroy container on shutdown This works for any bdev-type but is only used for overlayfs and aufs now

[Christian Brauner]

On Mon, Sep 07, 2015 at 05:13:56PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > Fixes for the return value checks.
> > 
> > On Sun, Sep 06, 2015 at 10:38:21AM +0200, Christian Brauner wrote:
> > > Now we can e.g. implement ephemeral containers in a consistent way.
> > > 
> > > Signed-off-by: Christian Brauner 
> > > 
> > >  100.0% src/lxc/
> > > diff --git a/src/lxc/start.c b/src/lxc/start.c
> > > index ffb8d12..1179d2c 100644
> > > --- a/src/lxc/start.c
> > > +++ b/src/lxc/start.c
> > > @@ -83,6 +83,11 @@ const struct ns_info ns_info[LXC_NS_MAX] = {
> > >   [LXC_NS_NET] = {"net", CLONE_NEWNET}
> > >  };
> > >  
> > > +static int bdev_destroy_wrapper(void *data);
> > > +static int lxc_rmdir_onedev_wrapper(void *data);
> > > +static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
> > > + const char *name);
> > > +
> > >  static void print_top_failing_dir(const char *path)
> > >  {
> > >   size_t len = strlen(path);
> > > @@ -495,6 +500,13 @@ void lxc_fini(const char *name, struct lxc_handler 
> > > *handler)
> > >   close(handler->ttysock[0]);
> > >   close(handler->ttysock[1]);
> > >   }
> > > + if (handler->conf->ephemeral > 0) {
> > > + /* Only destroy when rootfs is overlayfs or aufs for now. The
> > > +  * function however, works for other bdev types as well. */
> > > + if ((strncmp(handler->conf->rootfs.path, "overlayfs:", 10) == 
> > > 0) ||
> > > +(strncmp(handler->conf->rootfs.path, "aufs:", 5) == 0))
> > > + lxc_destroy_container_on_signal(handler, name);
> > > + }
> > >   cgroup_destroy(handler);
> > >   free(handler);
> > >  }
> > > @@ -1291,3 +1303,61 @@ int lxc_start(const char *name, char *const 
> > > argv[], struct lxc_conf *conf,
> > >   conf->need_utmp_watch = 1;
> > >   return __lxc_start(name, conf, _ops, _arg, lxcpath, 
> > > backgrounded);
> > >  }
> > > +
> > > +static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
> > > + const char *name)
> > > +{
> > > + char destroy[MAXPATHLEN];
> > > + bool bret = true;
> > > + int ret = 0;
> > > + if (handler->conf && handler->conf->rootfs.path && 
> > > handler->conf->rootfs.mount) {
> > > + if (am_unpriv())
> > > + ret = userns_exec_1(handler->conf, 
> > > bdev_destroy_wrapper, handler->conf);
> > > + else
> > > + bret = bdev_destroy(handler->conf);
> > > + if (ret < 0 || !bret) {
> > > + ERROR("Error destroying rootfs for %s", name);
> > > + } else {
> > > + INFO("Destroyed rootfs for %s", name);
> > > + }
> > > + }
> > 
> > The check for the return value here is wrong but it is fixed in the PR on 
> > github.
> > 
> > > + ret = snprintf(destroy, MAXPATHLEN, "%s/%s", handler->lxcpath, name);
> > > + if (ret < 0 || ret >= MAXPATHLEN)
> > > + ERROR("Error creating string");
> > > + if (am_unpriv())
> > > + ret = userns_exec_1(handler->conf, lxc_rmdir_onedev_wrapper, 
> > > destroy);
> > > + else
> > > + ret = lxc_rmdir_onedev(destroy, NULL);
> > > + if (ret < 0) {
> > > + ERROR("Error destroying container directory for %s", name);
> > > + } else {
> > > + INFO("Destroyed directory for %s", name);
> > > + }
> > > +}
> > > +
> > > +static int bdev_destroy_wrapper(void *data)
> > > +{
> > > + struct lxc_conf *conf = data;
> > > +
> > > + if (setgid(0) < 0) {
> > > + ERROR("Failed to setgid to 0");
> > > + return -1;
> > > + }
> > > + if (setgroups(0, NULL) < 0)
> > > + WARN("Failed to clear groups");
> > > + if (setuid(0) < 0) {
> > > + ERROR("Failed to setuid to 0");
> > > + return -1;
> > > + }
> > > + if (!bdev_destroy(conf))
> > > + return -1;
> > > + else
> > > + return 0;
> > > +}
> > > +
> > > +static int lxc_rmdir_onedev_wrapper(void *data)
> > > +{
> > > + char *arg = (char *) data;
> > > + return lxc_rmdir_onedev(arg, NULL);
> > > +}
> > > +
> > > -- 
> > > 2.5.1
> > > 
> > 
> > On Sun, Sep 06, 2015 at 10:38:20AM +0200, Christian Brauner wrote:
> > > Signed-off-by: Christian Brauner 
> > > 
> > >  100.0% src/lxc/
> > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > > index 2103437..9f22fdc 100644
> > > --- a/src/lxc/lxccontainer.c
> > > +++ b/src/lxc/lxccontainer.c
> > > @@ -2213,21 +2213,6 @@ static int lxc_rmdir_onedev_wrapper(void *data)
> > >   return lxc_rmdir_onedev(arg, "snaps");
> > >  }
> > >  
> > > -static int do_bdev_destroy(struct lxc_conf *conf)
> > > -{
> > > - struct bdev *r;
> > > - int ret = 0;
> > > -
> > > - r = bdev_init(conf, conf->rootfs.path, conf->rootfs.mount, NULL);
> > > - if (!r)
> > > - return -1;
> > > -
> > > - if (r->ops->destroy(r) < 0)
> > > - ret = -1;
> > > - bdev_put(r);
> > > - return 

Re: [lxc-devel] [PATCH] Fix strlen on non-null terminated buffer strlen() becomes strnlen()

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> On Mon, Sep 07, 2015 at 07:50:10PM +, Serge Hallyn wrote:
> > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > Sorry, forget it, that doesn't make sense...
> > > 
> > > On Mon, Sep 07, 2015 at 08:38:51PM +0200, Christian Brauner wrote:
> > > > Signed-off-by: Christian Brauner 
> > > > ---
> > > >  src/lxc/lxccontainer.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > > > index 932d658..ae9f895 100644
> > > > --- a/src/lxc/lxccontainer.c
> > > > +++ b/src/lxc/lxccontainer.c
> > > > @@ -2074,7 +2074,7 @@ static bool mod_rdep(struct lxc_container *c0, 
> > > > struct lxc_container *c, bool inc
> > > >  * a multiple of a pagesize. Hence, we'll use 
> > > > memmem(). */
> > > > if ((del = memmem(buf, fbuf.st_size, newpath, 
> > > > len))) {
> > > > /* remove container entry */
> > > > -   memmove(del, del + len, strlen(del) - 
> > > > len + 1);
> > > > +   memmove(del, del + len, strnlen(del, 
> > > > fbuf.st_size) - len + 1);
> > 
> > strnlen can still go off the end here.  I think you want something like:
> > 
> > if (del != buf + fbuf.st_size - len) {

The point of this check was only to avoid the case where the needle was
the very last thing in the haystack, because in that case (a) all our work
is a noop and more importantly (b) appending the '\0' will fall off the
end.

> > size_t difflen = fbuf.st_size - 
> > (del-buf);
> > memmove(del, del + len, strnlen(del, 
> > difflen) - len);
> > del[len] = '\0';
> > }
> Thanks, what about:
> if ((del = memmem(buf, fbuf.st_size, newpath, 
> len))) {
>   size_t dellen = fbuf.st_size - len;
>   memmove(del, del + len, dellen - len);

The third arg to memmove should be the size of the memory area we want to move,
which is not this 'dellen'.  It's the original size minus the found offset
(del-buf) minus the size of the needle.  (If I'm thinking right)

>   }
> 
> What would you prefer?
> I don't know if we want to append '\0' or leave that to the caller?

Oh, yeah, in this function (mod_rdep) that's ok, in fact better.

If you turn it into a general helper than it isn't optional, because the \0 is
no longer optional after we make this change.  In fact we should then mremap()
the area in case there are multiple callers.
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] Do not use strlen() on non-null terminated buffer

[Christian Brauner]

Signed-off-by: Christian Brauner 
---
 src/lxc/lxccontainer.c | 31 +--
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 932d658..fb99892 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -1989,7 +1989,7 @@ static bool mod_rdep(struct lxc_container *c0, struct 
lxc_container *c, bool inc
char newpath[MAXPATHLEN];
int fd, ret, n = 0, v = 0;
bool bret = false;
-   size_t len;
+   size_t len, difflen;
 
if (container_disk_lock(c0))
return false;
@@ -2072,19 +2072,22 @@ static bool mod_rdep(struct lxc_container *c0, struct 
lxc_container *c, bool inc
 
/* mmap()ed memory is only \0-terminated when it is not
 * a multiple of a pagesize. Hence, we'll use memmem(). 
*/
-   if ((del = memmem(buf, fbuf.st_size, newpath, len))) {
-   /* remove container entry */
-   memmove(del, del + len, strlen(del) - len + 1);
-
-   munmap(buf, fbuf.st_size);
-
-   if (ftruncate(fd, fbuf.st_size - len) < 0) {
-   SYSERROR("Failed to truncate file %s", 
path);
-   close(fd);
-   goto out;
-   }
-   } else {
-   munmap(buf, fbuf.st_size);
+if ((del = memmem(buf, fbuf.st_size, newpath, len))) {
+/* remove container entry */
+if (del != buf + fbuf.st_size - len) {
+difflen = fbuf.st_size - (del-buf);
+memmove(del, del + len, strnlen(del, 
difflen) - len);
+}
+
+munmap(buf, fbuf.st_size);
+
+if (ftruncate(fd, fbuf.st_size - len) < 0) {
+SYSERROR("Failed to truncate file %s", 
path);
+close(fd);
+goto out;
+}
+} else {
+munmap(buf, fbuf.st_size);
}
 
close(fd);
-- 
2.5.1

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

[lxc-devel] [PATCH] Do not use strlen() on non-null terminated buffer

[Christian Brauner]

Serge, you should add a Suggested-by line if you want to.

Christian Brauner (1):
  Do not use strlen() on non-null terminated buffer

 src/lxc/lxccontainer.c | 31 +--
 1 file changed, 17 insertions(+), 14 deletions(-)

-- 
2.5.1

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

Re: [lxc-devel] [PATCH 2/6] Delete string from array Add function to delete a string from a non-null terminated buffer

[Christian Brauner]

On Mon, Sep 07, 2015 at 05:03:37PM +, Serge Hallyn wrote:
> Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > 
> > I'm probably wrong, but
> > 
> > 1. if the buffer is non-null-terminated, then can't the memmove of
> > strlen(del) - needlelen + 1 end up in a segv?
> > 2. the comment should probably mention newlines.  If needle doesn't
> > include newline, and the string is a policy, we'll end up
> > with extra newlines.  Which may not matter, unless we do
> > thousands of operations on a container...
> 
> In fact, if it is not null-terminated, you can't do the strlen(del)
> safely, can you?

Right, I would change
memmove(del, del + needlelen, strlen(del) - needlelen + 1);
to
memmove(del, del + needlelen, strnlen(del, haystacklen) - needlelen);

The Current master relies on the same idea in mod_rdep(), I'll send a patch for
the current master in a few minutes, unless you're on it right now.

Christian
> 
> > > Signed-off-by: Christian Brauner 
> > > 
> > >  100.0% src/lxc/
> > > diff --git a/src/lxc/utils.c b/src/lxc/utils.c
> > > index 7ced314..5940542 100644
> > > --- a/src/lxc/utils.c
> > > +++ b/src/lxc/utils.c
> > > @@ -1466,3 +1466,17 @@ err:
> > >   close(fd);
> > >   return ret;
> > >  }
> > > +
> > > +bool lxc_delete_string_in_array(char *haystack, size_t haystacklen,
> > > + const char *needle, size_t needlelen)
> > > +{
> > > + char *del = NULL;
> > > + bool bret = false;
> > > +
> > > + if ((del = memmem(haystack, haystacklen, needle, needlelen))) {
> > > + memmove(del, del + needlelen, strlen(del) - needlelen + 1);
> > > + bret = true;
> > > + }
> > > + return bret;
> > > +}
> > > +
> > > diff --git a/src/lxc/utils.h b/src/lxc/utils.h
> > > index ee12dde..715d125 100644
> > > --- a/src/lxc/utils.h
> > > +++ b/src/lxc/utils.h
> > > @@ -243,6 +243,9 @@ extern char *lxc_append_paths(const char *first, 
> > > const char *second);
> > >  extern bool lxc_string_in_list(const char *needle, const char *haystack, 
> > > char sep);
> > >  extern char **lxc_string_split(const char *string, char sep);
> > >  extern char **lxc_string_split_and_trim(const char *string, char sep);
> > > +/* Delete a string from a non-null terminated buffer. */
> > > +bool lxc_delete_string_in_array(char *haystack, size_t haystacklen,
> > > + const char *needle, size_t needlelen);
> > >  
> > >  /* some simple array manipulation utilities */
> > >  typedef void (*lxc_free_fn)(void *);
> > > -- 
> > > 2.5.1
> > > 
> > ___
> > lxc-devel mailing list
> > lxc-devel@lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel


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] Fix strlen on non-null terminated buffer strlen() becomes strnlen()

[Christian Brauner]

Sorry, forget it, that doesn't make sense...

On Mon, Sep 07, 2015 at 08:38:51PM +0200, Christian Brauner wrote:
> Signed-off-by: Christian Brauner 
> ---
>  src/lxc/lxccontainer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index 932d658..ae9f895 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -2074,7 +2074,7 @@ static bool mod_rdep(struct lxc_container *c0, struct 
> lxc_container *c, bool inc
>* a multiple of a pagesize. Hence, we'll use memmem(). 
> */
>   if ((del = memmem(buf, fbuf.st_size, newpath, len))) {
>   /* remove container entry */
> - memmove(del, del + len, strlen(del) - len + 1);
> + memmove(del, del + len, strnlen(del, 
> fbuf.st_size) - len + 1);
>  
>   munmap(buf, fbuf.st_size);
>  
> -- 
> 2.5.1
> 


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] Fix strlen on non-null terminated buffer strlen() becomes strnlen()

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> Sorry, forget it, that doesn't make sense...
> 
> On Mon, Sep 07, 2015 at 08:38:51PM +0200, Christian Brauner wrote:
> > Signed-off-by: Christian Brauner 
> > ---
> >  src/lxc/lxccontainer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > index 932d658..ae9f895 100644
> > --- a/src/lxc/lxccontainer.c
> > +++ b/src/lxc/lxccontainer.c
> > @@ -2074,7 +2074,7 @@ static bool mod_rdep(struct lxc_container *c0, struct 
> > lxc_container *c, bool inc
> >  * a multiple of a pagesize. Hence, we'll use memmem(). 
> > */
> > if ((del = memmem(buf, fbuf.st_size, newpath, len))) {
> > /* remove container entry */
> > -   memmove(del, del + len, strlen(del) - len + 1);
> > +   memmove(del, del + len, strnlen(del, 
> > fbuf.st_size) - len + 1);

strnlen can still go off the end here.  I think you want something like:

if (del != buf + fbuf.st_size - len) {
size_t difflen = fbuf.st_size - 
(del-buf);
memmove(del, del + len, strnlen(del, 
difflen) - len);
del[len] = '\0';
}


> >  
> > munmap(buf, fbuf.st_size);
> >  
> > -- 
> > 2.5.1
> > 


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

[lxc-devel] [PATCH] Fix strlen on non-null terminated buffer

[Christian Brauner]

strlen() was used a non-null terminated buffer. Use strnlen instead.

Christian Brauner (1):
  Fix strlen on non-null terminated buffer strlen() becomes
strnlen()

 src/lxc/lxccontainer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.5.1

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

Re: [lxc-devel] [PATCH 2/6] Delete string from array Add function to delete a string from a non-null terminated buffer

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> On Mon, Sep 07, 2015 at 05:03:37PM +, Serge Hallyn wrote:
> > Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > 
> > > I'm probably wrong, but
> > > 
> > > 1. if the buffer is non-null-terminated, then can't the memmove of
> > >   strlen(del) - needlelen + 1 end up in a segv?
> > > 2. the comment should probably mention newlines.  If needle doesn't
> > >   include newline, and the string is a policy, we'll end up
> > >   with extra newlines.  Which may not matter, unless we do
> > >   thousands of operations on a container...
> > 
> > In fact, if it is not null-terminated, you can't do the strlen(del)
> > safely, can you?
> 
> Right, I would change
> memmove(del, del + needlelen, strlen(del) - needlelen + 1);
> to
> memmove(del, del + needlelen, strnlen(del, haystacklen) - needlelen);

Should probably be somethin glike

 memmove(del, del + needlelen, strnlen(del, haystacklen-del) - 
needlelen);
 del[needlelen] = '\0';

I'm not convinced eventhat is right.  We may need to either always assume it is
null-terminated, or always track the current valid length of the string.  
(and/or
keep extra state showing whether null-terminated or not)

> The Current master relies on the same idea in mod_rdep(), I'll send a patch 
> for
> the current master in a few minutes, unless you're on it right now.

I'm not.  Thanks.
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel