Skip to content

Commit 871d21b

Browse files
Krishcalinclaude
andcommitted
The ABAP lexer crashed on the way nearly every ABAP file begins
*&---------------------------------------------------------------------* *& Report Z_SOMETHING *&---------------------------------------------------------------------* REPORT z_something. `_scan_line` returned early on a column-1 `*` comment — before the line that recorded whether the line had left a literal open. The splitter reads that record on the very next statement, off the function object. So the FIRST file scanned in a process whose first line is a comment raised AttributeError. scan_tree does not catch it, so one ordinary abapGit banner failed the whole abap_sast module and the run reported no custom-code findings at all. I found it by writing a fixture that opens with a comment, which no existing fixture does — all nine start on a keyword. The second defect is quieter and was the reason to fix this properly rather than set the attribute on one more path. Once any non-comment line had set it, the value persisted ON THE FUNCTION across comment lines and across FILES. A file ending inside an open literal left it True, and the next file to begin with a comment had its first statement marked `degraded` — a lexer fault attributed to an innocent file, and one more on the `lex_degraded` count that raises a coverage finding about mis-lexed objects. `_scan_line` now returns the fact as its fifth value, so a path that forgets to produce it cannot compile rather than reading a stale one, and the attribute is gone. The comment defending the original design cited "four call sites [that] depend on" the signature; there were two. A comment line returns False rather than recomputing from the carried stack — they are equivalent on every stack the comment guard can be reached with, and False says the thing that is true: a comment opens nothing and closes nothing. Three of the four mutations run against the new test fail it; the fourth was provably a no-op rewrite of the same constant, not an escaped defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 8c462dc commit 871d21b

2 files changed

Lines changed: 140 additions & 8 deletions

File tree

modules/abap_sast.py

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -572,8 +572,8 @@ def emit(text: str, masked: str, degraded: bool = False) -> None:
572572
while _in_amdp(stack):
573573
stack.pop()
574574

575-
code, mask, term, stack = _scan_line(line, stack)
576-
if _scan_line.unterminated_at_eol:
575+
code, mask, term, stack, unterminated = _scan_line(line, stack)
576+
if unterminated:
577577
pending_degraded = True
578578
raw.append(line)
579579

@@ -934,8 +934,12 @@ def put(ch: str, m: Optional[str] = None, t: str = "0") -> None:
934934

935935
# A `*` in column 1 comments the whole line — but only when we are not inside
936936
# a literal or template left open on a previous line, where it is text.
937+
#
938+
# `False` for the last element and not a carried value: a comment line opens
939+
# nothing and closes nothing, so it leaves nothing unterminated. Whatever the
940+
# line above left open was reported when THAT line was scanned.
937941
if line[:1] == "*" and mode in (_M_CODE, _M_EMB, _M_AMDP):
938-
return "", "", "", stack
942+
return "", "", "", stack, False
939943

940944
i, n = 0, len(line)
941945
while i < n:
@@ -1071,10 +1075,8 @@ def put(ch: str, m: Optional[str] = None, t: str = "0") -> None:
10711075
i += 1
10721076

10731077
closed = _close_at_end_of_line(stack)
1074-
#: Set on the function so the splitter can mark the statement without
1075-
#: changing a return signature four call sites depend on.
1076-
_scan_line.unterminated_at_eol = len(closed) != len(stack)
1077-
return "".join(code), "".join(mask), "".join(term), closed
1078+
return ("".join(code), "".join(mask), "".join(term), closed,
1079+
len(closed) != len(stack))
10781080

10791081

10801082
def _close_at_end_of_line(stack: List[Tuple[str, str]]) -> List[Tuple[str, str]]:
@@ -1744,7 +1746,7 @@ def __init__(self, text: str, sanitizers: Iterable[str] = DEFAULT_SANITIZERS):
17441746
code_lines: List[str] = []
17451747
stack: List[Tuple[str, str]] = []
17461748
for line in self._raw:
1747-
_code, mask, _term, stack = _scan_line(line, stack)
1749+
_code, mask, _term, stack, _unterminated = _scan_line(line, stack)
17481750
code_lines.append(_taint_rewrite(mask.strip()))
17491751
self._code = code_lines
17501752
# B2/B4 — a METHOD implementation header carries no signature, so the
Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
"""The ABAP lexer crashed on the way nearly every ABAP file begins.
2+
3+
*&---------------------------------------------------------------------*
4+
*& Report Z_SOMETHING
5+
*&---------------------------------------------------------------------*
6+
REPORT z_something.
7+
8+
A `*` in column 1 is a full-line comment, and `_scan_line` returned early on it
9+
— before the line that recorded whether the line had left a literal open. The
10+
splitter read that record on the very next statement. So:
11+
12+
* The FIRST file scanned in a process whose first line is a comment raised
13+
AttributeError. `scan_tree` does not catch it, so one ordinary banner failed
14+
the whole `abap_sast` module and the run reported no custom-code findings at
15+
all. Every abapGit export and every SE38 program starts this way.
16+
17+
* Once any non-comment line had set it, the value PERSISTED on the function
18+
across comment lines and across FILES. A file that ended inside an open
19+
literal left it True, and the next file to begin with a comment had its first
20+
statement marked `degraded` — a lexer fault attributed to an innocent file,
21+
which also inflates the `lex_degraded` count that raises a coverage finding.
22+
23+
Both came from carrying the fact on the function object rather than returning it,
24+
and the comment that explained the choice cited "four call sites" that would have
25+
to change. There were two. `_scan_line` now returns it and the attribute is gone,
26+
so a path that forgets to set it cannot compile rather than reading a stale one.
27+
"""
28+
from __future__ import annotations
29+
30+
import sys
31+
from pathlib import Path
32+
33+
import pytest
34+
35+
ROOT = Path(__file__).resolve().parents[1]
36+
if str(ROOT) not in sys.path:
37+
sys.path.insert(0, str(ROOT))
38+
39+
from modules import abap_sast as sast # noqa: E402
40+
41+
#: What abapGit writes above `REPORT`. The literal cause of the crash.
42+
BANNER = (
43+
"*&---------------------------------------------------------------------*\n"
44+
"*& Report Z_DEMO\n"
45+
"*&---------------------------------------------------------------------*\n"
46+
)
47+
CLEAN = BANNER + "REPORT z_demo.\nWRITE 'hello'.\n"
48+
49+
#: A file whose last line leaves a text literal open. Genuinely degraded, and the
50+
#: source of the state that used to leak into the next file.
51+
UNTERMINATED = "REPORT z_a.\nDATA(x) = 'never closed\n"
52+
53+
54+
def test_a_file_that_opens_with_a_comment_does_not_raise():
55+
"""The crash, reproduced at its simplest.
56+
57+
Called first in this module so it runs against the same cold state a real
58+
scan starts from — the defect was invisible to any test that had already
59+
lexed a line of code."""
60+
assert sast.split_statements(CLEAN), "the banner-first file produced nothing"
61+
62+
63+
def test_the_fact_is_returned_and_not_parked_on_the_function():
64+
"""Structural, because the behavioural tests below pass either way once the
65+
attribute happens to be set. An attribute is reachable from anywhere and
66+
survives the call that wrote it; a return value cannot be stale."""
67+
assert not hasattr(sast._scan_line, "unterminated_at_eol"), (
68+
"the lexer is carrying its per-line state on the function object again. "
69+
"Return it instead — that is what made a comment line inherit the "
70+
"previous line's verdict.")
71+
_code, _mask, _term, _stack, unterminated = sast._scan_line("REPORT z.", [])
72+
assert unterminated is False
73+
74+
75+
def test_a_comment_line_reports_nothing_unterminated():
76+
"""It opens nothing and closes nothing. Whatever the line above left open was
77+
reported when that line was scanned."""
78+
_c, _m, _t, _s, unterminated = sast._scan_line("* just a comment", [])
79+
assert unterminated is False
80+
81+
82+
def test_an_unterminated_literal_is_still_degraded():
83+
"""The fix must not have bought its correctness by never flagging anything."""
84+
flagged = [st for st in sast.split_statements(UNTERMINATED) if st.degraded]
85+
assert flagged, "a line leaving a literal open is no longer marked degraded"
86+
assert "never closed" in flagged[0].text
87+
88+
89+
def test_one_files_open_literal_does_not_degrade_the_next_file():
90+
"""The contamination, stated as the customer would meet it: file B is clean
91+
ABAP and gets a lexer fault on its first statement because file A, scanned
92+
earlier in the same run, ended mid-literal."""
93+
sast.split_statements(UNTERMINATED)
94+
after = sast.split_statements(CLEAN)
95+
assert not any(st.degraded for st in after), (
96+
"a clean file was marked degraded by the previous file's open literal: "
97+
+ repr([st.text for st in after if st.degraded]))
98+
99+
100+
def test_the_banner_is_not_counted_as_a_statement():
101+
"""Three comment lines and two statements. A banner that lexed as a statement
102+
would give every rule three lines of prose to match against."""
103+
texts = [st.text for st in sast.split_statements(CLEAN)]
104+
assert texts == ["REPORT z_demo", "WRITE 'hello'"], texts
105+
106+
107+
@pytest.mark.parametrize("first", [
108+
"*&---------------------------------------------------------------------*",
109+
"* plain comment",
110+
"*",
111+
"*\"! ABAP Doc comment",
112+
])
113+
def test_every_column_one_comment_shape_opens_a_file_safely(first):
114+
"""`*` in column 1 is the comment marker whatever follows it, including the
115+
ABAP Doc form, whose `"` would otherwise open a literal."""
116+
assert sast.split_statements(first + "\nREPORT z.\nWRITE 'x'.\n")
117+
118+
119+
def test_the_scanner_reads_a_banner_first_file_end_to_end():
120+
"""Above the lexer: the module-level failure the crash actually produced was
121+
`abap_sast` returning nothing for the whole tree."""
122+
scanner = sast.AbapSourceScanner(data_flow=True)
123+
findings = scanner.scan_text(
124+
BANNER + "REPORT z_demo.\n"
125+
"PARAMETERS p_t TYPE string.\n"
126+
"SELECT * FROM (p_t) INTO TABLE @DATA(lt).\n",
127+
Path("z_demo.prog.abap"))
128+
assert any(f["rule_id"].startswith("ABAP-SQLI") for f in findings), (
129+
"the dynamic FROM clause under a banner produced no injection finding: "
130+
+ repr(sorted({f["rule_id"] for f in findings})))

0 commit comments

Comments
 (0)