Skip to content

Commit 30cbe88

Browse files
Improve pattern safety checks []
1 parent 74534ea commit 30cbe88

6 files changed

Lines changed: 125 additions & 32 deletions

File tree

‎dev_tools/bin/edgy‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -255,15 +255,36 @@ def load_edgy_config(args: argparse.Namespace, test_home: Path) -> EdgyConfig:
255255
)
256256

257257
test_suites = config_file_dict.get('test_suites', {})
258+
primary_test_suites = test_suites.get('primary', [])
259+
secondary_test_suites = test_suites.get('secondary', [])
260+
_validate_configured_suites(
261+
config_file_path,
262+
[*primary_test_suites, *secondary_test_suites]
263+
)
258264
config_sets = config_file_dict.get('config_sets', {})
259265
return EdgyConfig(
260266
test_home,
261267
loaded_configs,
262268
config_sets,
263-
test_suites.get('primary', []),
264-
test_suites.get('secondary', [])
269+
primary_test_suites,
270+
secondary_test_suites
265271
)
266272

273+
def _validate_configured_suites(config_file_path: Path,
274+
test_suite_names: List[Any]) -> None:
275+
'''Exit unless every suite ``config_file_path`` lists has a valid name.'''
276+
invalid_test_suite_encountered = False
277+
for test_suite in test_suite_names:
278+
if not (isinstance(test_suite, str) and
279+
edgtest.is_valid_suite_name(test_suite)):
280+
edgutil.eprint(f"Invalid test suite name {test_suite!r} in "
281+
f"{config_file_path} (expected 1-64 of A-Z, a-z, "
282+
"0-9, _ and -).")
283+
invalid_test_suite_encountered = True
284+
285+
if invalid_test_suite_encountered:
286+
sys.exit(1)
287+
267288
def get_expectation_path(run_config: TestRunConfig,
268289
test_suite_name: str) -> Path:
269290
return Path(run_config.expectations_dir, f"{test_suite_name}.log")

‎dev_tools/pylibs/edgacknowledg/forge/workspace.py‎

Lines changed: 22 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,11 @@
4141
from pathlib import Path, PurePosixPath
4242
from typing import Iterable, Optional, Sequence
4343

44+
import edgtest
45+
4446
from edgacknowledg.forge import Forge, PullRequestInfo
4547
from edgacknowledg.forge.runner.roles import BUILD_STAGING_DIR, REVIEWS_DIR
48+
from edgtest.runtest import get_bare_name
4649

4750
_log = logging.getLogger(__name__)
4851

@@ -57,12 +60,12 @@
5760

5861
BENCH_BASELINE_PATTERNS = ('/benchmarks/baselines/',)
5962

60-
# The longest file name (in bytes) a test can have; a longer one names no
61-
# file, so it needs no pattern (and its patterns would be quadratic in its
62-
# length, one per dot, each as long as the name).
63-
_NAME_MAX = 255
6463
# The most sparse-checkout patterns a checkout may have: matching costs
65-
# patterns x paths, and they are built from pull request content.
64+
# patterns x paths, and they are built from pull request content. Their
65+
# total size must also stay within a small multiple of the diff's (see
66+
# MAX_DIFF_BYTES): a test gets at most two patterns, each about as long as
67+
# its path, and the only text that is part of many patterns, a suite's
68+
# name, is short.
6669
MAX_PATTERNS = 20_000
6770
# The largest diff (of tests/, in bytes) a pull request may have: it is
6871
# held in memory, and copied and split into lines more than once.
@@ -94,28 +97,21 @@ def _escape_pattern(path: str) -> str:
9497
return re.sub(r'([\\*?\[\]!#])', r'\\\1', path)
9598

9699

97-
def _test_base_names(file_name: str) -> list[str]:
98-
'''Return the candidate base names of a test file (for its '.<base>.rto'
99-
recordings directory), e.g. 'a.b.sft.cpp' -> ['a.b.sft', 'a.b', 'a'].
100-
'''
101-
parts = file_name.split('.')
102-
return ['.'.join(parts[:count]) for count in range(len(parts) - 1, 0, -1)]
103-
104-
105100
def patterns_for_test(test_path: PurePosixPath) -> list[str]:
106101
'''Sparse-checkout patterns for one test (a repository-relative path such
107-
as 'tests/tests/suite/dir/name.sft.cpp'): the test file and its recordings,
108-
or the whole directory of a multi-file test.
102+
as 'tests/tests/suite/dir/name.sft.cpp'): the test file and its recordings
103+
(where the test runner keeps them, '.<bare name>.rto'), or the whole
104+
directory of a multi-file test. At most two, each about as long as the
105+
path, however it is named (see MAX_PATTERNS).
109106
'''
110-
if len(test_path.name.encode()) > _NAME_MAX:
111-
return []
112107
directory = _escape_pattern(str(test_path.parent))
113108
if '.mft.' in test_path.name:
114109
return [f"/{directory}/"]
115-
patterns = [f"/{_escape_pattern(str(test_path))}"]
116-
for base_name in _test_base_names(test_path.name):
117-
patterns.append(f"/{directory}/.{_escape_pattern(base_name)}.rto/")
118-
return patterns
110+
bare_name = _escape_pattern(get_bare_name(test_path))
111+
return [
112+
f"/{_escape_pattern(str(test_path))}",
113+
f"/{directory}/.{bare_name}.rto/",
114+
]
119115

120116

121117
def _patterns_for_changed_test_file(path: PurePosixPath) -> list[str]:
@@ -163,8 +159,11 @@ def changes_patterns(diff: str) -> list[str]:
163159
_add_patterns(patterns, _patterns_for_changed_test_file(path))
164160
elif (path.parts[:2] == ('tests', 'expectations') and
165161
path.suffix == '.log'):
166-
# Expectation lines name tests relative to the suite.
167-
current_suite = path.stem
162+
# Expectation lines name tests relative to the suite, so its name is
163+
# part of the patterns of each (see MAX_PATTERNS). A file whose name
164+
# no suite may have (e.g. too long) names no tests.
165+
if edgtest.is_valid_suite_name(path.stem):
166+
current_suite = path.stem
168167
continue
169168
if current_suite is None or line.startswith(('---', '+++')):
170169
continue

‎dev_tools/pylibs/edgtest/__init__.py‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,15 @@ def load_test_suite_env(test_suite_dir: Path) -> Dict[str, str]:
204204
{'sft.cpp', 'sft.c', 'mft.cpp', 'mft.c'}
205205
)
206206

207+
# What a test suite (a directory of the test home's tests/) may be named.
208+
# Tools taking suite names from untrusted input (e.g. the AcknowlEDG Forge,
209+
# from expectation file names) rely on it to bound them.
210+
_SUITE_NAME_REGEX = re.compile(r'[A-Za-z0-9_-]{1,64}')
211+
212+
def is_valid_suite_name(name: str) -> bool:
213+
'''Return whether ``name`` may name a test suite.'''
214+
return _SUITE_NAME_REGEX.fullmatch(name) is not None
215+
207216
def test_extensions_for_suite(test_home: Path,
208217
suite_name: str) -> Set[str]:
209218
'''Return a list of file extensions that should be considered when searching

‎dev_tools/pylibs/edgtest/runtest/__init__.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525

2626
from enum import IntEnum
2727
from collections import OrderedDict
28-
from pathlib import Path
28+
from pathlib import Path, PurePath
2929
from typing import (
3030
Any, Callable, Dict, IO, Iterator, List, Mapping, NamedTuple, Optional,
3131
Pattern, Set, Tuple, cast
@@ -83,7 +83,7 @@ def get_matches_possible_names(test_suite_env: Dict[str, str],
8383
results.append(f"default.{case_number}.matches.txt")
8484
return results
8585

86-
def get_bare_name(test_file_path: Path) -> str:
86+
def get_bare_name(test_file_path: PurePath) -> str:
8787
'''Return the test name without the file extension.'''
8888
return test_file_path.name.split('.')[0]
8989

‎dev_tools/services/acknowledg-forge/tests/test_safety.py‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -179,9 +179,10 @@ def test_ensure_real_directory_refuses_symlinks(tmp_path: Path) -> None:
179179

180180

181181
def test_test_patterns() -> None:
182+
# Recordings are where the test runner keeps them: named for the part of
183+
# the test's name before its first dot.
182184
assert patterns_for_test(PurePosixPath('tests/tests/s/d/a.sft.cpp')) == [
183185
'/tests/tests/s/d/a.sft.cpp',
184-
'/tests/tests/s/d/.a.sft.rto/',
185186
'/tests/tests/s/d/.a.rto/',
186187
]
187188
# Multi-file tests own their directory.
@@ -216,10 +217,31 @@ def test_changes_patterns_cover_recordings_and_expectations() -> None:
216217
assert not any(p.startswith('/dev_tools/') for p in patterns)
217218

218219

219-
def test_long_test_names_need_no_patterns() -> None:
220-
# No file has such a name; its patterns would be quadratic in its length.
221-
name = '.'.join(['x'] * 200) + '.sft.cpp'
222-
assert patterns_for_test(PurePosixPath('tests/tests/s', name)) == []
220+
def test_test_patterns_are_linear_in_the_path() -> None:
221+
# Two, however many dots its name has.
222+
name = '.'.join(['x'] * 2100) + '.sft.cpp'
223+
assert patterns_for_test(PurePosixPath('tests/tests/s', name)) == [
224+
f"/tests/tests/s/{name}",
225+
'/tests/tests/s/.x.rto/',
226+
]
227+
228+
229+
def test_changes_patterns_skip_suites_no_suite_may_be_named() -> None:
230+
# A suite is in the patterns of every test its expectation file names.
231+
def expectations(suite: str) -> str:
232+
log = f"tests/expectations/edg_x86_64/{suite}.log"
233+
return '\n'.join([
234+
f"diff --git a/{log} b/{log}",
235+
f"--- a/{log}",
236+
f"+++ b/{log}",
237+
'+a.sft.cpp: PASS',
238+
])
239+
240+
assert '/tests/tests/cwg/a.sft.cpp' in changes_patterns(expectations('cwg'))
241+
for suite in ['s' * 65, 'a.b', 'a b']:
242+
assert changes_patterns(expectations(suite)) == [
243+
'/tests/.edgy/', '/tests/expectations/',
244+
]
223245

224246

225247
def test_patterns_are_capped() -> None:
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM
2+
# Exceptions.
3+
# See https://edgcpp.org/LICENSE.txt for license information.
4+
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
5+
6+
'''Tests for the test suite names edgtest accepts (which edgy checks on
7+
loading .edgy/config.json).
8+
9+
Run with: python3 -m unittest discover -s dev_tools/tests
10+
(with dev_tools/pylibs on PYTHONPATH).
11+
'''
12+
13+
import json
14+
import unittest
15+
16+
import edgtest
17+
18+
from pathlib import Path
19+
20+
REPO_DIR = Path(__file__).resolve().parents[2]
21+
22+
class SuiteNameTests(unittest.TestCase):
23+
def test_valid_names(self) -> None:
24+
for name in ['cwg', 'edg_x86', 'cpp-23', 'A1', 's' * 64]:
25+
with self.subTest(name = name):
26+
self.assertTrue(edgtest.is_valid_suite_name(name))
27+
28+
def test_invalid_names(self) -> None:
29+
for name in ['', 's' * 65, 'a.b', 'a/b', '..', 'a b', 'café',
30+
'cwg\n']:
31+
with self.subTest(name = name):
32+
self.assertFalse(edgtest.is_valid_suite_name(name))
33+
34+
def test_the_test_homes_suites_are_valid(self) -> None:
35+
config_path = REPO_DIR / 'tests' / '.edgy' / 'config.json'
36+
test_suites = json.loads(config_path.read_text())['test_suites']
37+
for name in [*test_suites['primary'], *test_suites['secondary']]:
38+
with self.subTest(name = name):
39+
self.assertTrue(edgtest.is_valid_suite_name(name))
40+
41+
if __name__ == '__main__':
42+
unittest.main()

0 commit comments

Comments
 (0)