Handle terminal symlinks correctly in OpenAt() implementations.

All filesystems except tmpfs had the same bug regarding handling symlinks
in the terminal path component. When they tried to open(2) such a symlink
file, these implementations called into stepLocked() with
Then these implementations also called vfs.HandleSymlink() which would
also advance rp (when rp.Done() was already true).

This was tripping the invariants checks in sentry/vfs/debug.go.

PiperOrigin-RevId: 504098263
This commit is contained in:
Ayush Ranjan
2023-01-23 15:35:15 -08:00
committed by gVisor bot
parent 12a930a63e
commit aff351f684
5 changed files with 136 additions and 154 deletions
+20 -22
View File
@@ -164,9 +164,7 @@ func (fs *filesystem) renameMuUnlockAndCheckCaching(ctx context.Context, ds **[]
}
// stepLocked resolves rp.Component() to an existing file, starting from the
// given directory. If the file at rp.Component is a symlink and
// mayFollowSymlinks is set, the symlink is resolved and the result returned to
// the caller (single step).
// given directory.
//
// Dentries which may become cached as a result of the traversal are appended
// to *ds.
@@ -214,10 +212,8 @@ func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *
if err != nil {
return nil, false, err
}
if err := rp.HandleSymlink(target); err != nil {
return nil, false, err
}
return d, true, nil
followedSymlink, err := rp.HandleSymlink(target)
return d, followedSymlink, err
}
rp.Advance()
return child, false, nil
@@ -816,13 +812,13 @@ func (fs *filesystem) MknodAt(ctx context.Context, rp *vfs.ResolvingPath, opts v
// to creating a synthetic one, i.e. one that is kept entirely in memory.
// Check that we're not overriding an existing file with a synthetic one.
_, _, err := fs.stepLocked(ctx, rp, parent, false, ds)
_, _, err := fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, ds)
switch {
case err == nil:
// Step succeeded, another file exists.
return nil, linuxerr.EEXIST
case !linuxerr.Equals(linuxerr.ENOENT, err):
// Unexpected error.
// Schrödinger. File/Cat may or may not exist.
return nil, err
}
@@ -913,7 +909,21 @@ afterTrailingSymlink:
}
// Determine whether or not we need to create a file.
parent.dirMu.Lock()
child, _, err := fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, &ds)
child, followedSymlink, err := fs.stepLocked(ctx, rp, parent, true /* mayFollowSymlinks */, &ds)
if followedSymlink {
parent.dirMu.Unlock()
if mustCreate {
// EEXIST must be returned if an existing symlink is opened with O_EXCL.
return nil, linuxerr.EEXIST
}
if err != nil {
// If followedSymlink && err != nil, then this symlink resolution error
// must be handled by the VFS layer.
return nil, err
}
start = parent
goto afterTrailingSymlink
}
if linuxerr.Equals(linuxerr.ENOENT, err) && mayCreate {
if parent.isSynthetic() {
parent.dirMu.Unlock()
@@ -930,18 +940,6 @@ afterTrailingSymlink:
if mustCreate {
return nil, linuxerr.EEXIST
}
// Open existing child or follow symlink.
if child.isSymlink() && rp.ShouldFollowSymlink() {
target, err := child.readlink(ctx, rp.Mount())
if err != nil {
return nil, err
}
if err := rp.HandleSymlink(target); err != nil {
return nil, err
}
start = parent
goto afterTrailingSymlink
}
if rp.MustBeDir() && !child.isDir() {
return nil, linuxerr.ENOTDIR
}
+35 -46
View File
@@ -37,15 +37,14 @@ import (
// - !rp.Done().
//
// Postcondition: Caller must call fs.processDeferredDecRefs*.
func (fs *Filesystem) stepExistingLocked(ctx context.Context, rp *vfs.ResolvingPath, d *Dentry, mayFollowSymlinks bool) (*Dentry, error) {
func (fs *Filesystem) stepExistingLocked(ctx context.Context, rp *vfs.ResolvingPath, d *Dentry) (*Dentry, bool, error) {
if !d.isDir() {
return nil, linuxerr.ENOTDIR
return nil, false, linuxerr.ENOTDIR
}
// Directory searchable?
if err := d.inode.CheckPermissions(ctx, rp.Credentials(), vfs.MayExec); err != nil {
return nil, err
return nil, false, err
}
afterSymlink:
name := rp.Component()
// Revalidation must be skipped if name is "." or ".."; d or its parent
// respectively can't be expected to transition from invalidated back to
@@ -54,54 +53,49 @@ afterSymlink:
// calls d_revalidate(), but walk_component() => handle_dots() does not.
if name == "." {
rp.Advance()
return d, nil
return d, false, nil
}
if name == ".." {
if isRoot, err := rp.CheckRoot(ctx, d.VFSDentry()); err != nil {
return nil, err
return nil, false, err
} else if isRoot || d.parent == nil {
rp.Advance()
return d, nil
return d, false, nil
}
if err := rp.CheckMount(ctx, d.parent.VFSDentry()); err != nil {
return nil, err
return nil, false, err
}
rp.Advance()
return d.parent, nil
return d.parent, false, nil
}
if len(name) > linux.NAME_MAX {
return nil, linuxerr.ENAMETOOLONG
return nil, false, linuxerr.ENAMETOOLONG
}
d.dirMu.Lock()
next, err := fs.revalidateChildLocked(ctx, rp.VirtualFilesystem(), d, name, d.children[name])
d.dirMu.Unlock()
if err != nil {
return nil, err
return nil, false, err
}
if err := rp.CheckMount(ctx, next.VFSDentry()); err != nil {
return nil, err
return nil, false, err
}
// Resolve any symlink at current path component.
if mayFollowSymlinks && rp.ShouldFollowSymlink() && next.isSymlink() {
if rp.ShouldFollowSymlink() && next.isSymlink() {
targetVD, targetPathname, err := next.inode.Getlink(ctx, rp.Mount())
if err != nil {
return nil, err
return nil, false, err
}
if targetVD.Ok() {
err := rp.HandleJump(targetVD)
followedTarget, err := rp.HandleJump(targetVD)
fs.deferDecRefVD(ctx, targetVD)
if err != nil {
return nil, err
}
} else {
if err := rp.HandleSymlink(targetPathname); err != nil {
return nil, err
}
return d, followedTarget, err
}
goto afterSymlink
followedSymlink, err := rp.HandleSymlink(targetPathname)
return d, followedSymlink, err
}
rp.Advance()
return next, nil
return next, false, nil
}
// revalidateChildLocked must be called after a call to parent.vfsd.Child(name)
@@ -163,7 +157,7 @@ func (fs *Filesystem) walkExistingLocked(ctx context.Context, rp *vfs.ResolvingP
d := rp.Start().Impl().(*Dentry)
for !rp.Done() {
var err error
d, err = fs.stepExistingLocked(ctx, rp, d, true /* mayFollowSymlinks */)
d, _, err = fs.stepExistingLocked(ctx, rp, d)
if err != nil {
return nil, err
}
@@ -190,7 +184,7 @@ func (fs *Filesystem) walkParentDirLocked(ctx context.Context, rp *vfs.Resolving
d := rp.Start().Impl().(*Dentry)
for !rp.Final() {
var err error
d, err = fs.stepExistingLocked(ctx, rp, d, true /* mayFollowSymlinks */)
d, _, err = fs.stepExistingLocked(ctx, rp, d)
if err != nil {
return nil, err
}
@@ -561,7 +555,19 @@ afterTrailingSymlink:
return nil, linuxerr.ENOENT
}
// Determine whether or not we need to create a file.
child, err := fs.stepExistingLocked(ctx, rp, parent, false /* mayFollowSymlinks */)
child, followedSymlink, err := fs.stepExistingLocked(ctx, rp, parent)
if followedSymlink {
if mustCreate {
// EEXIST must be returned if an existing symlink is opened with O_EXCL.
return nil, linuxerr.EEXIST
}
if err != nil {
// If followedSymlink && err != nil, then this symlink resolution error
// must be handled by the VFS layer.
return nil, err
}
goto afterTrailingSymlink
}
if linuxerr.Equals(linuxerr.ENOENT, err) {
// Already checked for searchability above; now check for writability.
if err := parent.inode.CheckPermissions(ctx, rp.Credentials(), vfs.MayWrite); err != nil {
@@ -595,25 +601,8 @@ afterTrailingSymlink:
if mustCreate {
return nil, linuxerr.EEXIST
}
if rp.ShouldFollowSymlink() && child.isSymlink() {
targetVD, targetPathname, err := child.inode.Getlink(ctx, rp.Mount())
if err != nil {
return nil, err
}
if targetVD.Ok() {
err := rp.HandleJump(targetVD)
fs.deferDecRefVD(ctx, targetVD)
if err != nil {
return nil, err
}
} else {
if err := rp.HandleSymlink(targetPathname); err != nil {
return nil, err
}
}
// rp.Final() may no longer be true since we now need to resolve the
// symlink target.
goto afterTrailingSymlink
if rp.MustBeDir() && !child.isDir() {
return nil, linuxerr.ENOTDIR
}
if err := child.inode.CheckPermissions(ctx, rp.Credentials(), ats); err != nil {
return nil, err
+33 -34
View File
@@ -135,54 +135,51 @@ func (fs *filesystem) renameMuUnlockAndCheckDrop(ctx context.Context, ds **[]*de
// - fs.renameMu must be locked.
// - d.dirMu must be locked.
// - !rp.Done().
func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, mayFollowSymlinks bool, ds **[]*dentry) (*dentry, lookupLayer, error) {
func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, ds **[]*dentry) (*dentry, lookupLayer, bool, error) {
if !d.isDir() {
return nil, lookupLayerNone, linuxerr.ENOTDIR
return nil, lookupLayerNone, false, linuxerr.ENOTDIR
}
if err := d.checkPermissions(rp.Credentials(), vfs.MayExec); err != nil {
return nil, lookupLayerNone, err
return nil, lookupLayerNone, false, err
}
afterSymlink:
name := rp.Component()
if name == "." {
rp.Advance()
return d, d.topLookupLayer(), nil
return d, d.topLookupLayer(), false, nil
}
if name == ".." {
if isRoot, err := rp.CheckRoot(ctx, &d.vfsd); err != nil {
return nil, lookupLayerNone, err
return nil, lookupLayerNone, false, err
} else if isRoot || d.parent == nil {
rp.Advance()
return d, d.topLookupLayer(), nil
return d, d.topLookupLayer(), false, nil
}
if err := rp.CheckMount(ctx, &d.parent.vfsd); err != nil {
return nil, lookupLayerNone, err
return nil, lookupLayerNone, false, err
}
rp.Advance()
return d.parent, d.parent.topLookupLayer(), nil
return d.parent, d.parent.topLookupLayer(), false, nil
}
if uint64(len(name)) > fs.maxFilenameLen {
return nil, lookupLayerNone, linuxerr.ENAMETOOLONG
return nil, lookupLayerNone, false, linuxerr.ENAMETOOLONG
}
child, topLookupLayer, err := fs.getChildLocked(ctx, d, name, ds)
if err != nil {
return nil, topLookupLayer, err
return nil, topLookupLayer, false, err
}
if err := rp.CheckMount(ctx, &child.vfsd); err != nil {
return nil, lookupLayerNone, err
return nil, lookupLayerNone, false, err
}
if child.isSymlink() && mayFollowSymlinks && rp.ShouldFollowSymlink() {
if child.isSymlink() && rp.ShouldFollowSymlink() {
target, err := child.readlink(ctx)
if err != nil {
return nil, lookupLayerNone, err
return nil, lookupLayerNone, false, err
}
if err := rp.HandleSymlink(target); err != nil {
return nil, topLookupLayer, err
}
goto afterSymlink // don't check the current directory again
followedSymlink, err := rp.HandleSymlink(target)
return d, topLookupLayer, followedSymlink, err
}
rp.Advance()
return child, topLookupLayer, nil
return child, topLookupLayer, false, nil
}
// Preconditions:
@@ -447,7 +444,7 @@ func (ll lookupLayer) existsInOverlay() bool {
func (fs *filesystem) walkParentDirLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, ds **[]*dentry) (*dentry, error) {
for !rp.Final() {
d.dirMu.Lock()
next, _, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds)
next, _, _, err := fs.stepLocked(ctx, rp, d, ds)
d.dirMu.Unlock()
if err != nil {
return nil, err
@@ -467,7 +464,7 @@ func (fs *filesystem) resolveLocked(ctx context.Context, rp *vfs.ResolvingPath,
d := rp.Start().Impl().(*dentry)
for !rp.Done() {
d.dirMu.Lock()
next, _, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds)
next, _, _, err := fs.stepLocked(ctx, rp, d, ds)
d.dirMu.Unlock()
if err != nil {
return nil, err
@@ -855,7 +852,21 @@ afterTrailingSymlink:
}
// Determine whether or not we need to create a file.
parent.dirMu.Lock()
child, topLookupLayer, err := fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, &ds)
child, topLookupLayer, followedSymlink, err := fs.stepLocked(ctx, rp, parent, &ds)
if followedSymlink {
parent.dirMu.Unlock()
if mustCreate {
// EEXIST must be returned if an existing symlink is opened with O_EXCL.
return nil, linuxerr.EEXIST
}
if err != nil {
// If followedSymlink && err != nil, then this symlink resolution error
// must be handled by the VFS layer.
return nil, err
}
start = parent
goto afterTrailingSymlink
}
if linuxerr.Equals(linuxerr.ENOENT, err) && mayCreate {
fd, err := fs.createAndOpenLocked(ctx, rp, parent, &opts, &ds, topLookupLayer == lookupLayerUpperWhiteout)
parent.dirMu.Unlock()
@@ -865,21 +876,9 @@ afterTrailingSymlink:
if err != nil {
return nil, err
}
// Open existing child or follow symlink.
if mustCreate {
return nil, linuxerr.EEXIST
}
if child.isSymlink() && rp.ShouldFollowSymlink() {
target, err := child.readlink(ctx)
if err != nil {
return nil, err
}
if err := rp.HandleSymlink(target); err != nil {
return nil, err
}
start = parent
goto afterTrailingSymlink
}
if rp.MustBeDir() && !child.isDir() {
return nil, linuxerr.ENOTDIR
}
+34 -42
View File
@@ -51,53 +51,50 @@ func (fs *filesystem) Sync(ctx context.Context) error {
// Preconditions:
// - filesystem.mu must be locked.
// - !rp.Done().
func stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry) (*dentry, error) {
func stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry) (*dentry, bool, error) {
dir, ok := d.inode.impl.(*directory)
if !ok {
return nil, linuxerr.ENOTDIR
return nil, false, linuxerr.ENOTDIR
}
if err := d.inode.checkPermissions(rp.Credentials(), vfs.MayExec); err != nil {
return nil, err
return nil, false, err
}
afterSymlink:
name := rp.Component()
if name == "." {
rp.Advance()
return d, nil
return d, false, nil
}
if name == ".." {
if isRoot, err := rp.CheckRoot(ctx, &d.vfsd); err != nil {
return nil, err
return nil, false, err
} else if isRoot || d.parent == nil {
rp.Advance()
return d, nil
return d, false, nil
}
if err := rp.CheckMount(ctx, &d.parent.vfsd); err != nil {
return nil, err
return nil, false, err
}
rp.Advance()
return d.parent, nil
return d.parent, false, nil
}
if len(name) > d.inode.fs.maxFilenameLen {
return nil, linuxerr.ENAMETOOLONG
return nil, false, linuxerr.ENAMETOOLONG
}
child, ok := dir.childMap[name]
if !ok {
return nil, linuxerr.ENOENT
return nil, false, linuxerr.ENOENT
}
if err := rp.CheckMount(ctx, &child.vfsd); err != nil {
return nil, err
return nil, false, err
}
if symlink, ok := child.inode.impl.(*symlink); ok && rp.ShouldFollowSymlink() {
// Symlink traversal updates access time.
child.inode.touchAtime(rp.Mount())
if err := rp.HandleSymlink(symlink.target); err != nil {
return nil, err
}
goto afterSymlink // don't check the current directory again
followedSymlink, err := rp.HandleSymlink(symlink.target)
return d, followedSymlink, err
}
rp.Advance()
return child, nil
return child, false, nil
}
// walkParentDirLocked resolves all but the last path component of rp to an
@@ -113,7 +110,7 @@ afterSymlink:
// - !rp.Done().
func walkParentDirLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry) (*directory, error) {
for !rp.Final() {
next, err := stepLocked(ctx, rp, d)
next, _, err := stepLocked(ctx, rp, d)
if err != nil {
return nil, err
}
@@ -140,13 +137,13 @@ func resolveLocked(ctx context.Context, rp *vfs.ResolvingPath) (*dentry, error)
//
// Symlink traversal updates access time.
d.inode.touchAtime(rp.Mount())
if err := rp.HandleSymlink(symlink.target); err != nil {
if _, err := rp.HandleSymlink(symlink.target); err != nil {
return nil, err
}
} else {
// Path with multiple components, walk and resolve as required.
for !rp.Done() {
next, err := stepLocked(ctx, rp, d)
next, _, err := stepLocked(ctx, rp, d)
if err != nil {
return nil, err
}
@@ -397,15 +394,21 @@ afterTrailingSymlink:
return nil, linuxerr.EISDIR
}
name := rp.Component()
if name == "." || name == ".." {
return nil, linuxerr.EISDIR
child, followedSymlink, err := stepLocked(ctx, rp, &parentDir.dentry)
if followedSymlink {
if mustCreate {
// EEXIST must be returned if an existing symlink is opened with O_EXCL.
return nil, linuxerr.EEXIST
}
if err != nil {
// If followedSymlink && err != nil, then this symlink resolution error
// must be handled by the VFS layer.
return nil, err
}
start = &parentDir.dentry
goto afterTrailingSymlink
}
if len(name) > fs.maxFilenameLen {
return nil, linuxerr.ENAMETOOLONG
}
// Determine whether or not we need to create a file.
child, ok := parentDir.childMap[name]
if !ok {
if linuxerr.Equals(linuxerr.ENOENT, err) {
// Already checked for searchability above; now check for writability.
if err := parentDir.inode.checkPermissions(rp.Credentials(), vfs.MayWrite); err != nil {
return nil, err
@@ -429,22 +432,11 @@ afterTrailingSymlink:
parentDir.inode.touchCMtime()
return fd, nil
}
if mustCreate {
return nil, linuxerr.EEXIST
}
// Is the file mounted over?
if err := rp.CheckMount(ctx, &child.vfsd); err != nil {
if err != nil {
return nil, err
}
// Do we need to resolve a trailing symlink?
if symlink, ok := child.inode.impl.(*symlink); ok && rp.ShouldFollowSymlink() {
// Symlink traversal updates access time.
child.inode.touchAtime(rp.Mount())
if err := rp.HandleSymlink(symlink.target); err != nil {
return nil, err
}
start = &parentDir.dentry
goto afterTrailingSymlink
if mustCreate {
return nil, linuxerr.EEXIST
}
if rp.MustBeDir() && !child.inode.isDir() {
return nil, linuxerr.ENOTDIR
+14 -10
View File
@@ -327,23 +327,25 @@ func (rp *ResolvingPath) ShouldFollowSymlink() bool {
// HandleSymlink is called when the current path component is a symbolic link
// to the given target. If the calling Filesystem method should continue path
// traversal, HandleSymlink updates the path component stream to reflect the
// symlink target and returns nil. Otherwise it returns a non-nil error.
// symlink target and returns nil. Otherwise it returns a non-nil error. It
// also returns whether the symlink was successfully followed, which can be
// true even when a non-nil error like resolveAbsSymlinkError is returned.
//
// Preconditions: !rp.Done().
//
// Postconditions: If HandleSymlink returns a nil error, then !rp.Done().
func (rp *ResolvingPath) HandleSymlink(target string) error {
func (rp *ResolvingPath) HandleSymlink(target string) (bool, error) {
if rp.symlinks >= linux.MaxSymlinkTraversals {
return linuxerr.ELOOP
return false, linuxerr.ELOOP
}
if len(target) == 0 {
return linuxerr.ENOENT
return false, linuxerr.ENOENT
}
rp.symlinks++
targetPath := fspath.Parse(target)
if targetPath.Absolute {
rp.absSymlinkTarget = targetPath
return resolveAbsSymlinkError{}
return true, resolveAbsSymlinkError{}
}
// Consume the path component that represented the symlink.
rp.Advance()
@@ -354,7 +356,7 @@ func (rp *ResolvingPath) HandleSymlink(target string) error {
}
}
rp.relpathPrepend(targetPath)
return nil
return true, nil
}
// Preconditions: path.HasComponents().
@@ -379,12 +381,14 @@ func (rp *ResolvingPath) relpathPrepend(path fspath.Path) {
// the given VirtualDentry, like /proc/[pid]/fd/[fd]. If the calling Filesystem
// method should continue path traversal, HandleJump updates the path
// component stream to reflect the magic link target and returns nil. Otherwise
// it returns a non-nil error.
// it returns a non-nil error. It also returns whether the magic link was
// followed, which can be true even when a non-nil error like
// resolveMountRootOrJumpError is returned.
//
// Preconditions: !rp.Done().
func (rp *ResolvingPath) HandleJump(target VirtualDentry) error {
func (rp *ResolvingPath) HandleJump(target VirtualDentry) (bool, error) {
if rp.symlinks >= linux.MaxSymlinkTraversals {
return linuxerr.ELOOP
return false, linuxerr.ELOOP
}
rp.symlinks++
// Consume the path component that represented the magic link.
@@ -394,7 +398,7 @@ func (rp *ResolvingPath) HandleJump(target VirtualDentry) error {
target.IncRef()
rp.nextMount = target.mount
rp.nextStart = target.dentry
return resolveMountRootOrJumpError{}
return true, resolveMountRootOrJumpError{}
}
func (rp *ResolvingPath) handleError(ctx context.Context, err error) bool {