From 707c0d4aba0b65b1bef78faff21f7081cc86ed0f Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Fri, 29 Nov 2019 20:28:41 +0100
Subject: [PATCH 01/16] Convert ACL initialisation to allow file-path objects
as well
This is easier than in has_extended, as we can directly use the
converter. Yay!
---
acl.c | 49 +++++++++++++++++++++++++++++-----------------
tests/test_acls.py | 18 +++++++----------
2 files changed, 38 insertions(+), 29 deletions(-)
diff --git a/acl.c b/acl.c
index 9b599c3..7d7f4b2 100644
--- a/acl.c
+++ b/acl.c
@@ -132,16 +132,16 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
#ifdef HAVE_LINUX
static char *kwlist[] = { "file", "fd", "text", "acl", "filedef",
"mode", NULL };
- char *format = "|etisO!si";
+ char *format = "|O&OsO!O&i";
int mode = -1;
#else
static char *kwlist[] = { "file", "fd", "text", "acl", "filedef", NULL };
- char *format = "|etisO!s";
+ char *format = "|O&OsO!O&";
#endif
- char *file = NULL;
- char *filedef = NULL;
+ PyObject *file = NULL;
+ PyObject *filedef = NULL;
char *text = NULL;
- int fd = -1;
+ PyObject *fd = NULL;
ACL_Object* thesrc = NULL;
if(!PyTuple_Check(args) || PyTuple_Size(args) != 0 ||
@@ -151,8 +151,9 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
return -1;
}
if(!PyArg_ParseTupleAndKeywords(args, keywds, format, kwlist,
- NULL, &file, &fd, &text, &ACL_Type,
- &thesrc, &filedef
+ PyUnicode_FSConverter, &file,
+ &fd, &text, &ACL_Type, &thesrc,
+ PyUnicode_FSConverter, &filedef
#ifdef HAVE_LINUX
, &mode
#endif
@@ -164,16 +165,28 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
if(self->acl != NULL)
acl_free(self->acl);
- if(file != NULL)
- self->acl = acl_get_file(file, ACL_TYPE_ACCESS);
- else if(text != NULL)
+ if(file != NULL) {
+ fprintf(stderr, "foobar!\n");
+ char *path = PyBytes_AS_STRING(file);
+ self->acl = acl_get_file(path, ACL_TYPE_ACCESS);
+ Py_DECREF(file);
+ } else if(text != NULL)
self->acl = acl_from_text(text);
- else if(fd != -1)
- self->acl = acl_get_fd(fd);
+ else if(fd != NULL) {
+ int fdval;
+ if ((fdval = PyObject_AsFileDescriptor(fd)) != -1) {
+ self->acl = acl_get_fd(fdval);
+ } else {
+ self->acl = NULL;
+ }
+ }
else if(thesrc != NULL)
self->acl = acl_dup(thesrc->acl);
- else if(filedef != NULL)
- self->acl = acl_get_file(filedef, ACL_TYPE_DEFAULT);
+ else if(filedef != NULL) {
+ char *path = PyBytes_AS_STRING(filedef);
+ self->acl = acl_get_file(path, ACL_TYPE_DEFAULT);
+ Py_DECREF(path);
+ }
#ifdef HAVE_LINUX
else if(mode != -1)
self->acl = acl_from_mode(mode);
@@ -1224,11 +1237,11 @@ static char __ACL_Type_doc__[] =
"\n"
".. note:: only one keyword parameter should be provided\n"
"\n"
- ":param string file: creates an ACL representing\n"
- " the access ACL of the specified file.\n"
- ":param string filedef: creates an ACL representing\n"
+ ":param string/bytes/path-like file: creates an ACL representing\n"
+ " the access ACL of the specified file or directory.\n"
+ ":param string/bytes/path-like filedef: creates an ACL representing\n"
" the default ACL of the given directory.\n"
- ":param int fd: creates an ACL representing\n"
+ ":param int/iostream fd: creates an ACL representing\n"
" the access ACL of the given file descriptor.\n"
":param string text: creates an ACL from a \n"
" textual description; note the ACL must be valid, which\n"
diff --git a/tests/test_acls.py b/tests/test_acls.py
index 6e54367..3a25b8c 100644
--- a/tests/test_acls.py
+++ b/tests/test_acls.py
@@ -233,26 +233,22 @@ def subject(testdir, request):
class TestLoad:
"""Load/create tests"""
- def test_from_file(self, testdir):
- """Test loading ACLs from a file"""
- _, fname = get_file(testdir)
- acl1 = posix1e.ACL(file=fname)
- assert acl1.valid()
+ def test_from_file(self, file_subject):
+ """Test loading ACLs from a file/directory"""
+ acl = posix1e.ACL(file=file_subject)
+ assert acl.valid()
def test_from_dir(self, testdir):
"""Test loading ACLs from a directory"""
with get_dir(testdir) as dname:
- acl1 = posix1e.ACL(file=dname)
acl2 = posix1e.ACL(filedef=dname)
- assert acl1.valid()
# default ACLs might or might not be valid; missing ones are
# not valid, so we don't test acl2 for validity
- def test_from_fd(self, testdir):
+ def test_from_fd(self, fd_subject):
"""Test loading ACLs from a file descriptor"""
- fd, _ = get_file(testdir)
- acl1 = posix1e.ACL(fd=fd)
- assert acl1.valid()
+ acl = posix1e.ACL(fd=fd_subject)
+ assert acl.valid()
def test_from_nonexisting(self, testdir):
_, fname = get_file(testdir)
--
2.39.5
From 4c85c416d3296a83f30129ff81e45c84aaf6dbef Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Sat, 30 Nov 2019 16:57:45 +0100
Subject: [PATCH 02/16] Remove doc/news.rst from git
Forgot to do this when changed readme. It is now symlinked at build
time.
---
doc/news.rst | 1 -
1 file changed, 1 deletion(-)
delete mode 120000 doc/news.rst
diff --git a/doc/news.rst b/doc/news.rst
deleted file mode 120000
index 0fae0f8..0000000
--- a/doc/news.rst
+++ /dev/null
@@ -1 +0,0 @@
-../NEWS
\ No newline at end of file
--
2.39.5
From 7c479f50bdc0e623757e1b8897e9bae3c47cbfd5 Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Sat, 30 Nov 2019 16:58:31 +0100
Subject: [PATCH 03/16] Minor change on how error handling style in ACL_init
---
acl.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/acl.c b/acl.c
index 7d7f4b2..a12a9c5 100644
--- a/acl.c
+++ b/acl.c
@@ -165,6 +165,8 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
if(self->acl != NULL)
acl_free(self->acl);
+ self->acl = NULL;
+
if(file != NULL) {
fprintf(stderr, "foobar!\n");
char *path = PyBytes_AS_STRING(file);
@@ -176,11 +178,8 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
int fdval;
if ((fdval = PyObject_AsFileDescriptor(fd)) != -1) {
self->acl = acl_get_fd(fdval);
- } else {
- self->acl = NULL;
}
- }
- else if(thesrc != NULL)
+ } else if(thesrc != NULL)
self->acl = acl_dup(thesrc->acl);
else if(filedef != NULL) {
char *path = PyBytes_AS_STRING(filedef);
--
2.39.5
From 09c5bd80cf811a0e7b81ceddfb525d576885e097 Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Wed, 4 Dec 2019 00:35:33 +0100
Subject: [PATCH 04/16] Change entry qualifier set/get behaviour
This was intended to address #13, but investigation found out more
breakage than just that. It's hard to make overflow/underflow tests
without assuming the signedness of the uid_t/gid_t types, so
assume/require that they're unsigned (it is true with glibc, MacOS and
FreeBSD) and use this to improve the behaviour:
- Fix setting very large qualifiers, both in the sense of correctly
reporting overflow when too large, and not longer falsely reporting
overflow for larger than signed max but smaller than unsigned max;
- Fix returning very large (larger than signed max value) qualifiers;
Fixes #13.
---
NEWS | 6 ++++++
acl.c | 17 +++++++++++------
tests/test_acls.py | 11 +++++------
3 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/NEWS b/NEWS
index 466481f..43210c5 100644
--- a/NEWS
+++ b/NEWS
@@ -29,6 +29,12 @@ Important API changes/bug fixes:
inadvertently removed due a typo(!) when adding support for it in
FreeBSD. Pickle should work again for ACL instances, although not sure
how stable this serialisation format actually is.
+- Fix (and change) entry qualifier (which is a user/group ID) behaviour:
+ assume/require that uid_t/gid_t are unsigned types (they are with
+ glibc, MacOS and FreeBSD at least; the standard doesn't document the
+ signedness), and convert parsing and returning the qualifier to behave
+ accordingly. The breakage was most apparent on 32-bit architectures,
+ in which context the problem was originally reported (see issue #13).
Minor improvements:
diff --git a/acl.c b/acl.c
index a12a9c5..e88d143 100644
--- a/acl.c
+++ b/acl.c
@@ -869,7 +869,7 @@ static PyObject* Entry_get_tag_type(PyObject *obj, void* arg) {
*/
static int Entry_set_qualifier(PyObject* obj, PyObject* value, void* arg) {
Entry_Object *self = (Entry_Object*) obj;
- long uidgid;
+ unsigned long uidgid;
uid_t uid;
gid_t gid;
void *p;
@@ -882,7 +882,10 @@ static int Entry_set_qualifier(PyObject* obj, PyObject* value, void* arg) {
"qualifier must be integer");
return -1;
}
- if((uidgid = PyLong_AsLong(value)) == -1) {
+ /* This is the negative value check, and larger than long
+ check. If uid_t/gid_t are long-sized, this is enough to check
+ for both over and underflow. */
+ if((uidgid = PyLong_AsUnsignedLong(value)) == (unsigned long) -1) {
if(PyErr_Occurred() != NULL) {
return -1;
}
@@ -896,9 +899,11 @@ static int Entry_set_qualifier(PyObject* obj, PyObject* value, void* arg) {
}
uid = uidgid;
gid = uidgid;
+ /* This is an extra overflow check, in case uid_t/gid_t are
+ int-sized (and int size smaller than long size). */
switch(tag) {
case ACL_USER:
- if((long)uid != uidgid) {
+ if((unsigned long)uid != uidgid) {
PyErr_SetString(PyExc_OverflowError, "Can't assign given qualifier");
return -1;
} else {
@@ -906,7 +911,7 @@ static int Entry_set_qualifier(PyObject* obj, PyObject* value, void* arg) {
}
break;
case ACL_GROUP:
- if((long)gid != uidgid) {
+ if((unsigned long)gid != uidgid) {
PyErr_SetString(PyExc_OverflowError, "Can't assign given qualifier");
return -1;
} else {
@@ -929,7 +934,7 @@ static int Entry_set_qualifier(PyObject* obj, PyObject* value, void* arg) {
/* Returns the qualifier of the entry */
static PyObject* Entry_get_qualifier(PyObject *obj, void* arg) {
Entry_Object *self = (Entry_Object*) obj;
- long value;
+ unsigned long value;
tag_qual tq;
if (self->entry == NULL) {
@@ -949,7 +954,7 @@ static PyObject* Entry_get_qualifier(PyObject *obj, void* arg) {
" group tag");
return NULL;
}
- return PyLong_FromLong(value);
+ return PyLong_FromUnsignedLong(value);
}
/* Returns the parent ACL of the entry */
diff --git a/tests/test_acls.py b/tests/test_acls.py
index 3a25b8c..8ba98de 100644
--- a/tests/test_acls.py
+++ b/tests/test_acls.py
@@ -846,10 +846,7 @@ class TestModification:
qualifier = 1
e.tag_type = tag
while True:
- if tag == posix1e.ACL_USER:
- regex = re.compile("user with uid %d" % qualifier)
- else:
- regex = re.compile("group with gid %d" % qualifier)
+ regex = re.compile("(user|group) with (u|g)id %d" % qualifier)
try:
e.qualifier = qualifier
except OverflowError:
@@ -863,13 +860,15 @@ class TestModification:
"""Tests qualifier overflow handling"""
acl = posix1e.ACL()
e = acl.append()
- qualifier = sys.maxsize * 2
+ # the uid_t/gid_t are unsigned, so they can hold slightly more
+ # than sys.maxsize*2 (on Linux).
+ qualifier = (sys.maxsize + 1) * 2
for tag in [posix1e.ACL_USER, posix1e.ACL_GROUP]:
e.tag_type = tag
with pytest.raises(OverflowError):
e.qualifier = qualifier
- def test_negative_qualifier(self):
+ def test_qualifier_underflow(self):
"""Tests negative qualifier handling"""
# Note: this presumes that uid_t/gid_t in C are unsigned...
acl = posix1e.ACL()
--
2.39.5
From 30d5799a7061e3a55ed8cb7a9eed97e9c124e1fd Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Wed, 4 Dec 2019 00:50:57 +0100
Subject: [PATCH 05/16] Fix from_acl tests for non-Linux platforms
FreeBSD doesn't have acl_cmp, so comparison via rich compare is not
defined, thus all comparisons are False. Fix tests so the equality
check is only done on Linux, and add a poor man's test via string
representation equality.
---
tests/test_acls.py | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/tests/test_acls.py b/tests/test_acls.py
index 8ba98de..2063226 100644
--- a/tests/test_acls.py
+++ b/tests/test_acls.py
@@ -271,12 +271,20 @@ class TestLoad:
acl1 = posix1e.ACL(text=BASIC_ACL_TEXT)
assert acl1.valid()
+ # This is acl_check, but should actually be have_linux...
+ @require_acl_check
def test_from_acl(self):
"""Test creating an ACL from an existing ACL"""
- acl1 = posix1e.ACL()
+ acl1 = posix1e.ACL(text=BASIC_ACL_TEXT)
acl2 = posix1e.ACL(acl=acl1)
assert acl1 == acl2
+ def test_from_acl_via_str(self):
+ # This is needed for not HAVE_LINUX cases.
+ acl1 = posix1e.ACL(text=BASIC_ACL_TEXT)
+ acl2 = posix1e.ACL(acl=acl1)
+ assert str(acl1) == str(acl2)
+
def test_invalid_creation_params(self, testdir):
"""Test that creating an ACL from multiple objects fails"""
fd, _ = get_file(testdir)
--
2.39.5
From 2e8ed0ef1f219a4bdec927bd459e1747cb4aacfe Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Thu, 5 Dec 2019 00:26:12 +0100
Subject: [PATCH 06/16] Move a few tests from explicit loop to parameters
Failure messages should be more better in failure case.
---
tests/test_acls.py | 129 ++++++++++++++++++++++++---------------------
1 file changed, 70 insertions(+), 59 deletions(-)
diff --git a/tests/test_acls.py b/tests/test_acls.py
index 2063226..eb01669 100644
--- a/tests/test_acls.py
+++ b/tests/test_acls.py
@@ -42,11 +42,13 @@ TEST_DIR = os.environ.get("TEST_DIR", ".")
BASIC_ACL_TEXT = "u::rw,g::r,o::-"
# Permset permission information
-PERMSETS = {
- posix1e.ACL_READ: ("read", posix1e.Permset.read),
- posix1e.ACL_WRITE: ("write", posix1e.Permset.write),
- posix1e.ACL_EXECUTE: ("execute", posix1e.Permset.execute),
- }
+PERMSETS = [
+ (ACL_READ, "read", Permset.read),
+ (ACL_WRITE, "write", Permset.write),
+ (ACL_EXECUTE, "execute", Permset.execute),
+]
+
+PERMSETS_IDS = [p[1] for p in PERMSETS]
ALL_TAGS = [
(posix1e.ACL_USER, "user"),
@@ -777,59 +779,68 @@ class TestModification:
p.__init__(e) # type: ignore
assert ref == sys.getrefcount(e), "Uh-oh, ref leaks..."
- def test_permset(self):
+ @pytest.mark.parametrize("perm, txt, accessor",
+ PERMSETS, ids=PERMSETS_IDS)
+ def test_permset(self, perm, txt, accessor):
"""Test permissions"""
+ del accessor
acl = posix1e.ACL()
e = acl.append()
ps = e.permset
ps.clear()
str_ps = str(ps)
self.checkRef(str_ps)
- for perm in PERMSETS:
- str_ps = str(ps)
- txt = PERMSETS[perm][0]
- self.checkRef(str_ps)
- assert not ps.test(perm), ("Empty permission set should not"
- " have permission '%s'" % txt)
- ps.add(perm)
- assert ps.test(perm), ("Permission '%s' should exist"
- " after addition" % txt)
- str_ps = str(ps)
- self.checkRef(str_ps)
- ps.delete(perm)
- assert not ps.test(perm), ("Permission '%s' should not exist"
- " after deletion" % txt)
-
- def test_permset_via_accessors(self):
+ assert not ps.test(perm), ("Empty permission set should not"
+ " have permission '%s'" % txt)
+ ps.add(perm)
+ assert ps.test(perm), ("Permission '%s' should exist"
+ " after addition" % txt)
+ str_ps = str(ps)
+ self.checkRef(str_ps)
+ ps.delete(perm)
+ assert not ps.test(perm), ("Permission '%s' should not exist"
+ " after deletion" % txt)
+ ps.add(perm)
+ assert ps.test(perm), ("Permission '%s' should exist"
+ " after addition" % txt)
+ ps.clear()
+ assert not ps.test(perm), ("Permission '%s' should not exist"
+ " after clearing" % txt)
+
+
+
+ @pytest.mark.parametrize("perm, txt, accessor",
+ PERMSETS, ids=PERMSETS_IDS)
+ def test_permset_via_accessors(self, perm, txt, accessor):
"""Test permissions"""
acl = posix1e.ACL()
e = acl.append()
ps = e.permset
ps.clear()
+ def getter():
+ return accessor.__get__(ps) # type: ignore
+ def setter(value):
+ return accessor.__set__(ps, value) # type: ignore
str_ps = str(ps)
self.checkRef(str_ps)
- def getter(perm):
- return PERMSETS[perm][1].__get__(ps) # type: ignore
- def setter(parm, value):
- return PERMSETS[perm][1].__set__(ps, value) # type: ignore
- for perm in PERMSETS:
- str_ps = str(ps)
- self.checkRef(str_ps)
- txt = PERMSETS[perm][0]
- assert not getter(perm), ("Empty permission set should not"
- " have permission '%s'" % txt)
- setter(perm, True)
- assert ps.test(perm), ("Permission '%s' should exist"
- " after addition" % txt)
- assert getter(perm), ("Permission '%s' should exist"
- " after addition" % txt)
- str_ps = str(ps)
- self.checkRef(str_ps)
- setter(perm, False)
- assert not ps.test(perm), ("Permission '%s' should not exist"
- " after deletion" % txt)
- assert not getter(perm), ("Permission '%s' should not exist"
- " after deletion" % txt)
+ assert not getter(), ("Empty permission set should not"
+ " have permission '%s'" % txt)
+ setter(True)
+ assert ps.test(perm), ("Permission '%s' should exist"
+ " after addition" % txt)
+ assert getter(), ("Permission '%s' should exist"
+ " after addition" % txt)
+ str_ps = str(ps)
+ self.checkRef(str_ps)
+ setter(False)
+ assert not ps.test(perm), ("Permission '%s' should not exist"
+ " after deletion" % txt)
+ assert not getter(), ("Permission '%s' should not exist"
+ " after deletion" % txt)
+ setter(True)
+ assert getter()
+ ps.clear()
+ assert not getter()
def test_permset_invalid_type(self):
acl = posix1e.ACL()
@@ -845,24 +856,24 @@ class TestModification:
with pytest.raises(ValueError):
ps.write = object() # type: ignore
- def test_qualifier_values(self):
+ @pytest.mark.parametrize("tag", [ACL_USER, ACL_GROUP],
+ ids=["ACL_USER", "ACL_GROUP"])
+ def test_qualifier_values(self, tag):
"""Tests qualifier correct store/retrieval"""
acl = posix1e.ACL()
e = acl.append()
- # work around deprecation warnings
- for tag in [posix1e.ACL_USER, posix1e.ACL_GROUP]:
- qualifier = 1
- e.tag_type = tag
- while True:
- regex = re.compile("(user|group) with (u|g)id %d" % qualifier)
- try:
- e.qualifier = qualifier
- except OverflowError:
- # reached overflow condition, break
- break
- assert e.qualifier == qualifier
- assert regex.search(str(e)) is not None
- qualifier *= 2
+ qualifier = 1
+ e.tag_type = tag
+ while True:
+ regex = re.compile("(user|group) with (u|g)id %d" % qualifier)
+ try:
+ e.qualifier = qualifier
+ except OverflowError:
+ # reached overflow condition, break
+ break
+ assert e.qualifier == qualifier
+ assert regex.search(str(e)) is not None
+ qualifier *= 2
def test_qualifier_overflow(self):
"""Tests qualifier overflow handling"""
--
2.39.5
From b6d75498c5c2765e093db3b1dac2062389395a5b Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Thu, 5 Dec 2019 21:28:36 +0100
Subject: [PATCH 07/16] Change setstate to only take bytes
This is the recommended way (well, via Py_buffer, but I don't need
that).
---
NEWS | 2 ++
acl.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/NEWS b/NEWS
index 43210c5..94e5183 100644
--- a/NEWS
+++ b/NEWS
@@ -29,6 +29,8 @@ Important API changes/bug fixes:
inadvertently removed due a typo(!) when adding support for it in
FreeBSD. Pickle should work again for ACL instances, although not sure
how stable this serialisation format actually is.
+- Additionally, slightly change `__setstate__()` input to not allow
+ Unicode, since the serialisation format is an opaque binary format.
- Fix (and change) entry qualifier (which is a user/group ID) behaviour:
assume/require that uid_t/gid_t are unsigned types (they are with
glibc, MacOS and FreeBSD at least; the standard doesn't document the
diff --git a/acl.c b/acl.c
index e88d143..139d5e5 100644
--- a/acl.c
+++ b/acl.c
@@ -503,7 +503,7 @@ static PyObject* ACL_set_state(PyObject *obj, PyObject* args) {
acl_t ptr;
/* Parse the argument */
- if (!PyArg_ParseTuple(args, "s#", &buf, &bufsize))
+ if (!PyArg_ParseTuple(args, "y#", &buf, &bufsize))
return NULL;
/* Try to import the external representation */
--
2.39.5
From a5d88b2b489c66b8e225593c1eaf4635990523db Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Thu, 5 Dec 2019 21:31:42 +0100
Subject: [PATCH 08/16] Make the code Py_ssize_t clean
Thanks python 3.8 for the hint!
---
acl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/acl.c b/acl.c
index 139d5e5..6f51a04 100644
--- a/acl.c
+++ b/acl.c
@@ -20,6 +20,7 @@
*/
+#define PY_SSIZE_T_CLEAN
#include <Python.h>
#include <sys/types.h>
@@ -499,7 +500,7 @@ static PyObject* ACL_get_state(PyObject *obj, PyObject* args) {
static PyObject* ACL_set_state(PyObject *obj, PyObject* args) {
ACL_Object *self = (ACL_Object*) obj;
const void *buf;
- int bufsize;
+ Py_ssize_t bufsize;
acl_t ptr;
/* Parse the argument */
--
2.39.5
From fdbe25ea85625dcc8a1e13256542eff0b085df84 Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Thu, 5 Dec 2019 21:32:02 +0100
Subject: [PATCH 09/16] Add a test for __setstate__ arguments
---
tests/test_acls.py | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tests/test_acls.py b/tests/test_acls.py
index eb01669..8de6aa2 100644
--- a/tests/test_acls.py
+++ b/tests/test_acls.py
@@ -486,6 +486,12 @@ class TestAclExtensions:
assert a == b
assert b != c
+ @require_copy_ext
+ def test_acl_copy_ext_args(self):
+ a = posix1e.ACL()
+ with pytest.raises(TypeError):
+ a.__setstate__(None)
+
class TestWrite:
"""Write tests"""
--
2.39.5
From f7a8453482995cbce0e55b1bdb4180af109519cc Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Wed, 11 Dec 2019 20:20:46 +0100
Subject: [PATCH 10/16] Small simplification in ACL_init
---
acl.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/acl.c b/acl.c
index 6f51a04..2bd05d8 100644
--- a/acl.c
+++ b/acl.c
@@ -130,15 +130,17 @@ static PyObject* ACL_new(PyTypeObject* type, PyObject* args,
/* Initialization of a new ACL instance */
static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
ACL_Object* self = (ACL_Object*) obj;
-#ifdef HAVE_LINUX
static char *kwlist[] = { "file", "fd", "text", "acl", "filedef",
- "mode", NULL };
- char *format = "|O&OsO!O&i";
- int mode = -1;
-#else
- static char *kwlist[] = { "file", "fd", "text", "acl", "filedef", NULL };
- char *format = "|O&OsO!O&";
+#ifdef HAVE_LINUX
+ "mode",
#endif
+ NULL };
+ char *format = "|O&OsO!O&"
+#ifdef HAVE_LINUX
+ "i"
+#endif
+ ;
+ int mode = -1;
PyObject *file = NULL;
PyObject *filedef = NULL;
char *text = NULL;
--
2.39.5
From f6ba9364c2e7c8adc05e08ec305249e5031b8fbe Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Wed, 11 Dec 2019 20:50:55 +0100
Subject: [PATCH 11/16] Implement creating an ACL directly from serialised form
---
NEWS | 4 ++++
acl.c | 21 +++++++++++++++++++++
tests/test_acls.py | 13 +++++++++++++
3 files changed, 38 insertions(+)
diff --git a/NEWS b/NEWS
index 94e5183..a6a0df1 100644
--- a/NEWS
+++ b/NEWS
@@ -40,6 +40,10 @@ Important API changes/bug fixes:
Minor improvements:
+- Added a `data` keyword argument to `ACL()`, which allows restoring an
+ ACL directly from a serialised form (as given by `__getstate__()`),
+ which should simplify some uses cases (`a = ACL(); a.__set
+ state__(â¦)`).
- When available, add the file path to I/O error messages, which should
lead to easier debugging.
- The test suite has changed to `pytest`, which allows increased
diff --git a/acl.c b/acl.c
index 2bd05d8..99f3e9a 100644
--- a/acl.c
+++ b/acl.c
@@ -133,11 +133,17 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
static char *kwlist[] = { "file", "fd", "text", "acl", "filedef",
#ifdef HAVE_LINUX
"mode",
+#endif
+#ifdef HAVE_ACL_COPY_EXT
+ "data",
#endif
NULL };
char *format = "|O&OsO!O&"
#ifdef HAVE_LINUX
"i"
+#endif
+#ifdef HAVE_ACL_COPY_EXT
+ "y#"
#endif
;
int mode = -1;
@@ -146,6 +152,8 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
char *text = NULL;
PyObject *fd = NULL;
ACL_Object* thesrc = NULL;
+ const void *buf = NULL;
+ Py_ssize_t bufsize;
if(!PyTuple_Check(args) || PyTuple_Size(args) != 0 ||
(keywds != NULL && PyDict_Check(keywds) && PyDict_Size(keywds) > 1)) {
@@ -159,6 +167,9 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
PyUnicode_FSConverter, &filedef
#ifdef HAVE_LINUX
, &mode
+#endif
+#ifdef HAVE_ACL_COPY_EXT
+ , &buf, &bufsize
#endif
))
return -1;
@@ -192,6 +203,14 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
#ifdef HAVE_LINUX
else if(mode != -1)
self->acl = acl_from_mode(mode);
+#endif
+#ifdef HAVE_ACL_COPY_EXT
+ else if(buf != NULL) {
+ if((self->acl = acl_copy_int(buf)) == NULL) {
+ PyErr_SetFromErrno(PyExc_IOError);
+ return -1;
+ }
+ }
#endif
else
self->acl = acl_init(0);
@@ -1259,6 +1278,8 @@ static char __ACL_Type_doc__[] =
" (e.g. ``mode=0644``); this is valid only when the C library\n"
" provides the ``acl_from_mode call``, and\n"
" note that no validation is done on the given value.\n"
+ ":param bytes data: creates an ACL from a serialised form,\n"
+ " as provided by calling ``__getstate__()`` on an existing ACL\n"
"\n"
"If no parameters are passed, an empty ACL will be created; this\n"
"makes sense only when your OS supports ACL modification\n"
diff --git a/tests/test_acls.py b/tests/test_acls.py
index 8de6aa2..28f55c9 100644
--- a/tests/test_acls.py
+++ b/tests/test_acls.py
@@ -492,6 +492,19 @@ class TestAclExtensions:
with pytest.raises(TypeError):
a.__setstate__(None)
+ @require_copy_ext
+ def test_acl_init_copy_ext(self):
+ a = posix1e.ACL(text=BASIC_ACL_TEXT)
+ b = posix1e.ACL()
+ c = posix1e.ACL(data=a.__getstate__())
+ assert c != b
+ assert c == a
+
+ @require_copy_ext
+ def test_acl_init_copy_ext_invalid(self):
+ with pytest.raises(IOError):
+ posix1e.ACL(data=b"foobar")
+
class TestWrite:
"""Write tests"""
--
2.39.5
From 63e2dd28eac912dcf1b55814106609138eba090b Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Wed, 11 Dec 2019 21:04:42 +0100
Subject: [PATCH 12/16] Fix yet another bug in ACL re-inistialisation
---
NEWS | 3 +++
acl.c | 36 +++++++++++++++++-------------------
tests/test_acls.py | 12 ++++++++++++
3 files changed, 32 insertions(+), 19 deletions(-)
diff --git a/NEWS b/NEWS
index a6a0df1..84ec3bb 100644
--- a/NEWS
+++ b/NEWS
@@ -25,6 +25,9 @@ Important API changes/bug fixes:
non-initialised object attribute access. Note ACL re-initialisation is
tricky and (still) leads to undefined behaviour of existing Entry
objects pointing to it.
+- Fix another bug in ACL re-initialisation where failures would result
+ in invalid objects; now failed re-initialisation does not touch the
+ original object.
- Restore `__setstate__`/`__getstate__` support on Linux; this was
inadvertently removed due a typo(!) when adding support for it in
FreeBSD. Pickle should work again for ACL instances, although not sure
diff --git a/acl.c b/acl.c
index 99f3e9a..c280967 100644
--- a/acl.c
+++ b/acl.c
@@ -146,6 +146,7 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
"y#"
#endif
;
+ acl_t new = NULL;
int mode = -1;
PyObject *file = NULL;
PyObject *filedef = NULL;
@@ -174,52 +175,49 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
))
return -1;
- /* Free the old acl_t without checking for error, we don't
- * care right now */
- if(self->acl != NULL)
- acl_free(self->acl);
-
- self->acl = NULL;
-
if(file != NULL) {
fprintf(stderr, "foobar!\n");
char *path = PyBytes_AS_STRING(file);
- self->acl = acl_get_file(path, ACL_TYPE_ACCESS);
+ new = acl_get_file(path, ACL_TYPE_ACCESS);
Py_DECREF(file);
} else if(text != NULL)
- self->acl = acl_from_text(text);
+ new = acl_from_text(text);
else if(fd != NULL) {
int fdval;
if ((fdval = PyObject_AsFileDescriptor(fd)) != -1) {
- self->acl = acl_get_fd(fdval);
+ new = acl_get_fd(fdval);
}
} else if(thesrc != NULL)
- self->acl = acl_dup(thesrc->acl);
+ new = acl_dup(thesrc->acl);
else if(filedef != NULL) {
char *path = PyBytes_AS_STRING(filedef);
- self->acl = acl_get_file(path, ACL_TYPE_DEFAULT);
+ new = acl_get_file(path, ACL_TYPE_DEFAULT);
Py_DECREF(path);
}
#ifdef HAVE_LINUX
else if(mode != -1)
- self->acl = acl_from_mode(mode);
+ new = acl_from_mode(mode);
#endif
#ifdef HAVE_ACL_COPY_EXT
else if(buf != NULL) {
- if((self->acl = acl_copy_int(buf)) == NULL) {
- PyErr_SetFromErrno(PyExc_IOError);
- return -1;
- }
+ new = acl_copy_int(buf);
}
#endif
else
- self->acl = acl_init(0);
+ new = acl_init(0);
- if(self->acl == NULL) {
+ if(new == NULL) {
PyErr_SetFromErrno(PyExc_IOError);
return -1;
}
+ /* Free the old acl_t without checking for error, we don't
+ * care right now */
+ if(self->acl != NULL)
+ acl_free(self->acl);
+
+ self->acl = new;
+
return 0;
}
diff --git a/tests/test_acls.py b/tests/test_acls.py
index 28f55c9..cc9a2e2 100644
--- a/tests/test_acls.py
+++ b/tests/test_acls.py
@@ -313,6 +313,18 @@ class TestLoad:
assert acl1.valid()
acl1.__init__(text=BASIC_ACL_TEXT) # type: ignore
assert acl1.valid()
+ acl2 = ACL(mode=0o755)
+ assert acl1 != acl2
+ acl1.__init__(acl=acl2) # type: ignore
+ assert acl1 == acl2
+
+ def test_entry_reinit_failure_noop(self):
+ a = posix1e.ACL(mode=0o0755)
+ b = posix1e.ACL(acl=a)
+ assert a == b
+ with pytest.raises(IOError):
+ a.__init__(text='foobar')
+ assert a == b
@pytest.mark.xfail(reason="Unreliable test, re-init doesn't always invalidate children")
def test_double_init_breaks_children(self):
--
2.39.5
From 76bb5a0ba8126952a5a3cf7338639d7fddb274c7 Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Wed, 11 Dec 2019 21:18:46 +0100
Subject: [PATCH 13/16] Add file path to error message on ACL initialisation
---
acl.c | 17 ++++++++++++++++-
tests/test_acls.py | 2 ++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/acl.c b/acl.c
index c280967..f96b679 100644
--- a/acl.c
+++ b/acl.c
@@ -155,6 +155,7 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
ACL_Object* thesrc = NULL;
const void *buf = NULL;
Py_ssize_t bufsize;
+ int set_err = 0;
if(!PyTuple_Check(args) || PyTuple_Size(args) != 0 ||
(keywds != NULL && PyDict_Check(keywds) && PyDict_Size(keywds) > 1)) {
@@ -179,6 +180,12 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
fprintf(stderr, "foobar!\n");
char *path = PyBytes_AS_STRING(file);
new = acl_get_file(path, ACL_TYPE_ACCESS);
+ // Set custom exception on this failure path which includes
+ // the filename.
+ if (new == NULL) {
+ PyErr_SetFromErrnoWithFilename(PyExc_IOError, path);
+ set_err = 1;
+ }
Py_DECREF(file);
} else if(text != NULL)
new = acl_from_text(text);
@@ -192,6 +199,12 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
else if(filedef != NULL) {
char *path = PyBytes_AS_STRING(filedef);
new = acl_get_file(path, ACL_TYPE_DEFAULT);
+ // Set custom exception on this failure path which includes
+ // the filename.
+ if (new == NULL) {
+ PyErr_SetFromErrnoWithFilename(PyExc_IOError, path);
+ set_err = 1;
+ }
Py_DECREF(path);
}
#ifdef HAVE_LINUX
@@ -207,7 +220,9 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
new = acl_init(0);
if(new == NULL) {
- PyErr_SetFromErrno(PyExc_IOError);
+ if (!set_err) {
+ PyErr_SetFromErrno(PyExc_IOError);
+ }
return -1;
}
diff --git a/tests/test_acls.py b/tests/test_acls.py
index cc9a2e2..55ba9e1 100644
--- a/tests/test_acls.py
+++ b/tests/test_acls.py
@@ -256,6 +256,8 @@ class TestLoad:
_, fname = get_file(testdir)
with pytest.raises(IOError):
posix1e.ACL(file="fname"+".no-such-file")
+ with pytest.raises(IOError):
+ posix1e.ACL(filedef="fname"+".no-such-file")
def test_from_invalid_fd(self, testdir):
fd, _ = get_file(testdir)
--
2.39.5
From 4f7e4902b70101ee7a245ed26730fe8aeee65480 Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Wed, 11 Dec 2019 21:26:59 +0100
Subject: [PATCH 14/16] Remove forgotten debug statement
Oops :)
---
acl.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/acl.c b/acl.c
index f96b679..1d74833 100644
--- a/acl.c
+++ b/acl.c
@@ -177,7 +177,6 @@ static int ACL_init(PyObject* obj, PyObject* args, PyObject *keywds) {
return -1;
if(file != NULL) {
- fprintf(stderr, "foobar!\n");
char *path = PyBytes_AS_STRING(file);
new = acl_get_file(path, ACL_TYPE_ACCESS);
// Set custom exception on this failure path which includes
--
2.39.5
From eadcf345669eb3aaf3185631e435556b8cf19a54 Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Wed, 11 Dec 2019 21:30:12 +0100
Subject: [PATCH 15/16] Remove obsolete check for non-initialised Entry
Today, objects are always initialised, so this check is superfluous,
and the get_tag_qualifier will properly handle errors from acl_*
functions, so even if that invariant is actually violated, this will
not lead to undefined behaviour.
---
acl.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/acl.c b/acl.c
index 1d74833..eb7d491 100644
--- a/acl.c
+++ b/acl.c
@@ -971,10 +971,6 @@ static PyObject* Entry_get_qualifier(PyObject *obj, void* arg) {
unsigned long value;
tag_qual tq;
- if (self->entry == NULL) {
- PyErr_SetString(PyExc_ValueError, "Can't get qualifier on uninitalized Entry object");
- return NULL;
- }
if(get_tag_qualifier(self->entry, &tq) < 0) {
return NULL;
}
--
2.39.5
From 80ab186e327e31d296d51b4e81523faae2c94df5 Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Wed, 11 Dec 2019 21:40:37 +0100
Subject: [PATCH 16/16] Tests: replace two mode= uses with text=
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit
The text=⦠argument is supported more widely (e.g. FreeBSD doesn't
support mode), so let's use that in tests for better coverage on
multiple platforms.
---
tests/test_acls.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/test_acls.py b/tests/test_acls.py
index 55ba9e1..8234c5c 100644
--- a/tests/test_acls.py
+++ b/tests/test_acls.py
@@ -40,6 +40,7 @@ from posix1e import *
TEST_DIR = os.environ.get("TEST_DIR", ".")
BASIC_ACL_TEXT = "u::rw,g::r,o::-"
+TEXT_0755 = "u::rwx,g::rx,o::rx"
# Permset permission information
PERMSETS = [
@@ -315,13 +316,13 @@ class TestLoad:
assert acl1.valid()
acl1.__init__(text=BASIC_ACL_TEXT) # type: ignore
assert acl1.valid()
- acl2 = ACL(mode=0o755)
+ acl2 = ACL(text=TEXT_0755)
assert acl1 != acl2
acl1.__init__(acl=acl2) # type: ignore
assert acl1 == acl2
def test_entry_reinit_failure_noop(self):
- a = posix1e.ACL(mode=0o0755)
+ a = posix1e.ACL(text=TEXT_0755)
b = posix1e.ACL(acl=a)
assert a == b
with pytest.raises(IOError):
--
2.39.5