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, Christian.

Acked-by: Serge E. Hallyn 

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

[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