[lxc-devel] [PATCH] aufs: support multiple lower layers

[Christian Brauner]

On Tue, Jan 26, 2016 at 06:51:04PM +0000, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrauner at gmail.com):
> > Do it in a safe way by using strstr() to check for the substring ":/" should
> > ':' be part of a pathname.
> > 
> > Signed-off-by: Christian Brauner <christian.brauner at mailbox.org>
> 
> Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
> 
> > ---
> >  src/lxc/bdev/lxcaufs.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/lxc/bdev/lxcaufs.c b/src/lxc/bdev/lxcaufs.c
> > index a5c34aa..408f6a3 100644
> > --- a/src/lxc/bdev/lxcaufs.c
> > +++ b/src/lxc/bdev/lxcaufs.c
> > @@ -234,7 +234,7 @@ int aufs_detect(const char *path)
> >  
> >  int aufs_mount(struct bdev *bdev)
> >  {
> > -	char *options, *dup, *lower, *upper;
> > +	char *tmp, *options, *dup, *lower, *upper;
> >  	int len;
> >  	unsigned long mntflags;
> >  	char *mntdata;
> > @@ -250,9 +250,15 @@ int aufs_mount(struct bdev *bdev)
> >  	//  mount -t aufs -obr=${upper}=rw:${lower}=ro lower dest
> >  	dup = alloca(strlen(bdev->src)+1);
> >  	strcpy(dup, bdev->src);
> > -	if (!(lower = strchr(dup, ':')))
> > -		return -22;
> > -	if (!(upper = strchr(++lower, ':')))
> > +	/* support multiple lower layers */
> > +	if (!(lower = strstr(dup, ":/")))
> > +			return -22;
> > +	lower++;
> > +	upper = lower;
> > +	while ((tmp = strstr(++upper, ":/"))) {
> > +		upper = tmp;
> > +	}
> > +	if (--upper == lower)
> >  		return -22;
> 
> Hm, so if lower = /some/path:/other/path, strstr will result in upper =
> ':', then you check --upper and up with upper pointing to the last 'h'
> in /some/path.  Then you set that to \0.  I'm probably off since this
> would've failed for you immediately, but why am I wrong?
Hm, here is how I understood it:

**Case A: "Everything as expected"**

lxc.rootfs = aufs:/lower1:/lower2:/upper

	if (!(lower = strchr(dup, ':')))
		return -22;

now we have

        // lower = :/lower1:/lower2:/upper

we do

        lower++;

to move forward and not have strstr() do the work from the beginning again.

Now we have

        // lower = /lower1:/lower2:/upper

we set

        upper = lower;

and have

        // upper = lower = /lower1:/lower2:/upper

we do

        while ((tmp = strstr(++upper, ":/"))) {
        	upper = tmp;
        }

and end up with:

        // upper = /upper

now we do

        if (--upper == lower)
        	return -22;

and as you correctly noticed we compare

        // :/upper

with
        // /lower1:/lower2

But that is by design: For a case where we find at least two path entries this
test really does not serve any purpose at all. Moving back or forth one byte
does not alter the fact that we found two different entries. It becomes import
for a case where we fail to find a second path entry.

Now we set:

  	*upper = '\0';

which is at

        // :/upper

and hence "split" the rootfs string

        // aufs:/lower1:/lower2:/upper

into

        // aufs:/lower1:/lower2\0/upper

Now

        lower

points to

        // /lower1:/lower2\0

and upper moves one final byte forward

  	upper++;

and we get

        // /upper

voila.


**Case B: "Missing upper or lower"**

But now imagine doing the same stuff when we have:

        lxc.rootfs = aufs:/a

abbreviating a bit we have:

	if (!(lower = strchr(dup, ':')))
		return -22;
        // lower = :/a

        lower++;
        // lower = /a

        upper = lower;
        // upper = lower = /a

        while ((tmp = strstr(++upper, ":/"))) {
        	upper = tmp;
        }
        // upper = a

but now we do

        if (--upper == lower)
        	return -22;

and now we compare

        // /a

correctly with

        // /a

and error out.