Skip to content

Commit ff6a0a6

Browse files
committed
Snapshot add_ids' scope, retire dead registrations (#356)
Two findings on the shadow-mode id registry. add_ids recorded one spec per attribute name over the *scope*, and the replay re-walked that scope when something was signed — so the ids a signature could resolve were the ones the tree carried by then, not the ones xmlSecAddIDs had registered at the call. An element appended to the scope afterwards became resolvable where the fast path had never registered it; and the walk applied one name at a time across the whole scope, where xmlSecAddIDs takes the elements in document order and the names within each element. With <Y B="v"/><X A="v"/> and add_ids(root, ['A', 'B']), the raw path signs Y and the shadow signed X: both happily, over different content. RecordIds now expands the scope where the registration happens: one spec per element carrying one of the names, element by element in document order, names in the caller's order. Specs are single-node throughout — AddIdsBelow, the replay's subtree walk and the duplicate check's XPath probe over a scope are gone — so the shadow registers exactly the attributes the caller registered, never one that appeared later. A registration also outlived its element. The registry holds the element proxy (lxml refuses weak references) and released the slot only with the whole document entry, so a document that registers and drops temporary elements grew without bound, and kept claiming their id values: registering a value a dropped element had carried raised "duplicated id." where the fast path, whose id entry dies with the attribute, accepts it. A slot is now retired when the registry holds the only reference to the proxy, the tree it hangs in is not its document's, and no other proxy remains anywhere in that tree — exactly when lxml frees such a subtree and libxml2 drops the id entries of the attributes in it. Vacated slots are reused, so neither the elements nor the list grow, and the sweep runs before every registration and before every duplicate check. What this does not reproduce is lxml clearing its own id entry whenever an element is *moved* — even within the one document, and for a whole subtree when an ancestor moves (verified against lxml's id hash). The registry cannot observe a move, so a #id the fast path stops resolving keeps resolving under the shadow, to the element it was registered for and never to another one. Documented with the other divergences. Cost, worst case (2000 id-bearing elements): add_ids 0.000 -> 0.035 s, the sign that follows 11 -> 14 ms, and 2000 register_id calls on that document 0.54 -> 1.18 s, each call now sweeping 2000 registrations. 328 passed / 6 skipped on the mismatch build, also at PYXMLSEC_TEST_ITERATIONS=50; 340 / 6 on the matched static wheel, plain and with PYXMLSEC_FORCE_SHADOW=1. 10k sign+verify with an element registered and dropped per iteration: RSS 26.2 -> 26.3 MiB, output byte-identical throughout; 20k registration churn on one document 25.9 -> 25.9 MiB, where it was 27.2 -> 34.0 before. Three new tests fail on the shadow path before the fix and pass on the raw path; two more guard against retiring a registration that is still live.
1 parent b86e624 commit ff6a0a6

5 files changed

Lines changed: 393 additions & 132 deletions

File tree

‎developer.md‎

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -217,18 +217,35 @@ Under the shadow they record the id-attribute specs in a registry keyed by
217217
document identity (`RegisterId`, `RecordIds`), and every `BeginDoc` replays them onto its
218218
copy so that `#id` references resolve during sign/verify/decrypt. An entry
219219
keeps the registered elements themselves, so a spec is replayed at exactly
220-
the node it was registered for — that node alone for `register_id`, its
221-
subtree for `add_ids`, the scope `xmlSecAddIDs` walks. Registering every
222-
matching attribute of the copy instead would be unsafe, not merely generous:
223-
an unrelated element sharing the id value would claim it first and a `#id`
224-
reference could then resolve to content the caller never registered. lxml's
220+
the node it was registered for. Registering every matching attribute of the
221+
copy instead would be unsafe, not merely generous: an unrelated element
222+
sharing the id value would claim it first and a `#id` reference could then
223+
resolve to content the caller never registered.
224+
225+
`add_ids` covers a whole scope, and that scope is walked **at the call**:
226+
`RecordIds` expands it into one spec per element carrying one of the names,
227+
element by element in document order and, within an element, in the order of
228+
the names — the registration `xmlSecAddIDs` makes, at the moment it makes it.
229+
Leaving the scope to be walked at the replay would register whatever the tree
230+
had become by the time something was signed: an element that grew the
231+
attribute, or joined the scope, after the call would resolve under the shadow
232+
where the fast path never registered it, and two elements claiming one value
233+
under different names would be ordered by name rather than by document
234+
order — a `#id` covering different content on the two paths. lxml's
225235
classes refuse weak references, so the entry keeps strong references (to the
226236
document and to those elements) instead: the key (the document's address) can
227237
then never go stale, and since every element proxy holds a reference to its
228238
document, a document whose reference count is exactly what the registry holds
229239
— and whose registered elements nothing else holds — is provably unreachable,
230240
so its entry, and the document with it, is dropped before the next
231-
registration. The registry therefore tracks the documents still in use and
241+
registration. Registrations are retired one by one on the same principle: a
242+
slot whose proxy the registry alone holds, hanging in a tree that is not its
243+
document's, cannot be reached again — lxml keeps an unlinked subtree only for
244+
as long as a proxy remains somewhere in it, and the fast path's id entry dies
245+
at exactly that moment too, when libxml2 frees the attribute. The slot is
246+
vacated (and reused by the next registration), so registering and dropping
247+
elements on a long-lived document neither grows the registry nor keeps their
248+
values claimed. The registry therefore tracks the documents still in use and
232249
never evicts a live one. The two bindings are the only places, together with
233250
encrypt_xml/decrypt's replacement bodies, that branch on `IsActive()`.
234251

@@ -238,8 +255,8 @@ the same call: `xmlGetID(doc, value) != attr` — the test that raises
238255
under the shadow. What lxml's own parse declared (a DTD id attribute, an
239256
`xml:id`) is read back through XPath's `id()`, the one door into lxml's id
240257
hash that passes nothing but strings and elements; what earlier
241-
`register_id`/`add_ids` calls claimed is read from the registry, a subtree
242-
spec through one XPath over its scope.
258+
`register_id`/`add_ids` calls claimed is read from the registry, spec by
259+
spec.
243260

244261
Both halves compare *attributes*, not elements: `<N xml:id="dup" ID="dup"/>`
245262
answers `N` to `id('dup')` whichever attribute is asked about, while the fast
@@ -292,6 +309,13 @@ All invisible to the documented API:
292309
list of ids), so such a value is compared against the registry alone; a
293310
registration whose element has since been adopted into another document
294311
claims nothing, as at replay;
312+
- a registration follows the element it was made for. lxml drops its own id
313+
entry whenever an element is *moved* — even within the one document, and
314+
for a whole subtree when an ancestor moves — which the registry cannot
315+
observe, so a `#id` the fast path stops resolving after such a move keeps
316+
resolving under the shadow. It resolves to the registered element, never to
317+
another one: the shadow registers exactly the attributes the caller
318+
registered;
295319
- `encrypt_xml` encrypts a *copy* of the template, so the caller's template
296320
proxy is not the returned element; a template attached in the target's own
297321
document is unlinked afterwards (keeping its tail text where libxml2 would

0 commit comments

Comments
 (0)