diff --git a/changedetectionio/forms.py b/changedetectionio/forms.py index b2557b42..80e3c02c 100644 --- a/changedetectionio/forms.py +++ b/changedetectionio/forms.py @@ -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("") + 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("", 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("") + 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("", 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)}) diff --git a/changedetectionio/html_tools.py b/changedetectionio/html_tools.py index 8e3c392d..ae869c04 100644 --- a/changedetectionio/html_tools.py +++ b/changedetectionio/html_tools.py @@ -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 = "
" @@ -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? diff --git a/changedetectionio/tests/test_lxml_concurrency.py b/changedetectionio/tests/test_lxml_concurrency.py new file mode 100644 index 00000000..04f64d03 --- /dev/null +++ b/changedetectionio/tests/test_lxml_concurrency.py @@ -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"Person {i} {name}p{i}@{name}.example" for i in range(rows) + ) + return ( + f"{name}" + f"" + f"
AD-{name}
" + f"
{body}
" + f"" + ) + + +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