Skip to content

Commit 66a6742

Browse files
committed
Guard the shadow invariants with a source-level test (#356)
An audit of every binding that accepts an lxml element (30 of them) confirmed that all of them run their xmlsec call on a shadow copy when the shadow mode is on: 26 through the Begin/End helpers, and four (register_id, add_ids, encrypt_xml, decrypt) through an explicit IsActive() switch whose raw body only runs on matched libxml2. Nothing was left to convert, but the invariant was enforced by review alone: on matched libraries a binding that bypassed the switch would still pass the test suite. tests/test_shadow_audit.py now scans src/*.c and fails when a function that takes an lxml element neither calls a PyXmlSec_LxmlShadowBegin* helper nor is one of the listed dual-body functions, when a dual-body function touches a raw node before consulting IsActive(), or when ->_c_node / ->_c_doc appears outside the allowlisted functions (the dual bodies, the helpers' fast-path branches and the ID registry). Two tests prove the checker flags synthetic bad code and one guards the scanner against a broken regex. developer.md states the rules.
1 parent 8a487f2 commit 66a6742

2 files changed

Lines changed: 160 additions & 0 deletions

File tree

developer.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,15 @@ Rules every call site must keep:
9494
in between (the `Py_*_ALLOW_THREADS` pair is fine — the call is pure C);
9595
- call exactly one End function after a successful Begin.
9696

97+
**Invariant, enforced by `tests/test_shadow_audit.py`:** every C function that
98+
accepts an lxml element (`PyXmlSec_LxmlElementConverter`) either calls a
99+
`PyXmlSec_LxmlShadowBegin*` helper or is one of the four dual-body functions
100+
(`register_id`, `add_ids`, `encrypt_xml`, `decrypt`), which must consult
101+
`IsActive()` before touching a raw node; and `->_c_node` / `->_c_doc` appear
102+
only in those four, in the helpers' fast-path branches, and in the ID registry
103+
(which uses the addresses as keys only). The test scans `src/*.c`, so a raw
104+
access anywhere else fails the suite on both paths.
105+
97106
## How the reflection works
98107

99108
`Begin` serializes the element with lxml's own `etree.tostring`, re-parses

tests/test_shadow_audit.py

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
"""Source-level guard for the shadow-copy invariants (issue #356).
2+
3+
The extension may hand lxml's raw libxml2 nodes to xmlsec only on the fast path, when both
4+
link the same libxml2. ``developer.md`` describes the design; this module scans ``src/*.c`` and
5+
checks the two rules that keep every binding on the shadow path whenever it is on:
6+
7+
1. every C function that accepts an lxml element (it uses ``PyXmlSec_LxmlElementConverter``)
8+
either runs its xmlsec call through a ``PyXmlSec_LxmlShadowBegin*`` helper, or is one of
9+
the dual-body functions in ``DUAL_BODY_FUNCTIONS``, which must consult
10+
``PyXmlSec_LxmlShadowIsActive()`` before touching a raw node;
11+
2. raw node access (``->_c_node`` / ``->_c_doc``) appears only inside the functions listed in
12+
``RAW_ACCESS_ALLOWED``.
13+
14+
Adding a function to either list is a deliberate design decision; see developer.md.
15+
"""
16+
17+
import glob
18+
import os
19+
import re
20+
import unittest
21+
from collections.abc import Iterator
22+
23+
SRC_DIR = os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))), 'src')
24+
25+
# Bindings with a raw body for the fast path and a shadow body behind IsActive().
26+
DUAL_BODY_FUNCTIONS = frozenset(
27+
{
28+
'PyXmlSec_SignatureContextRegisterId',
29+
'PyXmlSec_TreeAddIds',
30+
'PyXmlSec_EncryptionContextEncryptXml',
31+
'PyXmlSec_EncryptionContextDecrypt',
32+
}
33+
)
34+
35+
# Functions allowed to dereference lxml's raw node/document pointers: the dual bodies above,
36+
# the fast-path branches of the shadow helpers, and the ID registry (addresses used as keys).
37+
RAW_ACCESS_ALLOWED = DUAL_BODY_FUNCTIONS | frozenset(
38+
{
39+
'PyXmlSec_LxmlShadowBegin',
40+
'PyXmlSec_LxmlShadowBeginDoc',
41+
'PyXmlSec_LxmlShadowBeginNewDoc',
42+
'PyXmlSec_LxmlShadowEnd',
43+
'PyXmlSec_LxmlShadowEndFind',
44+
'PyXmlSec_LxmlShadowRecordId',
45+
'PyXmlSec_LxmlShadowReplayIds',
46+
}
47+
)
48+
49+
# A function definition at column 0: `[static ]<type>[*] PyXmlSec_<Name>(`; prototypes end in ';'.
50+
FUNCTION_DEF = re.compile(r'^(?:static\s+)?[\w\s]+?\**\s*\**(PyXmlSec_\w+)\s*\(')
51+
RAW_ACCESS = re.compile(r'->_c_(?:node|doc)\b')
52+
BEGIN_CALL = re.compile(r'\bPyXmlSec_LxmlShadowBegin\w*\s*\(')
53+
CONVERTER = 'PyXmlSec_LxmlElementConverter'
54+
IS_ACTIVE = 'PyXmlSec_LxmlShadowIsActive()'
55+
56+
57+
def _functions(source: str) -> Iterator[tuple[str, list[str]]]:
58+
"""Yields (name, [lines]) for every PyXmlSec_* function defined in the C source."""
59+
name = None
60+
body: list[str] = []
61+
for line in source.splitlines():
62+
match = FUNCTION_DEF.match(line)
63+
if match and not line.rstrip().endswith(';'):
64+
if name is not None:
65+
yield name, body
66+
name, body = match.group(1), []
67+
elif name is not None:
68+
body.append(line)
69+
if name is not None:
70+
yield name, body
71+
72+
73+
def violations(source: str, filename: str = '<source>') -> list[str]:
74+
"""Returns a description of every rule violation in one C source file."""
75+
found: list[str] = []
76+
for name, body in _functions(source):
77+
where = f'{filename}:{name}'
78+
raw_lines = [i for i, line in enumerate(body) if RAW_ACCESS.search(line)]
79+
active_lines = [i for i, line in enumerate(body) if IS_ACTIVE in line]
80+
takes_element = any(CONVERTER in line for line in body)
81+
begins = any(BEGIN_CALL.search(line) for line in body)
82+
83+
if name in DUAL_BODY_FUNCTIONS:
84+
if not active_lines:
85+
found.append(f'{where}: dual-body function never consults {IS_ACTIVE}')
86+
elif raw_lines and raw_lines[0] < active_lines[0]:
87+
found.append(f'{where}: raw node access before {IS_ACTIVE}')
88+
elif takes_element and not begins:
89+
found.append(f'{where}: takes an lxml element but never calls a PyXmlSec_LxmlShadowBegin* helper')
90+
91+
if raw_lines and name not in RAW_ACCESS_ALLOWED:
92+
found.append(f'{where}: raw node access (->_c_node / ->_c_doc) outside the allowed functions')
93+
return found
94+
95+
96+
@unittest.skipUnless(os.path.isdir(SRC_DIR), 'C sources not available (installed package)')
97+
class TestShadowAudit(unittest.TestCase):
98+
def sources(self) -> Iterator[tuple[str, str]]:
99+
files = sorted(glob.glob(os.path.join(SRC_DIR, '*.c')))
100+
self.assertTrue(files, f'no C sources under {SRC_DIR}')
101+
for path in files:
102+
with open(path, encoding='utf-8') as f:
103+
yield os.path.basename(path), f.read()
104+
105+
def test_scanner_sees_the_bindings(self) -> None:
106+
# guards the scanner itself: a broken regex would make the sources look clean
107+
names: set[str] = set()
108+
raw_files: set[str] = set()
109+
for filename, source in self.sources():
110+
for name, body in _functions(source):
111+
if any(CONVERTER in line for line in body):
112+
names.add(name)
113+
if any(RAW_ACCESS.search(line) for line in body):
114+
raw_files.add(filename)
115+
self.assertGreaterEqual(len(names), 30)
116+
self.assertTrue(names.issuperset({'PyXmlSec_TemplateAddReference', 'PyXmlSec_SignatureContextSign'}))
117+
self.assertTrue(names.issuperset(DUAL_BODY_FUNCTIONS))
118+
self.assertEqual({'ds.c', 'enc.c', 'lxml.c', 'tree.c'}, raw_files)
119+
120+
def test_every_binding_goes_through_the_shadow(self) -> None:
121+
found: list[str] = []
122+
for filename, source in self.sources():
123+
found.extend(violations(source, filename))
124+
self.assertEqual([], found, '\n'.join(found))
125+
126+
def test_checker_flags_a_raw_binding(self) -> None:
127+
bad = (
128+
'static PyObject* PyXmlSec_Bad(PyObject* self, PyObject* args) {\n'
129+
' PyXmlSec_LxmlElementPtr node = NULL;\n'
130+
' if (!PyArg_ParseTuple(args, "O&:bad", PyXmlSec_LxmlElementConverter, &node)) return NULL;\n'
131+
' return (PyObject*)PyXmlSec_elementFactory(node->_doc, xmlSecFindChild(node->_c_node, NULL, NULL));\n'
132+
'}\n'
133+
)
134+
found = violations(bad, 'bad.c')
135+
self.assertEqual(2, len(found), found)
136+
self.assertIn('never calls a PyXmlSec_LxmlShadowBegin*', found[0])
137+
self.assertIn('outside the allowed functions', found[1])
138+
139+
def test_checker_flags_raw_access_before_the_switch(self) -> None:
140+
bad = (
141+
'static PyObject* PyXmlSec_TreeAddIds(PyObject* self, PyObject* args) {\n'
142+
' PyXmlSec_LxmlElementPtr node = NULL;\n'
143+
' if (!PyArg_ParseTuple(args, "O&:add_ids", PyXmlSec_LxmlElementConverter, &node)) return NULL;\n'
144+
' xmlDocPtr doc = node->_doc->_c_doc;\n'
145+
' if (PyXmlSec_LxmlShadowIsActive()) Py_RETURN_NONE;\n'
146+
' return NULL;\n'
147+
'}\n'
148+
)
149+
found = violations(bad, 'bad.c')
150+
self.assertEqual(1, len(found), found)
151+
self.assertIn('raw node access before', found[0])

0 commit comments

Comments
 (0)