Merge pull request #8178 from ThomasWaldmann/acl-error-handling-master

improve acl_get / acl_set error handling (master)

7 files changed, 168 insertions, 102 deletions

diff --git a/src/borg/archive.py b/src/borg/archive.py
index 33de6fac7..4270fc386 100644
--- a/src/borg/archive.py
+++ b/src/borg/archive.py
@@ -964,7 +964,11 @@ Duration: {0.duration}
                     if not symlink:
                         os.chmod(path, item.mode)
             if not self.noacls:
-                acl_set(path, item, self.numeric_ids, fd=fd)
+                try:
+                    acl_set(path, item, self.numeric_ids, fd=fd)
+                except OSError as e:
+                    if e.errno not in (errno.ENOTSUP,):
+                        raise
             if not self.noxattrs and "xattrs" in item:
                 # chown removes Linux capabilities, so set the extended attributes at the end, after chown,
                 # since they include the Linux capabilities in the "security.capability" attribute.
@@ -1210,7 +1214,11 @@ class MetadataCollector:
             attrs["xattrs"] = StableDict(xattrs)
         if not self.noacls:
             with backup_io("extended stat (ACLs)"):
-                acl_get(path, attrs, st, self.numeric_ids, fd=fd)
+                try:
+                    acl_get(path, attrs, st, self.numeric_ids, fd=fd)
+                except OSError as e:
+                    if e.errno not in (errno.ENOTSUP,):
+                        raise
         return attrs
 
     def stat_attrs(self, st, path, fd=None):
diff --git a/src/borg/platform/darwin.pyx b/src/borg/platform/darwin.pyx
index 7fe3db3ec..b9617a7a8 100644
--- a/src/borg/platform/darwin.pyx
+++ b/src/borg/platform/darwin.pyx
@@ -1,6 +1,7 @@
 import os
 
 from libc.stdint cimport uint32_t
+from libc cimport errno
 
 from .posix import user2uid, group2gid
 from ..helpers import safe_decode, safe_encode
@@ -115,20 +116,25 @@ def _remove_non_numeric_identifier(acl):
 def acl_get(path, item, st, numeric_ids=False, fd=None):
     cdef acl_t acl = NULL
     cdef char *text = NULL
+    if isinstance(path, str):
+        path = os.fsencode(path)
     try:
         if fd is not None:
             acl = acl_get_fd_np(fd, ACL_TYPE_EXTENDED)
         else:
-            if isinstance(path, str):
-                path = os.fsencode(path)
             acl = acl_get_link_np(path, ACL_TYPE_EXTENDED)
-        if acl is not NULL:
-            text = acl_to_text(acl, NULL)
-            if text is not NULL:
-                if numeric_ids:
-                    item['acl_extended'] = _remove_non_numeric_identifier(text)
-                else:
-                    item['acl_extended'] = text
+        if acl == NULL:
+            if errno.errno == errno.ENOENT:
+                # macOS weirdness: if a file has no ACLs, it sets errno to ENOENT. :-(
+                return
+            raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+        text = acl_to_text(acl, NULL)
+        if text == NULL:
+            raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+        if numeric_ids:
+            item['acl_extended'] = _remove_non_numeric_identifier(text)
+        else:
+            item['acl_extended'] = text
     finally:
         acl_free(text)
         acl_free(acl)
@@ -139,16 +145,19 @@ def acl_set(path, item, numeric_ids=False, fd=None):
     acl_text = item.get('acl_extended')
     if acl_text is not None:
         try:
+            if isinstance(path, str):
+                path = os.fsencode(path)
             if numeric_ids:
                 acl = acl_from_text(acl_text)
             else:
                 acl = acl_from_text(<bytes>_remove_numeric_id_if_possible(acl_text))
-            if acl is not NULL:
-                if fd is not None:
-                    acl_set_fd_np(fd, acl, ACL_TYPE_EXTENDED)
-                else:
-                    if isinstance(path, str):
-                        path = os.fsencode(path)
-                    acl_set_link_np(path, ACL_TYPE_EXTENDED, acl)
+            if acl == NULL:
+                raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+            if fd is not None:
+                if acl_set_fd_np(fd, acl, ACL_TYPE_EXTENDED) == -1:
+                    raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+            else:
+                if acl_set_link_np(path, ACL_TYPE_EXTENDED, acl) == -1:
+                    raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
         finally:
             acl_free(acl)
diff --git a/src/borg/platform/freebsd.pyx b/src/borg/platform/freebsd.pyx
index 0bfc4d750..c38f03588 100644
--- a/src/borg/platform/freebsd.pyx
+++ b/src/borg/platform/freebsd.pyx
@@ -1,4 +1,7 @@
 import os
+import stat
+
+from libc cimport errno
 
 from .posix import posix_acl_use_stored_uid_gid
 from ..helpers import safe_encode, safe_decode
@@ -6,10 +9,6 @@ from .xattr import _listxattr_inner, _getxattr_inner, _setxattr_inner, split_lst
 
 API_VERSION = '1.2_05'
 
-cdef extern from "errno.h":
-    int errno
-    int EINVAL
-
 cdef extern from "sys/extattr.h":
     ssize_t c_extattr_list_file "extattr_list_file" (const char *path, int attrnamespace, void *data, size_t nbytes)
     ssize_t c_extattr_list_link "extattr_list_link" (const char *path, int attrnamespace, void *data, size_t nbytes)
@@ -44,10 +43,12 @@ cdef extern from "sys/acl.h":
     char *acl_to_text_np(acl_t acl, ssize_t *len, int flags)
     int ACL_TEXT_NUMERIC_IDS
     int ACL_TEXT_APPEND_ID
+    int acl_extended_link_np(const char * path)  # check also: acl_is_trivial_np
 
 cdef extern from "unistd.h":
     long lpathconf(const char *path, int name)
     int _PC_ACL_NFS4
+    int _PC_ACL_EXTENDED
 
 
 # On FreeBSD, borg currently only deals with the USER namespace as it is unclear
@@ -124,21 +125,21 @@ def setxattr(path, name, value, *, follow_symlinks=False):
 
 
 cdef _get_acl(p, type, item, attribute, flags, fd=None):
-    cdef acl_t acl = NULL
-    cdef char *text = NULL
-    try:
-        if fd is not None:
-            acl = acl_get_fd_np(fd, type)
-        else:
-            acl = acl_get_link_np(p, type)
-        if acl is not NULL:
-            text = acl_to_text_np(acl, NULL, flags)
-            if text is not NULL:
-                item[attribute] = text
-    finally:
-        acl_free(text)
+    cdef acl_t acl
+    cdef char *text
+    if fd is not None:
+        acl = acl_get_fd_np(fd, type)
+    else:
+        acl = acl_get_link_np(p, type)
+    if acl == NULL:
+        raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(p))
+    text = acl_to_text_np(acl, NULL, flags)
+    if text == NULL:
         acl_free(acl)
-
+        raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(p))
+    item[attribute] = text
+    acl_free(text)
+    acl_free(acl)
 
 def acl_get(path, item, st, numeric_ids=False, fd=None):
     """Saves ACL Entries
@@ -146,34 +147,46 @@ def acl_get(path, item, st, numeric_ids=False, fd=None):
     If `numeric_ids` is True the user/group field is not preserved only uid/gid
     """
     cdef int flags = ACL_TEXT_APPEND_ID
+    flags |= ACL_TEXT_NUMERIC_IDS if numeric_ids else 0
     if isinstance(path, str):
         path = os.fsencode(path)
-    ret = lpathconf(path, _PC_ACL_NFS4)
-    if ret < 0 and errno == EINVAL:
+    ret = acl_extended_link_np(path)
+    if ret < 0:
+        raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+    if ret == 0:
+        # there is no ACL defining permissions other than those defined by the traditional file permission bits.
         return
-    flags |= ACL_TEXT_NUMERIC_IDS if numeric_ids else 0
-    if ret > 0:
+    ret = lpathconf(path, _PC_ACL_NFS4)
+    if ret < 0:
+        raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+    nfs4_acl = ret == 1
+    if nfs4_acl:
         _get_acl(path, ACL_TYPE_NFS4, item, 'acl_nfs4', flags, fd=fd)
     else:
         _get_acl(path, ACL_TYPE_ACCESS, item, 'acl_access', flags, fd=fd)
-        _get_acl(path, ACL_TYPE_DEFAULT, item, 'acl_default', flags, fd=fd)
+        if stat.S_ISDIR(st.st_mode):
+            _get_acl(path, ACL_TYPE_DEFAULT, item, 'acl_default', flags, fd=fd)
 
 
 cdef _set_acl(path, type, item, attribute, numeric_ids=False, fd=None):
     cdef acl_t acl = NULL
     text = item.get(attribute)
-    if text is not None:
-        if numeric_ids and type == ACL_TYPE_NFS4:
-            text = _nfs4_use_stored_uid_gid(text)
-        elif numeric_ids and type in (ACL_TYPE_ACCESS, ACL_TYPE_DEFAULT):
-            text = posix_acl_use_stored_uid_gid(text)
+    if text:
+        if numeric_ids:
+            if type == ACL_TYPE_NFS4:
+                text = _nfs4_use_stored_uid_gid(text)
+            elif type in (ACL_TYPE_ACCESS, ACL_TYPE_DEFAULT):
+                text = posix_acl_use_stored_uid_gid(text)
+        acl = acl_from_text(<bytes>text)
+        if acl == NULL:
+            raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
         try:
-            acl = acl_from_text(<bytes> text)
-            if acl is not NULL:
-                if fd is not None:
-                    acl_set_fd_np(fd, acl, type)
-                else:
-                    acl_set_link_np(path, type, acl)
+            if fd is not None:
+                if acl_set_fd_np(fd, acl, type) == -1:
+                    raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+            else:
+                if acl_set_link_np(path, type, acl) == -1:
+                    raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
         finally:
             acl_free(acl)
 
@@ -201,6 +214,14 @@ def acl_set(path, item, numeric_ids=False, fd=None):
     """
     if isinstance(path, str):
         path = os.fsencode(path)
-    _set_acl(path, ACL_TYPE_NFS4, item, 'acl_nfs4', numeric_ids, fd=fd)
-    _set_acl(path, ACL_TYPE_ACCESS, item, 'acl_access', numeric_ids, fd=fd)
-    _set_acl(path, ACL_TYPE_DEFAULT, item, 'acl_default', numeric_ids, fd=fd)
+    ret = lpathconf(path, _PC_ACL_NFS4)
+    if ret < 0:
+        raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+    if ret == 1:
+        _set_acl(path, ACL_TYPE_NFS4, item, 'acl_nfs4', numeric_ids, fd=fd)
+    ret = lpathconf(path, _PC_ACL_EXTENDED)
+    if ret < 0:
+        raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+    if ret == 1:
+        _set_acl(path, ACL_TYPE_ACCESS, item, 'acl_access', numeric_ids, fd=fd)
+        _set_acl(path, ACL_TYPE_DEFAULT, item, 'acl_default', numeric_ids, fd=fd)
diff --git a/src/borg/platform/linux.pyx b/src/borg/platform/linux.pyx
index aa46da5ee..178b184ba 100644
--- a/src/borg/platform/linux.pyx
+++ b/src/borg/platform/linux.pyx
@@ -50,7 +50,7 @@ cdef extern from "sys/acl.h":
     char *acl_to_text(acl_t acl, ssize_t *len)
 
 cdef extern from "acl/libacl.h":
-    int acl_extended_file(const char *path)
+    int acl_extended_file_nofollow(const char *path)
     int acl_extended_fd(int fd)
 
 cdef extern from "linux/fs.h":
@@ -233,15 +233,19 @@ def acl_get(path, item, st, numeric_ids=False, fd=None):
     cdef acl_t access_acl = NULL
     cdef char *default_text = NULL
     cdef char *access_text = NULL
+    cdef int ret = 0
 
-    if stat.S_ISLNK(st.st_mode):
-        # symlinks can not have ACLs
-        return
     if isinstance(path, str):
         path = os.fsencode(path)
-    if (fd is not None and acl_extended_fd(fd) <= 0
-        or
-        fd is None and acl_extended_file(path) <= 0):
+    if fd is not None:
+        ret = acl_extended_fd(fd)
+    else:
+        ret = acl_extended_file_nofollow(path)
+    if ret < 0:
+        raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+    if ret == 0:
+        # there is no ACL defining permissions other than those defined by the traditional file permission bits.
+        # note: this should also be the case for symlink fs objects, as they can not have ACLs.
         return
     if numeric_ids:
         converter = acl_numeric_ids
@@ -252,25 +256,28 @@ def acl_get(path, item, st, numeric_ids=False, fd=None):
             access_acl = acl_get_fd(fd)
         else:
             access_acl = acl_get_file(path, ACL_TYPE_ACCESS)
-        if access_acl is not NULL:
-            access_text = acl_to_text(access_acl, NULL)
-            if access_text is not NULL:
-                item['acl_access'] = converter(access_text)
+        if access_acl == NULL:
+            raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+        access_text = acl_to_text(access_acl, NULL)
+        if access_text == NULL:
+            raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+        item['acl_access'] = converter(access_text)
     finally:
         acl_free(access_text)
         acl_free(access_acl)
-
-    try:
-        if stat.S_ISDIR(st.st_mode):
-            # only directories can have a default ACL. there is no fd-based api to get it.
+    if stat.S_ISDIR(st.st_mode):
+        # only directories can have a default ACL. there is no fd-based api to get it.
+        try:
             default_acl = acl_get_file(path, ACL_TYPE_DEFAULT)
-            if default_acl is not NULL:
-                default_text = acl_to_text(default_acl, NULL)
-                if default_text is not NULL:
-                    item['acl_default'] = converter(default_text)
-    finally:
-        acl_free(default_text)
-        acl_free(default_acl)
+            if default_acl == NULL:
+                raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+            default_text = acl_to_text(default_acl, NULL)
+            if default_text == NULL:
+                raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+            item['acl_default'] = converter(default_text)
+        finally:
+            acl_free(default_text)
+            acl_free(default_acl)
 
 
 def acl_set(path, item, numeric_ids=False, fd=None):
@@ -281,7 +288,7 @@ def acl_set(path, item, numeric_ids=False, fd=None):
         # Linux does not support setting ACLs on symlinks
         return
 
-    if fd is None and isinstance(path, str):
+    if isinstance(path, str):
         path = os.fsencode(path)
     if numeric_ids:
         converter = posix_acl_use_stored_uid_gid
@@ -290,21 +297,26 @@ def acl_set(path, item, numeric_ids=False, fd=None):
     access_text = item.get('acl_access')
     if access_text is not None:
         try:
-            access_acl = acl_from_text(<bytes> converter(access_text))
-            if access_acl is not NULL:
-                if fd is not None:
-                    acl_set_fd(fd, access_acl)
-                else:
-                    acl_set_file(path, ACL_TYPE_ACCESS, access_acl)
+            access_acl = acl_from_text(<bytes>converter(access_text))
+            if access_acl == NULL:
+                raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+            if fd is not None:
+                if acl_set_fd(fd, access_acl) == -1:
+                    raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+            else:
+                if acl_set_file(path, ACL_TYPE_ACCESS, access_acl) == -1:
+                    raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
         finally:
             acl_free(access_acl)
     default_text = item.get('acl_default')
     if default_text is not None:
         try:
-            default_acl = acl_from_text(<bytes> converter(default_text))
-            if default_acl is not NULL:
-                # only directories can get a default ACL. there is no fd-based api to set it.
-                acl_set_file(path, ACL_TYPE_DEFAULT, default_acl)
+            default_acl = acl_from_text(<bytes>converter(default_text))
+            if default_acl == NULL:
+                raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+            # only directories can get a default ACL. there is no fd-based api to set it.
+            if acl_set_file(path, ACL_TYPE_DEFAULT, default_acl) == -1:
+                raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
         finally:
             acl_free(default_acl)
 
diff --git a/src/borg/testsuite/platform.py b/src/borg/testsuite/platform.py
index a8a859b09..40ea3d78c 100644
--- a/src/borg/testsuite/platform.py
+++ b/src/borg/testsuite/platform.py
@@ -1,3 +1,4 @@
+import errno
 import functools
 import os
 
@@ -31,25 +32,38 @@ def are_acls_working():
     with unopened_tempfile() as filepath:
         open(filepath, "w").close()
         try:
-            if is_freebsd:
-                access = b"user::rw-\ngroup::r--\nmask::rw-\nother::---\nuser:root:rw-\n"
-                contained = b"user:root:rw-"
+            if is_darwin:
+                acl_key = "acl_extended"
+                acl_value = b"!#acl 1\nuser:FFFFEEEE-DDDD-CCCC-BBBB-AAAA00000000:root:0:allow:read\n"
             elif is_linux:
-                access = b"user::rw-\ngroup::r--\nmask::rw-\nother::---\nuser:root:rw-:0\n"
-                contained = b"user:root:rw-:0"
-            elif is_darwin:
-                return True  # improve?
+                acl_key = "acl_access"
+                acl_value = b"user::rw-\ngroup::r--\nmask::rw-\nother::---\nuser:root:rw-:9999\ngroup:root:rw-:9999\n"
+            elif is_freebsd:
+                acl_key = "acl_access"
+                acl_value = b"user::rw-\ngroup::r--\nmask::rw-\nother::---\nuser:root:rw-\ngroup:wheel:rw-\n"
             else:
-                return False  # unsupported platform
-            acl = {"acl_access": access}
-            acl_set(filepath, acl)
+                return False  # ACLs unsupported on this platform.
+            write_acl = {acl_key: acl_value}
+            acl_set(filepath, write_acl)
             read_acl = {}
             acl_get(filepath, read_acl, os.stat(filepath))
-            read_acl_access = read_acl.get("acl_access", None)
-            if read_acl_access and contained in read_acl_access:
-                return True
+            acl = read_acl.get(acl_key, None)
+            if acl is not None:
+                if is_darwin:
+                    check_for = b"root:0:allow:read"
+                elif is_linux:
+                    check_for = b"user::rw-"
+                elif is_freebsd:
+                    check_for = b"user::rw-"
+                else:
+                    return False  # ACLs unsupported on this platform.
+                if check_for in acl:
+                    return True
         except PermissionError:
             pass
+        except OSError as e:
+            if e.errno not in (errno.ENOTSUP,):
+                raise
         return False
 
 
diff --git a/src/borg/testsuite/platform_darwin.py b/src/borg/testsuite/platform_darwin.py
index 8a614607b..8921beedc 100644
--- a/src/borg/testsuite/platform_darwin.py
+++ b/src/borg/testsuite/platform_darwin.py
@@ -20,7 +20,7 @@ def set_acl(path, acl, numeric_ids=False):
 
 
 @skipif_acls_not_working
-def test_access_acl():
+def test_extended_acl():
     file = tempfile.NamedTemporaryFile()
     assert get_acl(file.name) == {}
     set_acl(
diff --git a/src/borg/testsuite/platform_freebsd.py b/src/borg/testsuite/platform_freebsd.py
index 54fb29d6a..c47da68b6 100644
--- a/src/borg/testsuite/platform_freebsd.py
+++ b/src/borg/testsuite/platform_freebsd.py
@@ -49,6 +49,7 @@ def set_acl(path, access=None, default=None, nfs4=None, numeric_ids=False):
 @skipif_acls_not_working
 def test_access_acl():
     file1 = tempfile.NamedTemporaryFile()
+    assert get_acl(file1.name) == {}
     set_acl(
         file1.name,
         access=b"user::rw-\ngroup::r--\nmask::rw-\nother::---\nuser:root:rw-\ngroup:wheel:rw-\n",
@@ -86,6 +87,7 @@ def test_access_acl():
 @skipif_acls_not_working
 def test_default_acl():
     tmpdir = tempfile.mkdtemp()
+    assert get_acl(tmpdir) == {}
     set_acl(tmpdir, access=ACCESS_ACL, default=DEFAULT_ACL)
     assert get_acl(tmpdir)["acl_access"] == ACCESS_ACL
     assert get_acl(tmpdir)["acl_default"] == DEFAULT_ACL