Fix xattr locking (#3251)

* Remove dead code

* Always grab according locks when writing/listing xattrs

* Add changelog

changelog/unreleased/fix-xattrs-race-condition2.md

@@ -0,0 +1,5 @@
+Bugfix: Make listing xattrs more robust
+
+We fixed a potential race condition when listing xattrs of nodes in concurrency situations
+
+https://github.com/cs3org/reva/pull/3251

pkg/storage/utils/decomposedfs/grants.go

@@ -32,7 +32,6 @@ import (
 	"github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/node"
 	"github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/xattrs"
 	"github.com/cs3org/reva/v2/pkg/utils"
-	"github.com/pkg/xattr"
 )
 
 // DenyGrant denies access to a resource.
@@ -107,7 +106,7 @@ func (fs *Decomposedfs) ListGrants(ctx context.Context, ref *provider.Reference)
 	log := appctx.GetLogger(ctx)
 	np := node.InternalPath()
 	var attrs []string
-	if attrs, err = xattr.List(np); err != nil {
+	if attrs, err = xattrs.List(np); err != nil {
 		log.Error().Err(err).Msg("error listing attributes")
 		return nil, err
 	}

pkg/storage/utils/decomposedfs/tree/tree.go

@@ -230,19 +230,6 @@ func isRootNode(nodePath string) bool {
 	return err == nil && attr == node.RootID
 }
 
-/*
-func isSharedNode(nodePath string) bool {
-	if attrs, err := xattr.List(nodePath); err == nil {
-		for i := range attrs {
-			if strings.HasPrefix(attrs[i], xattrs.GrantPrefix) {
-				return true
-			}
-		}
-	}
-	return false
-}
-*/
-
 // GetMD returns the metadata of a node in the tree
 func (t *Tree) GetMD(ctx context.Context, n *node.Node) (os.FileInfo, error) {
 	md, err := os.Stat(n.InternalPath())

pkg/storage/utils/decomposedfs/xattrs/xattrs.go

@@ -191,17 +191,42 @@ func CopyMetadata(src, target string, filter func(attributeName string) bool) (e
 // Set an extended attribute key to the given value
 // No file locking is involved here as writing a single xattr is
 // considered to be atomic.
-func Set(filePath string, key string, val string) error {
-	if err := xattr.Set(filePath, key, []byte(val)); err != nil {
-		return err
+func Set(filePath string, key string, val string) (err error) {
+	fileLock, err := filelocks.AcquireWriteLock(filePath)
+
+	if err != nil {
+		return errors.Wrap(err, "xattrs: Can not acquire write log")
 	}
-	return nil
+	defer func() {
+		rerr := filelocks.ReleaseLock(fileLock)
+
+		// if err is non nil we do not overwrite that
+		if err == nil {
+			err = rerr
+		}
+	}()
+
+	return xattr.Set(filePath, key, []byte(val))
 }
 
 // Remove an extended attribute key
 // No file locking is involved here as writing a single xattr is
 // considered to be atomic.
-func Remove(filePath string, key string) error {
+func Remove(filePath string, key string) (err error) {
+	fileLock, err := filelocks.AcquireWriteLock(filePath)
+
+	if err != nil {
+		return errors.Wrap(err, "xattrs: Can not acquire write log")
+	}
+	defer func() {
+		rerr := filelocks.ReleaseLock(fileLock)
+
+		// if err is non nil we do not overwrite that
+		if err == nil {
+			err = rerr
+		}
+	}()
+
 	return xattr.Remove(filePath, key)
 }
 
@@ -273,16 +298,23 @@ func GetInt64(filePath, key string) (int64, error) {
 
 // List retrieves a list of names of extended attributes associated with the
 // given path in the file system.
-func List(path string) ([]string, error) {
-	attrNames, err := xattr.List(path)
-	// Retry once on failures to work around race conditions
+func List(filePath string) (attribs []string, err error) {
+	// now try to get a shared lock on the source
+	readLock, err := filelocks.AcquireReadLock(filePath)
+
 	if err != nil {
-		attrNames, err = xattr.List(path)
-		if err != nil {
-			return nil, err
-		}
+		return nil, errors.Wrap(err, "xattrs: Unable to lock file for read")
 	}
-	return attrNames, nil
+	defer func() {
+		rerr := filelocks.ReleaseLock(readLock)
+
+		// if err is non nil we do not overwrite that
+		if err == nil {
+			err = rerr
+		}
+	}()
+
+	return xattr.List(filePath)
 }
 
 // All reads all extended attributes for a node, protected by a