Never evict a live id registration (#356)

The shadow's id registry held no reference to the documents it recorded,
so entries could only be validated by a stored _c_doc address and the
dict was capped at 4096 to bound growth from dead documents. At the cap
it deleted an arbitrary entry, which could be a live document's: a
long-running process that keeps an early signed document and registers
ids for 4096 later ones silently stopped resolving that document's #id
references.

An entry now keeps a strong reference to its _Document. The key, the
document's address, can then never go stale — the entry itself keeps
that address occupied — so the _c_doc check is gone; and liveness
becomes decidable without weak references, which lxml's classes refuse:
when the registry holds the only reference to a document, nothing can
present that document to a binding again, so the entry is dead. Every
new registration first drops those entries, which bounds both the
registry and the documents it pins by the documents still in use. No
cap, and nothing reachable is ever evicted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: mxamin

developer.md

@@ -99,9 +99,8 @@ accepts an lxml element (`PyXmlSec_LxmlElementConverter`) either calls a
 `PyXmlSec_LxmlShadowBegin*` helper or is one of the four dual-body functions
 (`register_id`, `add_ids`, `encrypt_xml`, `decrypt`), which must consult
 `IsActive()` before touching a raw node; and `->_c_node` / `->_c_doc` appear
-only in those four, in the helpers' fast-path branches, and in the ID registry
-(which uses the addresses as keys only). The test scans `src/*.c`, so a raw
-access anywhere else fails the suite on both paths.
+only in those four and in the helpers' fast-path branches. The test scans
+`src/*.c`, so a raw access anywhere else fails the suite on both paths.
 
 ## How the reflection works
 
@@ -198,9 +197,12 @@ document identity (`RecordId`), and every `BeginDoc` replays them onto its
 copy so that `#id` references resolve during sign/verify/decrypt. The replay
 scans the whole copy for the recorded attribute names — a superset of the
 single-node registration on the raw path, mirroring what `xmlSecAddIDs` does
-from the root. The registry holds no strong references to documents (lxml's
-classes refuse weak references), so entries are validated by a stored
-`_c_doc` address and capped in size. The two bindings are the only places,
+from the root. lxml's classes refuse weak references, so an entry keeps a
+strong reference to its document instead: the key (the document's address)
+can then never go stale, and a document referenced by nothing but the
+registry is provably unreachable, so its entry — and the document with it —
+is dropped before the next registration. The registry therefore tracks the
+documents still in use and never evicts a live one. The two bindings are the only places,
 together with encrypt_xml/decrypt's replacement bodies, that branch on
 `IsActive()`.
 

src/lxml.c

@@ -491,20 +491,49 @@ void PyXmlSec_LxmlShadowDiscard(PyXmlSec_LxmlShadow* shadow) {
 // and every whole-document Begin replays them onto the private copy so that
 // #id references resolve during sign/verify/decrypt.
 //
-// The registry cannot hold strong references to lxml documents (that would
-// pin whole trees forever) and lxml's classes refuse weak references, so
-// entries are keyed by the _Document object's address with the underlying
-// xmlDoc pointer stored alongside as a staleness check: an entry is trusted
-// only while both addresses match, and is replaced when the address has been
-// reused by a different document. A size cap bounds growth from dead
-// documents whose addresses never get reused.
+// An entry is keyed by the _Document object's address and holds a strong
+// reference to that document, so the key can never go stale: the address
+// cannot be reused while the entry keeps the object alive. That reference
+// also makes liveness decidable without weak references (lxml's classes
+// refuse those): when the registry holds the *only* reference to a document,
+// nothing can hand that document to a binding again, so the entry is dead and
+// is dropped. Pruning runs before every new registration, which bounds the
+// registry — and the documents it pins — by the documents still in use. No
+// live registration is ever evicted.
 // ----------------------------------------------------------------------------
 
-#define PYXMLSEC_SHADOW_ID_REGISTRY_CAP 4096
+// Drops the entries of documents nobody but the registry still references.
+// Best effort: on an allocation failure the entries simply survive until the
+// next registration prunes them.
+static void PyXmlSec_LxmlShadowPruneIdRegistry(void) {
+    PyObject* dead;
+    PyObject* key;
+    PyObject* entry;
+    Py_ssize_t pos = 0;
+    Py_ssize_t i;
+
+    dead = PyList_New(0);
+    if (dead == NULL) {
+        PyErr_Clear();
+        return;
+    }
+    // The dict cannot be mutated while iterating it, so collect first.
+    while (PyDict_Next(PyXmlSec_LxmlShadowIdRegistry, &pos, &key, &entry)) {
+        if (Py_REFCNT(PyTuple_GET_ITEM(entry, 0)) == 1 && PyList_Append(dead, key) < 0) {
+            PyErr_Clear();
+            break;
+        }
+    }
+    for (i = 0; i < PyList_GET_SIZE(dead); ++i) {
+        if (PyDict_DelItem(PyXmlSec_LxmlShadowIdRegistry, PyList_GET_ITEM(dead, i)) < 0) {
+            PyErr_Clear();
+        }
+    }
+    Py_DECREF(dead);
+}
 
 int PyXmlSec_LxmlShadowRecordId(PyXmlSec_LxmlElementPtr element, const char* name, const char* ns) {
     PyObject* key = NULL;
-    PyObject* cdoc = NULL;
     PyObject* created = NULL;
     PyObject* spec = NULL;
     PyObject* entry;
@@ -513,36 +542,18 @@ int PyXmlSec_LxmlShadowRecordId(PyXmlSec_LxmlElementPtr element, const char* nam
     int result = -1;
 
     key = PyLong_FromVoidPtr((void*)element->_doc);
-    cdoc = PyLong_FromVoidPtr((void*)element->_doc->_c_doc);
-    if (key == NULL || cdoc == NULL) {
+    if (key == NULL) {
         goto DONE;
     }
 
     entry = PyDict_GetItem(PyXmlSec_LxmlShadowIdRegistry, key);  // borrowed
-    if (entry != NULL) {
-        int eq = PyObject_RichCompareBool(PyTuple_GET_ITEM(entry, 0), cdoc, Py_EQ);
-        if (eq < 0) {
-            goto DONE;
-        }
-        if (!eq) {
-            entry = NULL;  // the address was reused by another document
-        }
-    }
     if (entry == NULL) {
-        if (PyDict_Size(PyXmlSec_LxmlShadowIdRegistry) >= PYXMLSEC_SHADOW_ID_REGISTRY_CAP) {
-            PyObject* k;
-            PyObject* v;
-            Py_ssize_t pos = 0;
-            if (PyDict_Next(PyXmlSec_LxmlShadowIdRegistry, &pos, &k, &v)
-                && PyDict_DelItem(PyXmlSec_LxmlShadowIdRegistry, k) < 0) {
-                goto DONE;
-            }
-        }
+        PyXmlSec_LxmlShadowPruneIdRegistry();
         specs = PyList_New(0);
         if (specs == NULL) {
             goto DONE;
         }
-        created = PyTuple_Pack(2, cdoc, specs);
+        created = PyTuple_Pack(2, (PyObject*)element->_doc, specs);
         Py_DECREF(specs);
         if (created == NULL || PyDict_SetItem(PyXmlSec_LxmlShadowIdRegistry, key, created) < 0) {
             goto DONE;
@@ -563,7 +574,6 @@ int PyXmlSec_LxmlShadowRecordId(PyXmlSec_LxmlElementPtr element, const char* nam
 
 DONE:
     Py_XDECREF(key);
-    Py_XDECREF(cdoc);
     Py_XDECREF(created);
     Py_XDECREF(spec);
     return result;
@@ -596,33 +606,21 @@ static void PyXmlSec_LxmlShadowApplyIdSpec(xmlDocPtr doc, xmlNodePtr n, const xm
 // Replays the specs recorded for the shadow's live document onto the copy.
 static int PyXmlSec_LxmlShadowReplayIds(PyXmlSec_LxmlShadow* shadow) {
     PyObject* key;
-    PyObject* cdoc;
     PyObject* entry;
     PyObject* specs;
     Py_ssize_t i, n;
-    int eq;
 
     key = PyLong_FromVoidPtr((void*)shadow->element->_doc);
     if (key == NULL) {
         return -1;
     }
+    // The entry, if any, belongs to this very document: the registry's own
+    // reference keeps the address from being reused by another one.
     entry = PyDict_GetItem(PyXmlSec_LxmlShadowIdRegistry, key);  // borrowed
     Py_DECREF(key);
     if (entry == NULL) {
         return 0;
     }
-    cdoc = PyLong_FromVoidPtr((void*)shadow->element->_doc->_c_doc);
-    if (cdoc == NULL) {
-        return -1;
-    }
-    eq = PyObject_RichCompareBool(PyTuple_GET_ITEM(entry, 0), cdoc, Py_EQ);
-    Py_DECREF(cdoc);
-    if (eq < 0) {
-        return -1;
-    }
-    if (!eq) {
-        return 0;  // stale entry from a dead document at the same address
-    }
 
     specs = PyTuple_GET_ITEM(entry, 1);
     n = PyList_GET_SIZE(specs);

tests/test_ds.py

@@ -1,5 +1,7 @@
 import unittest
 
+from lxml import etree
+
 import xmlsec
 from tests import base
 
@@ -367,3 +369,28 @@ def test_set_enabled_key_data_bad_list(self):
         ctx.key = xmlsec.Key.from_file(self.path('rsakey.pem'), format=consts.KeyDataFormatPem)
         with self.assertRaisesRegex(TypeError, 'expected list of KeyData constants.'):
             ctx.set_enabled_key_data('foo')
+
+
+class TestIdRegistryLifetime(base.TestMemoryLeaks):
+    """Guards the lifetime rule of the shadow-mode id registry (issue #356)."""
+
+    # The document churn below is this test's point; the leak harness would repeat it ten times.
+    iterations = 0
+
+    # Far more documents than any bounded registry would have held at once.
+    OTHER_DOCUMENTS = 5000
+
+    def test_registration_survives_other_live_documents(self):
+        """Registering ids for other documents must never drop a live document's own registration."""
+        root = self.load_xml('sign4-out.xml')
+        ctx = xmlsec.SignatureContext()
+        ctx.register_id(root, 'ID')
+
+        # All kept alive, so none of these registrations may be traded for another.
+        others = [etree.fromstring(f'<Envelope ID="doc-{i}"/>') for i in range(self.OTHER_DOCUMENTS)]
+        for other in others:
+            ctx.register_id(other, 'ID')
+
+        sign = xmlsec.tree.find_node(root, consts.NodeSignature)
+        ctx.key = xmlsec.Key.from_file(self.path('rsapub.pem'), format=consts.KeyDataFormatPem)
+        ctx.verify(sign)  # resolves #ID through the registration made before the churn

tests/test_shadow_audit.py

@@ -32,17 +32,15 @@
     }
 )
 
-# Functions allowed to dereference lxml's raw node/document pointers: the dual bodies above,
-# the fast-path branches of the shadow helpers, and the ID registry (addresses used as keys).
+# Functions allowed to dereference lxml's raw node/document pointers: the dual bodies above
+# and the fast-path branches of the shadow helpers.
 RAW_ACCESS_ALLOWED = DUAL_BODY_FUNCTIONS | frozenset(
     {
         'PyXmlSec_LxmlShadowBegin',
         'PyXmlSec_LxmlShadowBeginDoc',
         'PyXmlSec_LxmlShadowBeginNewDoc',
         'PyXmlSec_LxmlShadowEnd',
         'PyXmlSec_LxmlShadowEndFind',
-        'PyXmlSec_LxmlShadowRecordId',
-        'PyXmlSec_LxmlShadowReplayIds',
     }
 )