From a34beacc4652c26f9d8c00d95f18f11299b9d294 Mon Sep 17 00:00:00 2001 From: Iustin Pop Date: Sat, 28 Jun 2014 14:13:32 +0200 Subject: [PATCH] Try to fix uid_t/gid_t usage in entry qualifiers The current code is very broken with regards to the casting between Python's integer type (either int in Py2 or the magic int/long in Py3) and the uid_t/gid_t POSIX types. This means that the code is broken outside "small" integer values. By using uid_t/gid_t as appropriate, we can fix most of the problem (at least as far as the new unittests are concerned). There's still no automatic printf format modifier for them, so the code hardcodes unsigned (which is what Linux/glibc defines them to), and also there's a unittest that expects negative values to fail when set. This should fix issue #3 (github). --- acl.c | 126 ++++++++++++++++++++++++++++++++++++---------- test/test_acls.py | 43 +++++++++++++++- 2 files changed, 140 insertions(+), 29 deletions(-) diff --git a/acl.c b/acl.c index 22f83fe..4811b39 100644 --- a/acl.c +++ b/acl.c @@ -672,6 +672,46 @@ static PyObject* ACL_append(PyObject *obj, PyObject *args) { /***** Entry type *****/ +typedef struct { + acl_tag_t tag; + union { + uid_t uid; + gid_t gid; + }; +} tag_qual; + +/* Helper function to get the tag and qualifier of an Entry at the + same time. This is "needed" because the acl_get_qualifier function + returns a pointer to different types, based on the tag value, and + thus it's not straightforward to get the right type. + + It sets a Python exception if an error occurs, and return 0 in this + case. If successful, the tag is set to the tag type, and the + qualifier (if any) to either the uid or the gid entry in the + tag_qual structure. +*/ +int get_tag_qualifier(acl_entry_t entry, tag_qual *tq) { + void *p; + + if(acl_get_tag_type(entry, &tq->tag) == -1) { + PyErr_SetFromErrno(PyExc_IOError); + return 0; + } + if (tq->tag == ACL_USER || tq->tag == ACL_GROUP) { + if((p = acl_get_qualifier(entry)) == NULL) { + PyErr_SetFromErrno(PyExc_IOError); + return 0; + } + if (tq->tag == ACL_USER) { + tq->uid = *(uid_t*)p; + } else { + tq->gid = *(gid_t*)p; + } + acl_free(p); + } + return 1; +} + /* Creation of a new Entry instance */ static PyObject* Entry_new(PyTypeObject* type, PyObject* args, PyObject *keywds) { @@ -725,31 +765,18 @@ static void Entry_dealloc(PyObject* obj) { /* Converts the entry to a text format */ static PyObject* Entry_str(PyObject *obj) { - acl_tag_t tag; - uid_t qualifier; - void *p; PyObject *format, *kind; Entry_Object *self = (Entry_Object*) obj; + tag_qual tq; - if(acl_get_tag_type(self->entry, &tag) == -1) { - PyErr_SetFromErrno(PyExc_IOError); + if(!get_tag_qualifier(self->entry, &tq)) { return NULL; } - if(tag == ACL_USER || tag == ACL_GROUP) { - if((p = acl_get_qualifier(self->entry)) == NULL) { - PyErr_SetFromErrno(PyExc_IOError); - return NULL; - } - qualifier = *(uid_t*)p; - acl_free(p); - } else { - qualifier = 0; - } format = MyString_FromString("ACL entry for "); if(format == NULL) return NULL; - switch(tag) { + switch(tq.tag) { case ACL_UNDEFINED_TAG: kind = MyString_FromString("undefined type"); break; @@ -763,10 +790,14 @@ static PyObject* Entry_str(PyObject *obj) { kind = MyString_FromString("the others"); break; case ACL_USER: - kind = MyString_FromFormat("user with uid %d", qualifier); + /* FIXME: here and in the group case, we're formatting with + unsigned, because there's no way to automatically determine + the signed-ness of the types; on Linux(glibc) they're + unsigned, so we'll go along with that */ + kind = MyString_FromFormat("user with uid %u", tq.uid); break; case ACL_GROUP: - kind = MyString_FromFormat("group with gid %d", qualifier); + kind = MyString_FromFormat("group with gid %u", tq.gid); break; case ACL_MASK: kind = MyString_FromString("the mask"); @@ -828,7 +859,11 @@ 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; - int uidgid; + long uidgid; + uid_t uid; + gid_t gid; + void *p; + acl_tag_t tag; if(value == NULL) { PyErr_SetString(PyExc_TypeError, @@ -846,7 +881,38 @@ static int Entry_set_qualifier(PyObject* obj, PyObject* value, void* arg) { return -1; } } - if(acl_set_qualifier(self->entry, (void*)&uidgid) == -1) { + /* Due to how acl_set_qualifier takes its argument, we have to do + this ugly dance with two variables and a pointer that will + point to one of them. */ + if(acl_get_tag_type(self->entry, &tag) == -1) { + PyErr_SetFromErrno(PyExc_IOError); + return -1; + } + uid = uidgid; + gid = uidgid; + switch(tag) { + case ACL_USER: + if((long)uid != uidgid) { + PyErr_SetString(PyExc_OverflowError, "cannot assign given qualifier"); + return -1; + } else { + p = &uid; + } + break; + case ACL_GROUP: + if((long)gid != uidgid) { + PyErr_SetString(PyExc_OverflowError, "cannot assign given qualifier"); + return -1; + } else { + p = &gid; + } + break; + default: + PyErr_SetString(PyExc_TypeError, + "can only set qualifiers on ACL_USER or ACL_GROUP entries"); + return -1; + } + if(acl_set_qualifier(self->entry, p) == -1) { PyErr_SetFromErrno(PyExc_IOError); return -1; } @@ -857,20 +923,26 @@ 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; - void *p; - int value; + long value; + tag_qual tq; if (self->entry == NULL) { PyErr_SetString(PyExc_AttributeError, "entry attribute"); return NULL; } - if((p = acl_get_qualifier(self->entry)) == NULL) { - PyErr_SetFromErrno(PyExc_IOError); + if(!get_tag_qualifier(self->entry, &tq)) { + return NULL; + } + if (tq.tag == ACL_USER) { + value = tq.uid; + } else if (tq.tag == ACL_GROUP) { + value = tq.gid; + } else { + PyErr_SetString(PyExc_TypeError, + "given entry doesn't have an user or" + " group tag"); return NULL; } - value = *(uid_t*)p; - acl_free(p); - return PyInt_FromLong(value); } diff --git a/test/test_acls.py b/test/test_acls.py index eea3f36..9928bf9 100644 --- a/test/test_acls.py +++ b/test/test_acls.py @@ -26,6 +26,7 @@ import os import tempfile import sys import platform +import re import posix1e from posix1e import * @@ -323,6 +324,33 @@ class ModificationTests(aclTest, unittest.TestCase): " after deletion" % pmap[perm]) + @has_ext(HAS_ACL_ENTRY and IS_PY_3K) + def testQualifierValues(self): + """Tests qualifier correct store/retrieval""" + acl = posix1e.ACL() + e = acl.append() + # work around deprecation warnings + if hasattr(self, 'assertRegex'): + fn = self.assertRegex + else: + fn = self.assertRegexpMatches + for tag in [posix1e.ACL_USER, posix1e.ACL_GROUP]: + 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) + try: + e.qualifier = qualifier + except OverflowError: + # reached overflow condition, break + break + self.assertEqual(e.qualifier, qualifier) + fn(str(e), regex) + qualifier *= 2 + @has_ext(HAS_ACL_ENTRY and IS_PY_3K) def testQualifierOverflow(self): """Tests qualifier overflow handling""" @@ -330,10 +358,21 @@ class ModificationTests(aclTest, unittest.TestCase): e = acl.append() qualifier = sys.maxsize * 2 for tag in [posix1e.ACL_USER, posix1e.ACL_GROUP]: - e.tag_type = posix1e.ACL_USER + e.tag_type = tag with self.assertRaises(OverflowError): e.qualifier = qualifier - print(e.qualifier) + + @has_ext(HAS_ACL_ENTRY and IS_PY_3K) + def testNegativeQualifier(self): + """Tests negative qualifier handling""" + # Note: this presumes that uid_t/gid_t in C are unsigned... + acl = posix1e.ACL() + e = acl.append() + for tag in [posix1e.ACL_USER, posix1e.ACL_GROUP]: + e.tag_type = tag + for qualifier in [-10, -5, -1]: + with self.assertRaises(OverflowError): + e.qualifier = qualifier if __name__ == "__main__": -- 2.39.5