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

[Christian Brauner]

On Tue, Jan 26, 2016 at 09:07:17PM +0100, Christian Brauner wrote:
> 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;
Typo: It should be (and is correct in the patch):
	if (!(lower = strstr(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;
Same typo: It should be (and is correct in the patch):
	if (!(lower = strstr(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.