[lxc-devel] [PATCH] Ensure that mmap()ed memory is \0-terminated (v2) Use pwrite() to write terminating \0-byte

[Christian Brauner]

Updated patch to follow.

On Thu, Sep 10, 2015 at 11:20:44PM +0000, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrauner at gmail.com):
> > This allows us to use standard string handling functions and we can avoid using
> > the GNU-extension memmem(). This simplifies removing the container from the
> > lxc_snapshots file. Wrap strstr() in a while loop to remove duplicate entries.
> > 
> > Signed-off-by: Christian Brauner <christianvanbrauner at gmail.com>
> 
> looks good, one comment below.
> 
> > ---
> >  src/lxc/lxccontainer.c | 85 ++++++++++++++++++++++++++------------------------
> >  1 file changed, 44 insertions(+), 41 deletions(-)
> > 
> > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > index fb99892..7026719 100644
> > --- a/src/lxc/lxccontainer.c
> > +++ b/src/lxc/lxccontainer.c
> > @@ -1983,13 +1983,13 @@ static bool mod_rdep(struct lxc_container *c0, struct lxc_container *c, bool inc
> >  {
> >  	FILE *f1;
> >  	struct stat fbuf;
> > -	char *buf = NULL;
> > -	char *del;
> > +	void *buf = NULL;
> > +	char *del = NULL;
> >  	char path[MAXPATHLEN];
> >  	char newpath[MAXPATHLEN];
> > -	int fd, ret, n = 0, v = 0;
> > +	int fd, ret, n = 0, v = 0, ind = 0;
> >  	bool bret = false;
> > -	size_t len, difflen;
> > +	size_t len = 0, bytes = 0;
> >  
> >  	if (container_disk_lock(c0))
> >  		return false;
> > @@ -2049,55 +2049,58 @@ static bool mod_rdep(struct lxc_container *c0, struct lxc_container *c, bool inc
> >  				goto out;
> >  			}
> >  		} else if (!inc) {
> > -			fd = open(path, O_RDWR | O_CLOEXEC);
> > -			if (fd < 0)
> > -				goto out;
> > -
> > -			ret = fstat(fd, &fbuf);
> > -			if (ret < 0) {
> > -				close(fd);
> > -				goto out;
> > -			}
> > +                        if ((fd = open(path, O_RDWR | O_CLOEXEC)) < 0)
> > +                                goto out;
> >  
> > -			if (fbuf.st_size != 0) {
> > -				buf = mmap(NULL, fbuf.st_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> > -				if (buf == MAP_FAILED) {
> > -					SYSERROR("Failed to create mapping %s", path);
> > -					close(fd);
> > -					goto out;
> > -				}
> > -			}
> > +                        if (fstat(fd, &fbuf) < 0) {
> > +                                close(fd);
> > +                                goto out;
> > +                        }
> >  
> > -			len = strlen(newpath);
> > -
> > -			/* 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 */
> > -                                if (del != buf + fbuf.st_size - len) {
> > -                                        difflen = fbuf.st_size - (del-buf);
> > -                                        memmove(del, del + len, strnlen(del, difflen) - len);
> > +                        if (fbuf.st_size != 0) {
> > +                                /* write terminating \0-byte to file */
> > +                                if (pwrite(fd, "", 1, fbuf.st_size) <= 0) {
> > +                                        close(fd);
> > +                                        goto out;
> >                                  }
> >  
> > -                                munmap(buf, fbuf.st_size);
> > -
> > -                                if (ftruncate(fd, fbuf.st_size - len) < 0) {
> > -                                        SYSERROR("Failed to truncate file %s", path);
> > +                                buf = mmap(NULL, fbuf.st_size + 1, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> > +                                if (buf == MAP_FAILED) {
> > +                                        SYSERROR("Failed to create mapping %s", path);
> >                                          close(fd);
> >                                          goto out;
> >                                  }
> > -                        } else {
> > -                                munmap(buf, fbuf.st_size);
> > -			}
> >  
> > -			close(fd);
> > -		}
> > +                                len = strlen(newpath);
> > +                                while ((del = strstr((char *)buf, newpath))) {
> > +                                        memmove(del, del + len, strlen(del) - len + 1);
> > +                                        bytes += len;
> > +                                        ind++;
> > +                                }
> > +
> > +                                munmap(buf, fbuf.st_size + 1);
> 
> since bytes is 0 if ind is 0, you can just do the 
> 
>                                 if (ftruncate(fd, fbuf.st_size - bytes) < 0) {
>                                         SYSERROR("Failed to truncate file %s", path);
>                                         close(fd);
>                                         goto out;
>                                 }
> 
> below, rather than special-casing (ind <= 0)

Thanks!

> 
> > +                                if (ind > 0) {
> > +                                        if (ftruncate(fd, fbuf.st_size - bytes) < 0) {
> > +                                                SYSERROR("Failed to truncate file %s", path);
> > +                                                close(fd);
> > +                                                goto out;
> > +                                        }
> > +                                } else {
> > +                                        if (ftruncate(fd, fbuf.st_size) < 0) {
> > +                                                SYSERROR("Failed to truncate file %s", path);
> > +                                                close(fd);
> > +                                                goto out;
> > +                                        }
> > +                                }
> > +                        }
> > +                        close(fd);
> > +                }
> >  
> >  		/* If the lxc-snapshot file is empty, remove it. */
> >  		if (stat(path, &fbuf) < 0)
> >  			goto out;
> > -		if (!fbuf.st_size) {
> > -			remove(path);
> > +                if (!fbuf.st_size) {
> > +                        remove(path);
> >  		}
> >  	}
> >  
> > -- 
> > 2.5.1
> > 
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20150911/c4f7cd17/attachment.sig>