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

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