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

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrauner at gmail.com):
> On Mon, Sep 07, 2015 at 07:50:10PM +0000, Serge Hallyn wrote:
> > Quoting Christian Brauner (christianvanbrauner at 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 <christianvanbrauner at gmail.com>
> > > > ---
> > > >  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.