Test and fix IO callback bindings

Turns out the `xmlSecAllIOCallbacks` pointer list yields the stored
callbacks in reverse, _and_ the default callbacks steal everything (
libxml2's `xmlFileMatch` is literally defined as `return(1)`! So we

 - Simplify our linked list of sets of Python callbacks to cons, rather
   than append
 - Don't bother trying to track interleaving default callbacks and Python
   callbacks, because `cur_cb_list_item` would get left in a bad state when
   the default matched.
 - Instead, drop all callbacks added prior to the default callbacks being
   added.

Tests-wise, there are a lot of failures relating to unreferenced objects
and/or memory leaks. However, as an example the top test is completely
commented out right now (i.e. doesn't do anything with the library), so
I'm a bit stuck as to how to grapple with that!

Author: ch3pjw

src/main.c

@@ -146,47 +146,34 @@ typedef struct CbList {
 } CbList;
 
 static CbList* registered_callbacks = NULL;
-static CbList* rcb_tail = NULL;
 
-static void RCBListAppend(CbList* cb_list_item) {
-    if (registered_callbacks == NULL) {
-      registered_callbacks = cb_list_item;
-    } else {
-      rcb_tail->next = cb_list_item;
-    }
-    rcb_tail = cb_list_item;
+static void RCBListCons(CbList* cb_list_item) {
+    cb_list_item->next = registered_callbacks;
+    registered_callbacks = cb_list_item;
 }
 
 static void RCBListClear() {
     CbList* cb_list_item = registered_callbacks;
     while (cb_list_item) {
-        Py_XDECREF(cb_list_item->match_cb);
-        Py_XDECREF(cb_list_item->open_cb);
-        Py_XDECREF(cb_list_item->read_cb);
-        Py_XDECREF(cb_list_item->close_cb);
+        Py_DECREF(cb_list_item->match_cb);
+        Py_DECREF(cb_list_item->open_cb);
+        Py_DECREF(cb_list_item->read_cb);
+        Py_DECREF(cb_list_item->close_cb);
         CbList* next = cb_list_item->next;
         free(cb_list_item);
         cb_list_item = next;
     }
     registered_callbacks = NULL;
-    rcb_tail = NULL;
 }
 
 // The currently executing set of Python callbacks:
-static CbList* cur_cb_list_item = NULL;
+static CbList* cur_cb_list_item;
 
 static int PyXmlSec_MatchCB(const char* filename) {
-    if (!cur_cb_list_item) {
-      cur_cb_list_item = registered_callbacks;
-    }
-    while (cur_cb_list_item && !cur_cb_list_item->match_cb) {
-        // Spool past any default callback placeholders executed since we were
-        // last called back:
-        cur_cb_list_item = cur_cb_list_item->next;
-    }
+    cur_cb_list_item = registered_callbacks;
     PyGILState_STATE state = PyGILState_Ensure();
     PyObject* args = Py_BuildValue("(y)", filename);
-    while (cur_cb_list_item && cur_cb_list_item->match_cb) {
+    while (cur_cb_list_item) {
         PyObject* result = PyObject_CallObject(cur_cb_list_item->match_cb, args);
         if (result && PyObject_IsTrue(result)) {
             Py_DECREF(result);
@@ -247,9 +234,6 @@ static int PyXmlSec_CloseCB(void* context) {
     Py_DECREF(result);
 
     PyGILState_Release(state);
-    // We reset `cur_cb_list_item` because we've finished processing the set of
-    // callbacks that was matched
-    cur_cb_list_item = NULL;
     return 0;
 }
 
@@ -263,31 +247,25 @@ static PyObject* PyXmlSec_PyIOCleanupCallbacks(PyObject *self) {
             PyXmlSec_MatchCB, PyXmlSec_OpenCB, PyXmlSec_ReadCB,
             PyXmlSec_CloseCB) < 0) {
         return NULL;
-    };
+    }
     RCBListClear();
     Py_RETURN_NONE;
 }
 
 static char PyXmlSec_PyIORegisterDefaultCallbacks__doc__[] = \
     "Register globally xmlsec's own default set of IO callbacks.";
 static PyObject* PyXmlSec_PyIORegisterDefaultCallbacks(PyObject *self) {
+    // NB: The default callbacks (specifically libxml2's `xmlFileMatch`) always
+    // match, and callbacks are called in the reverse order to that which they
+    // were added. So, there's no point in holding onto any previously registered
+    // callbacks, because they will never be run:
+    xmlSecIOCleanupCallbacks();
+    RCBListClear();
     if (xmlSecIORegisterDefaultCallbacks() < 0) {
         return NULL;
     }
-    // We place a nulled item on the callback list to represent whenever the
-    // default callbacks are going to be invoked:
-    CbList* cb_list_item = malloc(sizeof(CbList));
-    if (cb_list_item == NULL) {
-      return NULL;
-    }
-    cb_list_item->match_cb = NULL;
-    cb_list_item->open_cb = NULL;
-    cb_list_item->read_cb = NULL;
-    cb_list_item->close_cb = NULL;
-    cb_list_item->next = NULL;
-    RCBListAppend(cb_list_item);
-    // We need to make sure we can continue trying to match futher Python
-    // callbacks if the default callback doesn't match:
+    // We need to make sure we can continue trying to match any newly added
+    // Python callbacks:
     if (xmlSecIORegisterCallbacks(
             PyXmlSec_MatchCB, PyXmlSec_OpenCB, PyXmlSec_ReadCB,
             PyXmlSec_CloseCB) < 0) {
@@ -354,7 +332,7 @@ static PyObject* PyXmlSec_PyIORegisterCallbacks(PyObject *self, PyObject *args,
     Py_INCREF(cb_list_item->read_cb);
     Py_INCREF(cb_list_item->close_cb);
     cb_list_item->next = NULL;
-    RCBListAppend(cb_list_item);
+    RCBListCons(cb_list_item);
     // NB: We don't need to register the callbacks with `xmlsec` here, because
     // we've already registered our helper functions that will trawl through our
     // list of callbacks.

tests/test_main.py

@@ -1,4 +1,10 @@
 import xmlsec
+from xmlsec import constants as consts
+
+from io import BytesIO
+
+from hypothesis import given, strategies
+import pytest
 from tests import base
 
 
@@ -30,3 +36,131 @@ def test_set_base64_default_line_size_rejects_negative_values(self):
         with self.assertRaises(ValueError):
             xmlsec.base64_default_line_size(-1)
         self.assertEqual(xmlsec.base64_default_line_size(), size)
+
+
+class TestCallbacks(base.TestMemoryLeaks):
+    def setUp(self):
+        super().setUp()
+        xmlsec.cleanup_callbacks()
+
+    @given(funcs=strategies.lists(
+        strategies.sampled_from([
+            lambda: None
+            # xmlsec.cleanup_callbacks,
+            # xmlsec.register_default_callbacks,
+        ])
+    ))
+    def test_arbitrary_cleaning_and_default_callback_registration(self, funcs):
+        # FIXME: This test seems to detelct unreferenced objects and memory
+        # leaks even if it never does anything!
+        pass
+        # for f in funcs:
+        #     f()
+
+    def _sign_doc(self):
+        root = self.load_xml("doc.xml")
+        sign = xmlsec.template.create(
+            root,
+            c14n_method=consts.TransformExclC14N,
+            sign_method=consts.TransformRsaSha1
+        )
+        xmlsec.template.add_reference(
+            sign, consts.TransformSha1, uri="cid:123456")
+
+        ctx = xmlsec.SignatureContext()
+        ctx.key = xmlsec.Key.from_file(
+            self.path("rsakey.pem"), format=consts.KeyDataFormatPem
+        )
+        ctx.sign(sign)
+        return sign
+
+    def _expect_sign_failure(self):
+        exc_info = pytest.raises(xmlsec.Error, self._sign_doc)
+        self.assertEqual(exc_info.value.args, (1, 'failed to sign'))
+
+    def _register_mismatch_callbacks(self, match_cb=lambda filename: False):
+        xmlsec.register_callbacks(
+            match_cb,
+            lambda filename: None,
+            lambda none, buf: 0,
+            lambda none: None,
+        )
+
+    def _register_match_callbacks(self):
+        xmlsec.register_callbacks(
+            lambda filename: filename == b'cid:123456',
+            lambda filename: BytesIO(b'<html><head/><body/></html>'),
+            lambda bio, buf: bio.readinto(buf),
+            lambda bio: bio.close(),
+        )
+
+    def _find(self, elem, *tags):
+        for tag in tags:
+            elem = elem.find(
+                '{{http://www.w3.org/2000/09/xmldsig#}}{}'.format(tag))
+        return elem
+
+    def _verify_external_data_signature(self):
+        signature = self._sign_doc()
+        digest = self._find(
+            signature, 'SignedInfo', 'Reference', 'DigestValue'
+        ).text
+        self.assertEqual(digest, 'VihZwVMGJ48NsNl7ertVHiURXk8=')
+
+    def test_sign_external_data_no_callbacks_fails(self):
+        self._expect_sign_failure()
+
+    def test_sign_external_data_default_callbacks_fails(self):
+        xmlsec.register_default_callbacks()
+        self._expect_sign_failure()
+
+    def test_sign_external_data_no_matching_callbacks_fails(self):
+        self._register_mismatch_callbacks()
+        self._expect_sign_failure()
+
+    def test_sign_data_from_callbacks(self):
+        self._register_match_callbacks()
+        self._verify_external_data_signature()
+
+    @given(
+        num_prior_mismatches=strategies.integers(min_value=1, max_value=50),
+        num_post_mismatches=strategies.integers(min_value=1, max_value=50),
+    )
+    def test_sign_data_not_first_callback(
+            self, num_prior_mismatches, num_post_mismatches
+    ):
+        bad_match_calls = 0
+
+        def match_cb(filename):
+            nonlocal bad_match_calls
+            bad_match_calls += 1
+            False
+
+        for _ in range(num_post_mismatches):
+            self._register_mismatch_callbacks(match_cb)
+
+        self._register_match_callbacks()
+
+        for _ in range(num_prior_mismatches):
+            self._register_mismatch_callbacks()
+
+        self._verify_external_data_signature()
+        self.assertEqual(bad_match_calls, 0)
+
+    def test_failed_sign_because_default_callbacks(self):
+        mismatch_calls = 0
+
+        def mismatch_cb(filename):
+            nonlocal mismatch_calls
+            mismatch_calls += 1
+            False
+
+        # NB: These first two sets of callbacks should never get called,
+        # because the default callbacks always match beforehand:
+        self._register_match_callbacks()
+        self._register_mismatch_callbacks(mismatch_cb)
+        xmlsec.register_default_callbacks()
+        self._register_mismatch_callbacks(mismatch_cb)
+        self._register_mismatch_callbacks(mismatch_cb)
+        self._expect_sign_failure()
+        self.assertEqual(mismatch_calls, 2)