Quoting Tyler Hicks (<email address hidden>):
> Hi Serge - I've reviewed the patch against git HEAD and have a couple
> questions.
Thanks, Tyler.
> 1) In mount_entry(), the call to mount(2) is changed to use target as
> both the source and the destination. I can't see why this would be a
> valid change.
It appears to me to be more correct. Since we are doing a remount,
the old source isn't really valid any more. I'm thinking of it in
terms of
mount --bind /ab /cd
mount -o bind.remount,ro /cd
But I'll test with that part undone, and change it back, as I may just
not be thinking right.
> 2) In is_file_mount(), the usage of lstat(2) seems odd for a couple reasons:
> - If path is a symlink to a directory, the mount will be incorrectly treated as a file mount in safe_mount(). I think stat(2) should be used.
Hm, yeah. I'll change that.
> - Error checking of lstat(2) is ignored. I think this should be treated as fatal for most (all?) cases.
My tree has
ret = lstat(path, &sb);
if (ret)
return false;
Oh, you mean that if the lstat (or stat) fails, the whole safe_mount()
should be aborted? That's probably a good thing to fix.
> 3) In open_without_symlink(), why is curlen decremented after assigning
> it the return value of strlen(prefix_skip)? Is it because prefix_skip is
> somehow guaranteed to have a trailing '/' character?
Hm, I don't think that's guaranteed.
> 4) open_without_symlink() is documented to return -1 upon error but it
> returns -EINVAL when target doesn't start with prefix_skip. (minor)
>
> 5) Is there a need to protect against ".." components in
> open_without_symlink()? I'm thinking of a target of "/foo/../" and a
> prefix_skip of "/foo".
The container admin cannot change the path being passed to
open_without_symlink() without having access to the container config
file, in which case he owns the host (or parent container) end of
story. But I think it's probably worth simply refusing any paths
with '..' in them, yes.
Quoting Tyler Hicks (<email address hidden>):
> Hi Serge - I've reviewed the patch against git HEAD and have a couple
> questions.
Thanks, Tyler.
> 1) In mount_entry(), the call to mount(2) is changed to use target as
> both the source and the destination. I can't see why this would be a
> valid change.
It appears to me to be more correct. Since we are doing a remount,
the old source isn't really valid any more. I'm thinking of it in
terms of
mount --bind /ab /cd
mount -o bind.remount,ro /cd
But I'll test with that part undone, and change it back, as I may just
not be thinking right.
> 2) In is_file_mount(), the usage of lstat(2) seems odd for a couple reasons:
> - If path is a symlink to a directory, the mount will be incorrectly treated as a file mount in safe_mount(). I think stat(2) should be used.
Hm, yeah. I'll change that.
> - Error checking of lstat(2) is ignored. I think this should be treated as fatal for most (all?) cases.
My tree has
ret = lstat(path, &sb);
if (ret)
return false;
Oh, you mean that if the lstat (or stat) fails, the whole safe_mount()
should be aborted? That's probably a good thing to fix.
> 3) In open_without_ symlink( ), why is curlen decremented after assigning prefix_ skip)? Is it because prefix_skip is
> it the return value of strlen(
> somehow guaranteed to have a trailing '/' character?
Hm, I don't think that's guaranteed.
> 4) open_without_ symlink( ) is documented to return -1 upon error but it symlink( )? I'm thinking of a target of "/foo/../" and a
> returns -EINVAL when target doesn't start with prefix_skip. (minor)
>
> 5) Is there a need to protect against ".." components in
> open_without_
> prefix_skip of "/foo".
The container admin cannot change the path being passed to symlink( ) without having access to the container config
open_without_
file, in which case he owns the host (or parent container) end of
story. But I think it's probably worth simply refusing any paths
with '..' in them, yes.