LIBXML Data integrity fix, libxml must be locked across threads for any XML parsing (#4325)

2026-08-25 23:57:17 +00:00 · 2026-08-24 19:21:07 +02:00
parent f42da28dab
commit d11b584fe4
3 changed files with 252 additions and 18 deletions
@@ -663,12 +663,14 @@ class ValidateCSSJSONXPATHInput(object):
                    raise ValidationError("XPath not permitted in this field!")
                from lxml import etree, html
                import elementpath
-                from changedetectionio.html_tools import SafeXPath3Parser
-                tree = html.fromstring("<html></html>")
+                from changedetectionio.html_tools import SafeXPath3Parser, lxml_guard, lxml_html_parser
                line = line.replace('xpath:', '')

                try:
-                    elementpath.select(tree, line.strip(), parser=SafeXPath3Parser)
+                    # Runs on a Flask request thread - must share the worker's lxml lock.
+                    with lxml_guard():
+                        tree = html.fromstring("<html></html>", parser=lxml_html_parser())
+                        elementpath.select(tree, line.strip(), parser=SafeXPath3Parser)
                except elementpath.ElementPathError as e:
                    message = field.gettext('\'%(expression)s\' is not a valid XPath expression. (%(error)s)')
                    raise ValidationError(message % {'expression': line, 'error': str(e)})
@@ -679,11 +681,14 @@ class ValidateCSSJSONXPATHInput(object):
                if not self.allow_xpath:
                    raise ValidationError("XPath not permitted in this field!")
                from lxml import etree, html
-                tree = html.fromstring("<html></html>")
+                from changedetectionio.html_tools import lxml_guard, lxml_html_parser
                line = re.sub(r'^xpath1:', '', line)

                try:
-                    tree.xpath(line.strip())
+                    # Runs on a Flask request thread - must share the worker's lxml lock.
+                    with lxml_guard():
+                        tree = html.fromstring("<html></html>", parser=lxml_html_parser())
+                        tree.xpath(line.strip())
                except etree.XPathEvalError as e:
                    message = field.gettext('\'%(expression)s\' is not a valid XPath expression. (%(error)s)')
                    raise ValidationError(message % {'expression': line, 'error': str(e)})
@@ -1,4 +1,4 @@
-from functools import lru_cache
+from functools import lru_cache, wraps

 from loguru import logger
 from typing import List
@@ -6,6 +6,89 @@ import html
 import json
 import os
 import re
+import threading
+
+# ---------------------------------------------------------------------------------------
+# libxml2 (lxml) concurrency guard
+#
+# Watch checks run run_changedetection() on a shared ThreadPoolExecutor
+# (worker_pool.queue_executor), so extraction for different watches parses HTML in parallel
+# threads. A single check uses TWO different lxml parsers: xpath_filter() builds its own
+# etree.HTMLParser(), while html_to_text() -> inscriptis parses with lxml's process-global
+# html_parser. lxml locks per parser OBJECT, so many threads on the SAME parser serialise
+# safely - but that lock does not span two DIFFERENT parsers, and they still share libxml2's
+# interned-string dictionary. Concurrent parses corrupt it, and the rendered text of one
+# watch's page ends up inside another watch's snapshot.
+#
+# lxml's FAQ sanctions exactly two patterns: "the default parser (which is replicated for
+# each thread) or create a parser for each thread yourself". A parser per CALL is neither -
+# but note that a parser per THREAD does not fix this either (measured: still leaks), because
+# inscriptis picks its own parser internally and we cannot redirect it. Serialising is the
+# only configuration measured clean.
+#
+# Serialising is measured to be free: extraction is ~80ms mean / 204ms p95 over real data,
+# a ceiling of ~12 checks/sec against the ~0.5 checks/sec that browser fetches can feed.
+# It also HALVES peak RSS on large pages (481MB -> 232MB on a 3.85MB page across 7 threads),
+# because only one libxml2 document is ever live.
+#
+# EVERY lxml entry point in this process must go through here - a single unguarded parse in
+# any thread (Flask request, worker) is enough to reintroduce the corruption.
+#
+# This is a threading lock, not an asyncio one, and that is deliberate: nothing here is called
+# from a coroutine. The async workers hand run_changedetection() to a ThreadPoolExecutor
+# (worker.py: "Run change detection in executor to avoid blocking event loop"), the requests
+# fetcher does the same, and Flask is synchronous. If you ever call a guarded function directly
+# from a coroutine you will stall that worker's event loop for the duration of someone else's
+# parse - bounded and deadlock-free (the holder is pure CPU and never awaits), but avoid it.
+#
+# Re-entrant: these helpers call into each other (element_removal -> subtractive_xpath_selector).
+#
+# LXML_LOCK_DISABLED=true removes the guard. It exists so the corruption can be reproduced on
+# demand (see tests/test_lxml_concurrency.py) - it is NOT a tuning option, it reinstates a
+# data-integrity bug that silently writes one watch's page text into another watch's snapshot.
+def _build_lxml_lock():
+    from changedetectionio.strtobool import strtobool
+
+    if not strtobool(os.getenv('LXML_LOCK_DISABLED', 'false')):
+        return threading.RLock()
+
+    logger.warning("LXML_LOCK_DISABLED=true - lxml parsing is unguarded. Under concurrency this "
+                   "is known to leak one watch's rendered page text into another watch's "
+                   "snapshot. For testing only, never run this in production.")
+
+    class _NullLock:
+        def __enter__(self): return self
+        def __exit__(self, *a): return False
+
+    return _NullLock()
+
+
+_LXML_LOCK = _build_lxml_lock()
+
+
+def lxml_guarded(fn):
+    """Serialise a function's libxml2 work - parse, xpath traversal, tostring() and clear()
+    all touch the document, so the whole call is held, not just the parse."""
+    @wraps(fn)
+    def wrapper(*args, **kwargs):
+        with _LXML_LOCK:
+            return fn(*args, **kwargs)
+    return wrapper
+
+
+def lxml_guard():
+    """Context manager for code OUTSIDE this module that drives lxml directly, so it shares the
+    same lock (e.g. XPath validation in forms.py, which runs on Flask request threads)."""
+    return _LXML_LOCK
+
+
+def lxml_html_parser():
+    """A fresh parser for each call, freed immediately afterwards. Never lxml's process-global
+    default parser, which is shared across every thread."""
+    from lxml import etree
+    return etree.HTMLParser()
+
+# ---------------------------------------------------------------------------------------

 # HTML added to be sure each result matching a filter (.example) gets converted to a new line by Inscriptis
 TEXT_FILTER_LIST_LINE_SUFFIX = "<br>"
@@ -168,10 +251,11 @@ def subtractive_css_selector(css_selector, content):

    return str(soup)

+@lxml_guarded
 def subtractive_xpath_selector(selectors: List[str], html_content: str) -> str:
    from lxml import etree
-    # Parse the HTML content using lxml
-    html_tree = etree.HTML(html_content)
+    # Parse the HTML content using lxml. Own parser, not etree.HTML()'s process-global default.
+    html_tree = etree.fromstring(html_content, parser=lxml_html_parser())

    # First, collect all elements to remove
    elements_to_remove = []
@@ -265,6 +349,7 @@ def elementpath_tostring(obj):
    return str(obj)

 # Return str Utf-8 of matched rules
+@lxml_guarded
 def xpath_filter(xpath_filter, html_content, append_pretty_line_formatting=False, is_xml=False):
    """

@@ -277,7 +362,7 @@ def xpath_filter(xpath_filter, html_content, append_pretty_line_formatting=False
    from lxml import etree, html
    import elementpath

-    parser = etree.HTMLParser()
+    parser = lxml_html_parser()
    tree = None
    try:
        if is_xml:
@@ -338,10 +423,12 @@ def xpath_filter(xpath_filter, html_content, append_pretty_line_formatting=False

 # Return str Utf-8 of matched rules
 # 'xpath1:'
+@lxml_guarded
 def xpath1_filter(xpath_filter, html_content, append_pretty_line_formatting=False, is_xml=False):
    from lxml import etree, html

-    parser = None
+    # Own parser, not lxml's process-global default (which parser=None would select).
+    parser = lxml_html_parser()
    tree = None
    try:
        if is_xml:
@@ -687,14 +774,14 @@ def html_to_text(html_content: str, render_anchor_tag_content=False, is_rss=Fals
    """
    Convert HTML content to plain text using inscriptis.

-    Thread-Safety: This function uses inscriptis.get_text() which internally calls
-    lxml.html.fromstring() with the default parser. Testing with 50 concurrent threads
-    confirms this approach is thread-safe and produces deterministic output.
+    Thread-Safety: inscriptis.get_text() parses with lxml's process-global default parser.
+    That is fine on its own, but NOT alongside the xpath helpers' own parser - see the
+    _LXML_LOCK comment at the top of this module. So the get_text() call is guarded.

-    Alternative Approach Rejected: An explicit HTMLParser instance (thread-local or fresh)
-    would also be thread-safe, but was found to break change detection logic in subtle ways
-    (test_check_basic_change_detection_functionality). The default parser provides correct
-    and reliable behavior.
+    Do NOT "fix" this by giving get_text() its own parser instead. That was tried
+    (bee1130c6, reverted by 272e68ad2) and measured at ~10x WORSE cross-document leakage,
+    because then both sides use unlocked per-call parsers and lxml's per-parser lock stops
+    helping at all.
    """
    from inscriptis import get_text
    from inscriptis.model.config import ParserConfig
@@ -734,7 +821,9 @@ def html_to_text(html_content: str, render_anchor_tag_content=False, is_rss=Fals

        html_content = str(soup)

-    text_content = get_text(html_content, config=parser_config)
+    # Only the lxml part is guarded - the BeautifulSoup stripping above is pure Python.
+    with _LXML_LOCK:
+        text_content = get_text(html_content, config=parser_config)
    return text_content

 # Does LD+JSON exist with a @type=='product' and a .price set anywhere?
@@ -0,0 +1,140 @@
+"""
+Guards against cross-watch snapshot contamination in the filter/extract chain.
+
+Watch checks run run_changedetection() on a shared ThreadPoolExecutor, so different watches
+parse HTML in parallel threads. A single check mixes two lxml parsers - xpath_filter() has its
+own, html_to_text() -> inscriptis uses lxml's process-global one. Without html_tools' lxml lock
+those races corrupt libxml2 state and one document's rendered text lands in another's output.
+
+Reported by a hosted customer whose snapshots contained an entirely different university's staff
+directory concatenated ahead of their own, at double the normal size.
+
+These tests fail reliably (tens of failures, several whole-document leaks) with the lock removed.
+"""
+import threading
+from concurrent.futures import ThreadPoolExecutor
+
+import pytest
+
+from changedetectionio import html_tools
+
+THREADS = 12
+# The bug corrupted ~4.75% of extractions, so 600 jobs means ~28 expected failures with the
+# guard removed - enough that this cannot pass by luck. Paths that were never affected only
+# need enough volume to catch a future regression, hence JOBS_GUARD.
+JOBS = 600
+JOBS_GUARD = 200
+
+# Distinct, mutually-identifiable documents. Each row carries a marker unique to its document so
+# any foreign content is unambiguous - mirrors the real report, where the giveaway was another
+# university's email domain appearing in the snapshot.
+NAMES = ["hamline", "gwynedd", "thiel", "lbcc", "wabash", "baruch", "purdue", "butler"]
+
+
+def _make_doc(name, rows=120):
+    body = "\n".join(
+        f"<tr><td>Person {i} {name}</td><td>p{i}@{name}.example</td></tr>" for i in range(rows)
+    )
+    return (
+        f"<html><head><title>{name}</title>"
+        f"<script>var pad=\"{'z' * 1200}\";</script><style>.a{{color:red}}</style></head>"
+        f"<body><div class='ads'>AD-{name}</div>"
+        f"<div id='staff'><table class='sidearm-table'>{body}</table></div>"
+        f"<footer>END-{name}</footer></body></html>"
+    )
+
+
+DOCS = {n: _make_doc(n) for n in NAMES}
+
+
+def _assert_no_contamination(pipeline, jobs=JOBS):
+    """Run pipeline concurrently over distinct docs; output must be byte-identical to the
+    single-threaded result and must never contain another document's marker."""
+    expected = {n: pipeline(DOCS[n]) for n in NAMES}
+    problems = []
+    lock = threading.Lock()
+
+    def job(i):
+        name = NAMES[i % len(NAMES)]
+        out = pipeline(DOCS[name])
+        found = [o for o in NAMES if o != name and f"{o}.example" in out]
+        if found:
+            with lock:
+                problems.append(f"{name} snapshot contains {found} (len {len(out)}, "
+                                f"expected {len(expected[name])})")
+        elif out != expected[name]:
+            with lock:
+                problems.append(f"{name} non-deterministic output: len {len(out)}, "
+                                f"expected {len(expected[name])}")
+
+    with ThreadPoolExecutor(max_workers=THREADS) as pool:
+        list(pool.map(job, range(jobs)))
+
+    assert not problems, (
+        f"{len(problems)} of {jobs} concurrent extractions were corrupted "
+        f"(html_tools lxml guard broken?):\n  " + "\n  ".join(problems[:10])
+    )
+
+
+def test_xpath_filter_with_html_to_text_is_not_contaminated():
+    """The customer's exact filter shape - this is the combination that reproduced."""
+    def pipeline(doc):
+        return html_tools.html_to_text(
+            html_tools.xpath_filter("//table[contains(@class,'sidearm-table')]", doc,
+                                    append_pretty_line_formatting=True))
+    _assert_no_contamination(pipeline)
+
+
+def test_xpath1_filter_with_html_to_text_is_not_contaminated():
+    def pipeline(doc):
+        return html_tools.html_to_text(
+            html_tools.xpath1_filter("//table[contains(@class,'sidearm-table')]", doc,
+                                     append_pretty_line_formatting=True))
+    _assert_no_contamination(pipeline)
+
+
+def test_subtractive_xpath_with_html_to_text_is_not_contaminated():
+    def pipeline(doc):
+        return html_tools.html_to_text(html_tools.element_removal(['//div[@class="ads"]'], doc))
+    _assert_no_contamination(pipeline, jobs=JOBS_GUARD)
+
+
+def test_css_filters_with_html_to_text_are_not_contaminated():
+    """CSS goes through BeautifulSoup (pure Python, never libxml2) so this has always been
+    safe - kept so a future move of CSS filtering onto lxml cannot regress silently."""
+    def pipeline(doc):
+        return html_tools.html_to_text(
+            html_tools.include_filters("#staff", doc, append_pretty_line_formatting=True))
+    _assert_no_contamination(pipeline, jobs=JOBS_GUARD)
+
+
+def test_xpath_helpers_never_use_lxmls_global_default_parser():
+    """lxml's default parser object is shared process-wide; the xpath helpers must own theirs."""
+    from lxml import etree
+    import lxml.html
+
+    p1 = html_tools.lxml_html_parser()
+    assert isinstance(p1, etree.HTMLParser)
+    assert p1 is not lxml.html.html_parser
+    # Fresh each call, so libxml2 state is never carried between watches
+    assert html_tools.lxml_html_parser() is not p1
+
+
+def test_lxml_lock_is_enabled_by_default():
+    """LXML_LOCK_DISABLED is a reproduction switch, not a tuning option - the default must be on.
+    To see these tests fail (i.e. to confirm they still detect the bug), run:
+        LXML_LOCK_DISABLED=true pytest changedetectionio/tests/test_lxml_concurrency.py
+    """
+    assert isinstance(html_tools._LXML_LOCK, type(threading.RLock())), (
+        "lxml lock is not a real lock - watch snapshots can be contaminated with other "
+        "watches' page text"
+    )
+
+
+def test_lxml_guard_is_shared_and_reentrant():
+    """forms.py drives lxml on Flask threads and must contend with the workers, not run free."""
+    assert html_tools.lxml_guard() is html_tools._LXML_LOCK
+    # Re-entrant: element_removal -> subtractive_xpath_selector is one step from nesting
+    with html_tools.lxml_guard():
+        with html_tools.lxml_guard():
+            pass