From 707c0d4aba0b65b1bef78faff21f7081cc86ed0f Mon Sep 17 00:00:00 2001 From: Iustin Pop 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 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 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 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 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 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 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 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 #include @@ -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 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 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 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 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 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 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 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 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