Do not factor the files in an .rt.tar.gz as part of the overall review limits, check them independently []

dev_tools/pylibs/edgacknowledg/client/test/__init__.py

@@ -55,14 +55,26 @@
 __all__ = ['IllegalFileAccessError']
 
 
+def _run_test_output(
+    path: Path, max_rt_entries: Optional[int],
+    max_rt_member_bytes: Optional[int]) -> edgtest.runtest.RunTestOutput:
+  return edgtest.runtest.RunTestOutput(
+    path,
+    max_entries = max_rt_entries,
+    max_member_bytes = max_rt_member_bytes
+  )
+
+
 def _rt_path_for(config_dir: ConfinedPath, test_id: str) -> ConfinedPath:
   return config_dir / str(PurePosixPath(test_id).with_suffix(
     edgtest.runtest.RUN_TEST_OUTPUT_SUFFIX
   ))
 
 
 def _create_status_dict(elog_file_paths: List[ConfinedPath], *,
-                        is_multi_config_run: bool) -> _StatusDict:
+                        is_multi_config_run: bool,
+                        max_rt_entries: Optional[int],
+                        max_rt_member_bytes: Optional[int]) -> _StatusDict:
   # test_id -> config_name -> combined_diff_hash.
   pending: Dict[str, Dict[str, str]] = {}
   lines_by_test_config: Dict[str, Dict[str, List[Dict[str, str]]]] = {}
@@ -85,7 +97,11 @@ def _create_status_dict(elog_file_paths: List[ConfinedPath], *,
         if config_name not in pending[key]:
           rt_file_path = _rt_path_for(elog_file_path.parent, key)
           pending[key][config_name] = (
-            edgtest.runtest.grouping_combined_diff_hash(rt_file_path.path)
+            edgtest.runtest.grouping_combined_diff_hash(
+              rt_file_path.path,
+              max_entries = max_rt_entries,
+              max_member_bytes = max_rt_member_bytes
+            )
           )
           lines_by_test_config[key][config_name] = []
 
@@ -183,7 +199,9 @@ async def run_test_source_handler(channel: Channel,
                                   context: SandboxContext,
                                   test_home: ConfinedPath,
                                   run_dir: ConfinedPath,
-                                  changes_files: Dict[str, ConfinedPath]
+                                  changes_files: Dict[str, ConfinedPath], *,
+                                  max_rt_entries: Optional[int] = None,
+                                  max_rt_member_bytes: Optional[int] = None
                                   ) -> None:
   '''Handle one test source request on ``channel``. Every file is reached
   from ``test_home`` or ``run_dir``, so it can't leave them, and commands
@@ -222,7 +240,9 @@ def test_id_to_path(test_id: str) -> ConfinedPath:
             'name': 'Changes List',
             'statuses': _create_status_dict(
               _matching_change_files(changes_files, 'changes.elog'),
-              is_multi_config_run = is_multi_config_run
+              is_multi_config_run = is_multi_config_run,
+              max_rt_entries = max_rt_entries,
+              max_rt_member_bytes = max_rt_member_bytes
             )
           }
         )
@@ -237,7 +257,9 @@ def test_id_to_path(test_id: str) -> ConfinedPath:
               'name': 'Regressions List',
               'statuses': _create_status_dict(
                 regress_files,
-                is_multi_config_run = is_multi_config_run
+                is_multi_config_run = is_multi_config_run,
+                max_rt_entries = max_rt_entries,
+                max_rt_member_bytes = max_rt_member_bytes
               )
             }
           )
@@ -252,7 +274,9 @@ def test_id_to_path(test_id: str) -> ConfinedPath:
               'name': 'Improvements List',
               'statuses': _create_status_dict(
                 improve_files,
-                is_multi_config_run = is_multi_config_run
+                is_multi_config_run = is_multi_config_run,
+                max_rt_entries = max_rt_entries,
+                max_rt_member_bytes = max_rt_member_bytes
               )
             }
           )
@@ -314,7 +338,8 @@ async def form_curr_diff_text(config_name: str,
             tmp_dir = Path(tmp_dir_name)
             entries: List[Tuple[Dict[str, Any], List[str]]] = []
 
-            with edgtest.runtest.RunTestOutput(case_rt_path.path) as rt_output:
+            with _run_test_output(case_rt_path.path, max_rt_entries,
+                                  max_rt_member_bytes) as rt_output:
               for case_number, case in enumerate(
                 rt_output.metadata.get('cases', []),
                 start = 1
@@ -413,14 +438,16 @@ async def form_curr_diff_text(config_name: str,
             )
 
         if request_tag == RequestTags.TEST_SHOW_DIFF:
-          with edgtest.runtest.RunTestOutput(rt_file_path.path) as rt_output:
+          with _run_test_output(rt_file_path.path, max_rt_entries,
+                                max_rt_member_bytes) as rt_output:
             static_text = ''.join(rt_output.combined_static_diff())
           client_response = {
             'id': configured_test_id,
             **trunc_display_content(static_text)
           }
         elif request_tag == RequestTags.TEST_SHOW_RAW_OUTPUT:
-          with edgtest.runtest.RunTestOutput(rt_file_path.path) as rt_output:
+          with _run_test_output(rt_file_path.path, max_rt_entries,
+                                max_rt_member_bytes) as rt_output:
             raw_output = ''.join(rt_output.combined_raw_output())
           client_response = {
             'id': configured_test_id,
@@ -602,7 +629,9 @@ def __init__(self,
                slug: str,
                context: SandboxContext,
                test_home: ConfinedPath,
-               root: ConfinedPath) -> None:
+               root: ConfinedPath, *,
+               max_rt_entries: Optional[int] = None,
+               max_rt_member_bytes: Optional[int] = None) -> None:
     '''Requests reach files only through ``test_home`` and the run
     directory ``root``, and run commands in ``context``.
     '''
@@ -611,6 +640,8 @@ def __init__(self,
     self._context = context
     self._test_home = test_home
     self._root = root
+    self._max_rt_entries = max_rt_entries
+    self._max_rt_member_bytes = max_rt_member_bytes
 
   def _changes_files(self) -> Dict[str, ConfinedPath]:
     '''Each configuration's changes file in the run, found now.'''
@@ -638,7 +669,9 @@ async def handle_request(self, channel: Channel) -> None:
       self._context,
       self._test_home,
       self._root,
-      await edgutil.async_to_thread(self._changes_files)
+      await edgutil.async_to_thread(self._changes_files),
+      max_rt_entries = self._max_rt_entries,
+      max_rt_member_bytes = self._max_rt_member_bytes
     )
 
 async def handle_test_edit_request(

dev_tools/pylibs/edgacknowledg/forge/config.py

@@ -34,10 +34,10 @@
 # The largest diff (of tests/, in bytes) a pull request may have: it is
 # held in memory, and copied and split into lines more than once.
 DEFAULT_MAX_DIFF_BYTES = 64 * 1024 * 1024
-# The most entries an artifact may have (its zip's, and its tarballs'
-# together), and the largest its zip's central directory may be: zipfile
-# reads all of that directory into memory, before we can count. CI's test
-# results reach about 170,000 entries, of about 106 bytes each there.
+# The most entries an artifact zip may have, and the largest its central
+# directory may be: zipfile reads all of that directory into memory, before
+# we can count. CI's test results reach about 170,000 entries, of about 106
+# bytes each there.
 DEFAULT_MAX_ARTIFACT_ENTRIES = 500_000
 DEFAULT_MAX_ARTIFACT_DIRECTORY_BYTES = 64 * 1024 * 1024
 

dev_tools/pylibs/edgacknowledg/forge/publisher/pool_process.py

@@ -51,6 +51,8 @@
 # Parallel cg_annotate runs in one process (it answers one request at a
 # time, so this only matters to a request running several).
 _BENCH_WORKERS = 2
+_MAX_RT_ENTRIES = 10_000
+_MAX_RT_MEMBER_BYTES = 64 * 1024 * 1024
 
 
 def build_handler(section: SectionRecord,
@@ -67,7 +69,9 @@ def build_handler(section: SectionRecord,
       section.slug,
       context,
       context.subpath(section.home),
-      context.subpath(section.root)
+      context.subpath(section.root),
+      max_rt_entries = _MAX_RT_ENTRIES,
+      max_rt_member_bytes = _MAX_RT_MEMBER_BYTES
     )
   if section.kind == KIND_BENCH:
     session = BenchReviewSession(

dev_tools/pylibs/edgacknowledg/forge/workspace.py

@@ -33,7 +33,6 @@
 import shutil
 import stat
 import struct
-import tarfile
 import urllib.parse
 import zipfile
 
@@ -348,22 +347,6 @@ def _central_directory_size(zip_path: Path) -> Optional[int]:
   return directory_size
 
 
-def _check_inner_tarball(path: Path, budget: _Budget) -> None:
-  '''Charge a (gzipped) tarball's decompressed size and entries to
-  ``budget``; reviewers' requests decompress these, so they're
-  bombs-in-waiting otherwise.
-  '''
-  try:
-    with tarfile.open(path, 'r:*') as archive:
-      for member in archive:
-        budget.count_entry()
-        if not (member.isfile() or member.isdir()):
-          raise UnsafeArchiveError(f"{path.name} contains a link or device")
-        budget.spend(member.size)
-  except tarfile.TarError as ex:
-    raise UnsafeArchiveError(f"{path.name} is not a valid tarball") from ex
-
-
 def extract_artifact(
       zip_path: Path, destination: Path, *, max_bytes: int,
       max_entries: int = DEFAULT_MAX_ARTIFACT_ENTRIES,
@@ -372,8 +355,8 @@ def extract_artifact(
 
   Rejects absolute or escaping paths and symlinks, and a zip whose central
   directory is over ``max_directory_bytes``, and stops once more than
-  ``max_bytes`` would be written, or ``max_entries`` entries made (counting
-  inner tarballs' contents too).
+  ``max_bytes`` would be written, or ``max_entries`` zip entries made.
+  Inner tarballs are checked when opened by a reviewer.
   '''
   directory_size = _central_directory_size(zip_path)
   if directory_size is not None and directory_size > max_directory_bytes:
@@ -382,7 +365,6 @@ def extract_artifact(
       f"({ENV_PREFIX}MAX_ARTIFACT_DIRECTORY_BYTES)"
     )
   budget = _Budget(max_bytes, max_entries)
-  tarballs = []
   try:
     with zipfile.ZipFile(zip_path) as archive:
       for info in archive.infolist():
@@ -407,14 +389,9 @@ def extract_artifact(
               break
             budget.spend(len(chunk))
             sink.write(chunk)
-        if target.name.endswith(('.tar.gz', '.tgz', '.tar')):
-          tarballs.append(target)
   except zipfile.BadZipFile as ex:
     raise UnsafeArchiveError('artifact is not a valid zip archive') from ex
 
-  for tarball in tarballs:
-    _check_inner_tarball(tarball, budget)
-
 
 class Workspaces:
   '''Clones and worktrees under ``home`` for one forge; a diff (``diff``)

dev_tools/pylibs/edgtest/runtest/__init__.py

@@ -958,20 +958,29 @@ def _unlink_missing_ok(path: Path) -> None:
   elif path.exists():
     path.unlink()
 
-def read_combined_diff_hash(rt_file_path: Path) -> Optional[str]:
+def read_combined_diff_hash(rt_file_path: Path, *,
+                            max_entries: Optional[int] = None,
+                            max_member_bytes: Optional[int] = None
+                            ) -> Optional[str]:
   '''Return metadata.combined_diff_hash, or None when there is no diff.'''
-  with RunTestOutput(rt_file_path) as rt_output:
+  with RunTestOutput(rt_file_path, max_entries = max_entries,
+                     max_member_bytes = max_member_bytes) as rt_output:
     return rt_output.metadata.get('combined_diff_hash')
 
-def grouping_combined_diff_hash(rt_file_path: Path) -> str:
+def grouping_combined_diff_hash(rt_file_path: Path, *,
+                                max_entries: Optional[int] = None,
+                                max_member_bytes: Optional[int] = None) -> str:
   '''Return a combined diff hash suitable for grouping configs.
 
   Missing .rt.tar files use the sentinel "missing". Archives with no
   change.diff members use the SHA-256 of empty content so they still group.
   '''
   if not rt_file_path.exists():
     return 'missing'
-  combined = read_combined_diff_hash(rt_file_path)
+  combined = read_combined_diff_hash(
+    rt_file_path, max_entries = max_entries,
+    max_member_bytes = max_member_bytes
+  )
   if combined is None:
     return hash_diff_bytes(b'')
   return combined
@@ -1320,8 +1329,11 @@ def _read_tar_lines_member(tar: tarfile.TarFile,
 class RunTestOutput:
   '''Context manager for reading a structured .rt.tar run artifact once.'''
 
-  def __init__(self, path: Path) -> None:
+  def __init__(self, path: Path, *, max_entries: Optional[int] = None,
+               max_member_bytes: Optional[int] = None) -> None:
     self.path = path
+    self._max_entries = max_entries
+    self._max_member_bytes = max_member_bytes
     self._tar: Optional[tarfile.TarFile] = None
     self._metadata: Optional[Dict[str, Any]] = None
     self._member_names: Optional[Set[str]] = None
@@ -1332,6 +1344,22 @@ def __enter__(self) -> 'RunTestOutput':
       'r:gz' if RT_TAR_GZ_SUPPORTED else 'r',
       format = tarfile.PAX_FORMAT
     )
+    try:
+      if self._max_entries is not None or self._max_member_bytes is not None:
+        for count, member in enumerate(self._tar, start = 1):
+          if self._max_entries is not None and count > self._max_entries:
+            raise ValueError('run artifact has too many entries')
+          if not (member.isfile() or member.isdir()):
+            raise RunTestOutputLinkError(
+              f"{member.name} is a link or device"
+            )
+          if (self._max_member_bytes is not None and
+              member.size > self._max_member_bytes):
+            raise ValueError(f"run artifact member {member.name} is too large")
+    except BaseException:
+      self._tar.close()
+      self._tar = None
+      raise
     return self
 
   def __exit__(self, _type: Any, _value: Any, _tb: Any) -> None:

dev_tools/services/acknowledg-forge/README.md

@@ -111,8 +111,8 @@ from the environment variable shown as a fallback.
 | `ACKNOWLEDG_FORGE_COMMENTS` | `0` | `1` enables comments (needs the forge token) |
 | `ACKNOWLEDG_FORGE_SETTLE_TIMEOUT` | `7200` | Seconds after a push to stop waiting for CI before commenting |
 | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_BYTES` | 256 MiB | Largest artifact download |
-| `ACKNOWLEDG_FORGE_MAX_EXTRACTED_BYTES` | 2 GiB | Largest extracted artifact (including inner tarballs) |
-| `ACKNOWLEDG_FORGE_MAX_ARTIFACT_ENTRIES` | `500000` | Most entries in an artifact (including inner tarballs'). CI's test results reach about 170,000 |
+| `ACKNOWLEDG_FORGE_MAX_EXTRACTED_BYTES` | 2 GiB | Largest extracted artifact (including stored, compressed inner tarballs, but not their unextracted contents) |
+| `ACKNOWLEDG_FORGE_MAX_ARTIFACT_ENTRIES` | `500000` | Most ZIP entries in an artifact. Inner tarballs are checked when read |
 | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_DIRECTORY_BYTES` | 64 MiB | Largest artifact zip central directory, which is read into memory whole (about 106 bytes per entry in CI's test results) |
 | `ACKNOWLEDG_FORGE_MAX_DIFF_BYTES` | 64 MiB | Largest diff of a pull request's `tests/`, which is held in memory and copied more than once: raising it may need a higher `mem_limit` for the service ([`compose.yaml`](compose.yaml)) |
 | `ACKNOWLEDG_FORGE_MAX_SPARSE_PATTERNS` | `20000` | Most sparse-checkout patterns in a review's worktree (up to two per test). Checking out takes time proportional to patterns times paths |

dev_tools/services/acknowledg-forge/SANDBOXING.md

@@ -279,7 +279,7 @@ sequenceDiagram
   Note over S: check out its merge-base,<br/>symlinks as plain files
   Note over S: check_plain_tree(worktree)
   S->>F: download the CI artifact (size-capped)
-  Note over S: extract it: no links, no escaping paths,<br/>inner tarballs size-capped
+  Note over S: extract it: no links or escaping paths,<br/>stored inner tarballs size-capped
   S->>B: edg-rematerialize-test-run diff
   Note over B: worktree read-only,<br/>build-staging writable,<br/>time-limited
   B-->>S: output, exit status
@@ -512,9 +512,9 @@ them. Several layers keep the service from reading through one:
   writes, which also may not hold hard links.
 - The section's directories, including the bench baselines a pull request
   supplies, must be real directories.
-- CI artifacts may not contain links or escaping paths, nor may the
-  `.rt.tar.gz` files inside them. Reading those tarballs later refuses link
-  members as well.
+- CI artifact ZIPs may not contain links or escaping paths. When a reviewer
+  opens a `.rt.tar.gz` file inside one, its entries are checked for links
+  and devices, excessive counts, and oversized members.
 - The handlers open files through `ConfinedPath`, which opens each path
   component with `O_NOFOLLOW` relative to the last, so a symlink anywhere
   on the way is refused, even one created after a check. A refused path
@@ -525,8 +525,10 @@ them. Several layers keep the service from reading through one:
 
 | Resource | Limit |
 | --- | --- |
-| Artifact download | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_BYTES` (256 MiB), also counting the decompressed size of inner tarballs |
-| Artifact entries | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_ENTRIES` (500,000), counting inner tarballs' too; and its zip's central directory, `ACKNOWLEDG_FORGE_MAX_ARTIFACT_DIRECTORY_BYTES` (64 MiB), checked before zipfile reads it |
+| Artifact download | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_BYTES` (256 MiB) |
+| Extracted artifact | `ACKNOWLEDG_FORGE_MAX_EXTRACTED_BYTES` (2 GiB), counting the stored size of inner tarballs, not their unextracted contents |
+| Artifact entries | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_ENTRIES` (500,000) for ZIP entries, 10,000 per inner tarball when opened; and its zip's central directory, `ACKNOWLEDG_FORGE_MAX_ARTIFACT_DIRECTORY_BYTES` (64 MiB), checked before zipfile reads it |
+| Inner tarball member | 64 MiB per member when a reviewer opens a tarball |
 | A pull request's diff of `tests/` | `ACKNOWLEDG_FORGE_MAX_DIFF_BYTES` (64 MiB) |
 | A review worktree's sparse-checkout patterns | `ACKNOWLEDG_FORGE_MAX_SPARSE_PATTERNS` (20,000) |
 | A file read for a reviewer | 64 MiB; a larger one fails the request |