From 27e8980a256e305e028a0afc381ac196c3e3ba3d Mon Sep 17 00:00:00 2001
From: Iustin Pop <iustin@k1024.org>
Date: Fri, 29 Nov 2019 14:28:06 +0100
Subject: [PATCH] Change Entry initialisation protocol

This fixes very large and significant bugs - segfaults and memory
leaks - that were present for uninitialised object, more precisely
created but not init'ed ones.

I spent quite a bit of time thinking back on forth how to fix this,
and from the two options of:

- check initialised status on all code paths, or
- don't ever allow invalid/un-initialised objects

The latter one seems the correct one, even though the Python C API
docs imply that doing actual stuff in `__new__` should be "rare".

Tests for reference leaks and wrong re-init added as well; these would
have caught at least memory leaks before.
---
 NEWS               |  9 +++++++++
 acl.c              | 32 +++++++++++++++++++-------------
 tests/test_acls.py | 16 ++++++++++++++++
 3 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/NEWS b/NEWS
index 9e8f2e8..389f7fd 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,15 @@ and new features, such as:
   when dealing with file names, removing custom code (with the
   associated benefits).
 
+Important API changes/bug fixes:
+
+- Initialisation protocol has been changed, to disallow uninitialised
+  objects; this means that `__new__` will always create valid objects,
+  to prevent the need for checking initialisation status in all code
+  paths; this also (implicitly) fixes memory leaks on re-initialisation
+  (calling `__init__(…)` on an existing object) and segfaults (!) on
+  non-initialised object attribute access.
+
 Additionally, test suite has changed to `pytest`.
 
 Version 0.5.4
diff --git a/acl.c b/acl.c
index 2556389..16004b5 100644
--- a/acl.c
+++ b/acl.c
@@ -709,14 +709,27 @@ static int get_tag_qualifier(acl_entry_t entry, tag_qual *tq) {
 static PyObject* Entry_new(PyTypeObject* type, PyObject* args,
                            PyObject *keywds) {
     PyObject* newentry;
+    Entry_Object* entry;
+    ACL_Object* parent = NULL;
+
+    if (!PyArg_ParseTuple(args, "O!", &ACL_Type, &parent))
+        return NULL;
 
     newentry = PyType_GenericNew(type, args, keywds);
 
-    if(newentry != NULL) {
-        ((Entry_Object*)newentry)->entry = NULL;
-        ((Entry_Object*)newentry)->parent_acl = NULL;
+    if(newentry == NULL) {
+        return NULL;
     }
 
+    entry = (Entry_Object*)newentry;
+
+    if(acl_create_entry(&parent->acl, &entry->entry) == -1) {
+        PyErr_SetFromErrno(PyExc_IOError);
+        Py_DECREF(newentry);
+        return NULL;
+    }
+    Py_INCREF(parent);
+    entry->parent_acl = (PyObject*)parent;
     return newentry;
 }
 
@@ -728,14 +741,11 @@ static int Entry_init(PyObject* obj, PyObject* args, PyObject *keywds) {
     if (!PyArg_ParseTuple(args, "O!", &ACL_Type, &parent))
         return -1;
 
-    if(acl_create_entry(&parent->acl, &self->entry) == -1) {
-        PyErr_SetFromErrno(PyExc_IOError);
+    if ((PyObject*)parent != self->parent_acl) {
+        PyErr_SetString(PyExc_ValueError,
+                        "Can't reinitialize with a different parent");
         return -1;
     }
-
-    self->parent_acl = (PyObject*)parent;
-    Py_INCREF(parent);
-
     return 0;
 }
 
@@ -833,10 +843,6 @@ static PyObject* Entry_get_tag_type(PyObject *obj, void* arg) {
     Entry_Object *self = (Entry_Object*) obj;
     acl_tag_t value;
 
-    if (self->entry == NULL) {
-        PyErr_SetString(PyExc_AttributeError, "entry attribute");
-        return NULL;
-    }
     if(acl_get_tag_type(self->entry, &value) == -1) {
         PyErr_SetFromErrno(PyExc_IOError);
         return NULL;
diff --git a/tests/test_acls.py b/tests/test_acls.py
index e170add..b827b8d 100644
--- a/tests/test_acls.py
+++ b/tests/test_acls.py
@@ -520,6 +520,22 @@ class TestModification:
         with pytest.raises(TypeError):
           posix1e.Entry(object())
 
+    def test_entry_reinitialisations(self):
+        a = posix1e.ACL()
+        b = posix1e.ACL()
+        e = posix1e.Entry(a)
+        e.__init__(a)
+        with pytest.raises(ValueError, match="different parent"):
+            e.__init__(b)
+
+    @NOT_PYPY
+    def test_entry_reinit_leaks_refcount(self):
+        acl = posix1e.ACL()
+        e = acl.append()
+        ref = sys.getrefcount(acl)
+        e.__init__(acl)
+        assert ref == sys.getrefcount(acl), "Uh-oh, ref leaks..."
+
     def test_delete(self):
         """Test delete Entry from the ACL"""
         acl = posix1e.ACL()
-- 
2.39.5