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] 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.2