Repository navigation
fix: keep validation time linear on deeply nested or repeated input - #10
Merged
Merged
Conversation
lint_st rescanned the whole block stack for every token: the conditional and in-loop checks, the EXIT/CONTINUE check, the scope lookup and the closer search when nothing matched. "IF a THEN " repeated 32000 times took 24 s and doubling the depth quadrupled the time. Keep a per-keyword count of the open blocks, updated where frames are pushed and where a closer discards them, and give every frame a pointer to its nearest scope frame at push time. Findings and analysis fields are unchanged.
… it carries A ratio test over four deeply nested inputs (one per rescan site) in the style of the XML text-join test, plus behaviour tests for conditional/in-loop/EXIT state after closers, closers that discard unclosed blocks, and declaration scope through nested blocks.
|
Caution Review failedThe pull request is closed. Walkthrough
Change: Bug fix Merge Risk:
 |
A scope frame's owner is the frame itself, so the generated __eq__ recursed without end (RecursionError) when comparing two distinct scope frames. Nothing compares frames today; mark the field compare=False to remove the trap. No test: _Frame is internal and no current code path reaches the comparison.
Measure the XML text-join and ST nesting scaling tests with time.process_time(): both are single-thread CPU-bound, so process time drops scheduler noise on a shared runner. Best-of-3, floor and threshold are unchanged. Add two tests that pin set membership nobody tested: a timer called only inside CASE gives ST030, and EXIT inside REPEAT gives no ST005. They also pass on the previous st.py.
An STSource inside nested FBSource elements was linted once per enclosing FBSource, so findings and memory multiplied with the nesting depth.
Every BodyContent rescanned its whole subtree for ids and Connection elements, so nested BodyContent elements cost time quadratic in the depth. Each node now counts for its nearest enclosing BodyContent; files with non-nested bodies give identical findings.
Each opener that never closed rescanned the rest of the text, so a file of
unclosed (*, /* or { openers took time quadratic in its size. Scanning with
str.find stops at the first opener with no closer, and the output is
byte-identical to the regexes.
A closer matching no open block walked the whole stack of open blocks, so a file of many open blocks and unmatched closers took quadratic time. A count of open frames per keyword, as in the ST linter, skips the scan.
… stack check_simatic_sd had the same per-closer stack scan as the L5K checker: a closer matching no open NETWORK or RUNG walked the whole stack. Count the open frames per keyword instead.
Each ST routine was padded with one newline per file line before it, so a file with many ST routines cost time quadratic in its size. lint_st now takes the file line of the text's first line; findings are unchanged, message text included.
…d it Every RLL routine iterated its whole subtree for rungs, so rungs under nested Routines elements were checked once per enclosing routine: time and findings grew with the nesting depth. A nested Routines element is now its own scope; files without nested Routines give identical findings.
Every CompileUnit collected the network payloads of its whole subtree, so payloads under nested CompileUnit elements were checked once per enclosing unit: time and findings grew with the nesting depth. A nested CompileUnit is now its own scope; files without nested units give identical findings.
Each PLCopenXMLInterface link parsed the target file and walked it again, so n links to one file (including the AutomationML file itself) cost n parses. The globalId and uuid values are now collected once per file, keyed by the resolved path; a malformed file is cached as such.
…tion Every `NAME(` scanned on to its matching parenthesis and rebuilt its operand text, so nested or unclosed instructions cost time quadratic in the rung length, and a long identifier rescanned itself from every character. Operands are now read from one pass over the rung. An OTE operand that holds a `(` is a nested instruction, not a tag, and is no longer recorded as a coil: copies of every enclosing operand took memory quadratic in the nesting depth (20 KB of `OTE(` held 40 MB).
The Control Expert scaling tests put STSource directly under FBSource; Control Expert writes FBSource > FBProgram > STSource, so dropping the child walk in the nested-source finder went unnoticed. Build the real shape, add a DFB body with an unclosed IF, two sources in document order and a source under two wrappers. Add tests for the operand-count depth and comma arithmetic in the Logix rung reader, the document order of rungs and SimaticML payloads, and loosen the TwinCAT search-count spy to "at most 3". Reference the private TwinCAT helpers as module attributes so reverting one module fails its own tests instead of the import of the whole test module. tokenize() takes first_line explicitly: its only caller always passes it.
…TING test_linear_time.py had reached 574 lines. Move the ratio helper and the XML builders to linear_support.py (not a test module, like codesys_synth.py) and the tests to test_linear_xml.py (Control Expert, IEC 61131-10, L5X, SimaticML), test_linear_aml.py and test_linear_text.py (TwinCAT, L5K, SIMATIC SD, rungs). AutomationML gets its own file so that none of them passes about 300 lines. The tests are unchanged. The tests table in CONTRIBUTING.md lists the new files and test_codesys_hardening.py, which it omitted.
The one-pass operand reader dropped every OTE operand that held a `(`, so OTE(Arr[(i+1)]) written twice lost its LX040 double-coil warning. Drop an operand only when a `NAME(` starts inside it: that is a nested call, and keeping a copy of it per enclosing instruction is what grew with the square of the nesting depth. Matches do not overlap, so only the next one can lie inside an operand; the memory stays linear (5000 nested OTE( peak about 2 MB, where the rescan held 50 MB).
Expat copies a namespace URI into every element name and namespaced attribute name under its declaration, so one long URI costs its length times the element count: a 36 KB file with a 20,000-character default namespace and 4000 empty children peaked at 81 MB, nested elements at 62 MB. A cap in a namespace-declaration handler is too late, because the declaring element's own attributes are already expanded. Run a first pass without namespace processing that rejects an xmlns or xmlns:* attribute longer than 256 characters (the longest URI the checkers know is 77) with XML001, before the real parse. That pass still expands entities in attribute values, so it is also where entity declarations are refused: they precede the root element, and the real parse never sees one. The pass costs about 15 percent more on a 4 MB file.
A name from the file is quoted in the context of every finding under it (a Logix program name in each rung finding, a POU name in each body finding), so the printed output grew with the square of the file size: a 2,000-character program name over 40 rungs printed 85 KB, a 20,000-character one over 400 rungs 8 MB. Finding.__post_init__ cuts message and context to MAX_FINDING_TEXT, with an ellipsis inside the limit. Every finding is built through it, including those re-anchored by with_context(). The longest message the tests produce is 195 characters, the longest context 26, and the longest message template 183 plus its two names, so ordinary output is unchanged. The same files now print 21 KB and 206 KB.
The check rebuilt a set of the (family, version) pairs it had seen for every namespaced element: k namespaces over m elements cost k times m (1000 over 10,000 elements, 158 KB, took 0.35 s; it now takes 0.006 s). Keep a dict from pair to the line of its first element instead. The SM003 findings were sorted by line over a set, so two namespaces on one line came out in an order that changed with the hash seed. They now follow document order.
…s safe The linked-file cache was keyed by the resolved path, which misses another spelling of the same file on a case-insensitive file system and a hard link: 800 links to a file under 32 case spellings parsed it 32 times (0.17 s), and under 800 spellings once each. Key it by (st_dev, st_ino) and fall back to the resolved path when the file system reports no inode (0); the same links now parse once (0.02 s). Check refURI and ExternalReference targets with os.path.exists(): on Python 3.9 Path.exists() raises ENAMETOOLONG for a path component over the file system's limit, which crashed the validator instead of giving AM011/AM013. Rename the local `ids` in check_aml's link loop to `target_ids`; it shadowed the dict of IDs above.
Add a test that a namespace declaration defaulted by an ATTLIST is capped too (no entity involved; expat reports a defaulted attribute to the start handler). Correct three comments to what was measured: the longest message from an ordinary name in the tests is 203 characters (not 195); the distinct-attribute input is 29 KB (not 33 KB); Path.exists() raises ENAMETOOLONG on Python 3.9.2 and 3.12.3 and answers False on 3.14.8.
This reverts commit 40e0958.
…ong names safe" This reverts commit 0619029.
This reverts commit 68b4632.
This reverts commit 900bbe3.
This reverts commit cdc6d23.
SmailG
changed the title
SmailG
marked this pull request as ready for review
November 10, 2026 16:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Validation time grew with the square of the nesting depth (or of the number of repeated openers) in several checkers, so a crafted file made the validator spin: for example
IF a THENrepeated 32,000 times took 23.8 s in the ST linter. Most of these sites rescanned a stack or a subtree per token or per element. A nested Control ExpertFBSourcealso multiplied the findings and the memory by its depth.Change
The ST linter keeps a per-keyword count of open blocks and a pointer to the nearest scope, so "is any
IF/loop open" and the scope lookup are O(1); a closer that matches nothing no longer scans the stack. The same idea is applied where the same shape showed up:FBSource: eachSTSourceis linted once, under its nearest enclosingFBSource(before, once per enclosing one).str.find, byte-identical to the old regexes.lint_sttakesfirst_line).Routines, SimaticML nestedCompileUnit: one walk, each rung or unit attributed to its nearest scope.check_rung: operand extraction is linear.Version 0.2.1 to 0.2.2 in both manifests (a test checks they agree). The tests that cover this are listed in
CONTRIBUTING.md.Measured (CPU seconds; before is v0.2.1)
IF a THENrepeatedFBSourcearound a 1000-token STBodyContent(*PROGRAMx n thenEND_ZZx ncheck_rung, nestedXIC(Behaviour on legitimate input
Unchanged. Compared with v0.2.1 over a corpus of the repository's documents, targeted L5X rung variants and 3,000 seeded mutations (3,050 cases at the public
check_fileAPI), the only differences are listed below, and every one is on non-legitimate input.Behaviour changes on non-legitimate input
BodyContent(IEC 61131-10): thePX121scope is the nearest body.Routinesis its own rung scope; aRoutinenested without aRoutineswrapper keeps its rungs with the enclosing routine.CompileUnitis its own scope, also inside a payload.check_rung: a coil operand that holds a nested call (OTE(OTE(a)),OTE(Foo(1))) or a quote is no longer recorded, so a repeated one no longer givesLX040. 20 KB ofOTE(held 40 MB of operand copies before. A bare(does not count:OTE(Arr[(i+1)])is still recorded, and a subscript written with a call (OTE(Arr[Idx(1)])) is not.check_rung: an unclosed instruction drops the last character of its operand text; a mnemonic skips leading digits.Tests
291 to 365 (green on Python 3.9, 3.12 and 3.14; about +0.3 s of suite time). New:
tests/linear_support.py,test_linear_xml.py,test_linear_text.py,test_linear_aml.py. Each fixed site has a CPU-time ratio test (best of three, small sizes so the unfixed code fails in under a second) and a test that pins the unchanged result on legitimate input; the Control Expert tests use the realFBSource > FBProgram > STSourceshape. Every new branch was checked with mutants (each goes red) and old-versus-new differentials with a deliberately wrong control.Summary by CodeRabbit