[GH-ISSUE #2220] [Bug] collect_adb_bugreport can overwrite an existing archive #739

New issue

Closed

opened 2026-10-15 18:27:51 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-15 18:27:51 +00:00

Owner

Originally created by @dakjdakd on GitHub (Oct 12, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/2220

collect_adb_bugreport can replace a ZIP that already exists in the caller's output directory. The operation is described as writing one new archive, but it currently delegates the final destination directly to adb bugreport, which can reuse a device-generated filename. If that name collides with existing evidence, the old bytes are lost before REA reads and hashes the resulting file.

This matters when an analyst keeps bugreports from multiple acquisitions in the same directory, retains a baseline report for a comparison, or has another collection writing into that directory. The request has no overwrite option, so choosing an existing directory does not express permission to replace its contents. A successful response and a valid digest for the new archive can conceal the loss: they describe the file after the overwrite, rather than establish that the prior evidence survived.

The implementation is available in the accompanying draft pull request.

Environment and boundary	Details
REA source revision	Affected behavior inspected at `58fb37207`; the fix is based on `e4d3e1399d9cd4932653e7015a1bf8589e56029d`, where the same acquisition path remains affected.
Runtime and platform	Local regression verification: Node 24.18.0, npm 11.16.0, Windows x64.
Area	Evidence, artifacts, or reconstruction; host-side ADB archive acquisition.
Interface	`collect_adb_bugreport` through MCP and the shared `collect-adb-bugreport` CLI workflow. The executed local regression calls the provider boundary directly.
Earliest failure stage	Capture or observation: ADB writes the archive before REA inspects or registers the resulting evidence.
Artifact context	A device-generated bugreport ZIP and an occupied host output filename. No Hopper, Ghidra, native-binary format, or browser dependency.

What triggers the problem

The necessary condition is a collision between the name ADB chooses for its new archive and an existing host file in output_directory. Reusing a directory alone is not enough to demonstrate the bug; the destination filename must collide. The same problem applies if the conflicting file appears while collection is running, which is relevant because a bugreport can take minutes.

For example, suppose /tmp/rea-bugreports/bugreport.zip already contains a previously collected report. A caller sends this tool request:

{
  "name": "collect_adb_bugreport",
  "arguments": {
    "serial": "device",
    "output_directory": "/tmp/rea-bugreports"
  }
}

If ADB writes the next report as bugreport.zip, the affected implementation invokes:

adb -s device bugreport /tmp/rea-bugreports

An ADB completion such as Bug report copied to /tmp/rea-bugreports/bugreport.zip is then accepted, and REA hashes that path. The original archive can already have been replaced at this point.

A controlled reproduction can use a caller-selected ADB fixture that responds to version, then writes new bugreport bytes to <the directory passed to bugreport>/bugreport.zip and returns that completion line. Pre-create bugreport.zip with distinct bytes, execute the request above against the affected revision, and compare the original file's bytes afterward. A second scenario creates the conflicting file during the fixture's collection step. These conditions isolate the host-side preservation problem without depending on a particular Android build's naming convention. The local verification described below uses simulated ADB replies; it does not establish a real-device reproduction.

Where the behavior comes from

The producing boundary is AdbProvider.collectBugreport. It prepares the caller's directory and passes that same directory to the external ADB command. It does not reserve an exclusive destination or isolate ADB's writes. After completion, it parses the reported path and hashes the file that now exists there. Detecting a collision at that later stage cannot restore the old archive.

The output_directory input contract says that REA writes exactly one new ZIP into the selected directory. Both the CLI and MCP tool reach the shared ADB provider, so the correction belongs at this producing boundary rather than in one adapter or in caller-side workarounds.

Expected outcome

An existing caller-owned file must retain its bytes, including when it appears during collection. A successful acquisition should still place one new ZIP directly in the caller's selected directory and return its actual local path, byte count, and SHA-256 digest. If publication needs a different name, the returned local_path should identify that new file while adb_reported_path preserves the path ADB actually reported. Cancellation and cleanup failures must keep meaningful failure information and any completed acquisition facts.

For the example above, the prior bugreport.zip should remain available and the new archive can be published as bugreport (1).zip. If that file also exists, publication can try bugreport (2).zip. The archive bytes remain the device-generated report; choosing a host filename does not alter or reinterpret its contents.

Proposed correction and evidence

The corresponding implementation collects into an atomically allocated staging directory inside the selected output directory. It accepts only a reported regular archive directly within that staging directory, then publishes it with an exclusive copy. A collision selects the next numeric suffix instead of overwriting a file. An existence check alone is insufficient because a file can appear between the check and publication; exclusive creation is the operation that enforces preservation.

The provider also owns staging cleanup. It uses the existing directory identity checks and cleanup retry mechanism, retaining a cleanup failure rather than letting it mask the original protocol/cancellation failure. If publication completed before a digest cancellation or cleanup failure, the error retains the final local path and the original producer path; completed digest facts are retained when available.

The executed regression covers existing names before collection and names appearing during collection, including an occupied first suffix. It verifies unchanged old bytes, the newly published archive's bytes and digest, distinct local/producer paths, and removal of staging output. Additional cases exercise cancelled digest verification, staging allocation failure, an out-of-staging reported path, successful publication followed by failed cleanup and retry, and a protocol failure accompanied by failed cleanup.

Local validation on Windows with Node 24.18.0 and npm 11.16.0:

npm run test:focused -- tests/boundary/providers/adb/adbBugreportPublication.test.ts src/adb/AdbProvider.test.ts — 7 regression tests passed.
npm run check:fast — passed.
npm run verify:test-discovery — passed; all 931 test files were discovered exactly once.
npm run check — passed on the rebased final implementation: all 6 Turbo tasks completed successfully, resolved module boundaries passed, and architecture guards passed across 1,210 source files.

The regressions exercise the Windows filesystem with simulated ADB output. Real-device verification is outside local coverage.

Originally created by @dakjdakd on GitHub (Oct 12, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/2220 `collect_adb_bugreport` can replace a ZIP that already exists in the caller's output directory. The operation is described as writing one **new** archive, but it currently delegates the final destination directly to `adb bugreport`, which can reuse a device-generated filename. If that name collides with existing evidence, the old bytes are lost before REA reads and hashes the resulting file. This matters when an analyst keeps bugreports from multiple acquisitions in the same directory, retains a baseline report for a comparison, or has another collection writing into that directory. The request has no overwrite option, so choosing an existing directory does not express permission to replace its contents. A successful response and a valid digest for the new archive can conceal the loss: they describe the file after the overwrite, rather than establish that the prior evidence survived. The implementation is available in the accompanying [draft pull request](https://github.com/morluto/rea/pull/2221). | Environment and boundary | Details | | --- | --- | | REA source revision | Affected behavior inspected at `58fb37207`; the fix is based on `e4d3e1399d9cd4932653e7015a1bf8589e56029d`, where the same acquisition path remains affected. | | Runtime and platform | Local regression verification: Node 24.18.0, npm 11.16.0, Windows x64. | | Area | Evidence, artifacts, or reconstruction; host-side ADB archive acquisition. | | Interface | `collect_adb_bugreport` through MCP and the shared `collect-adb-bugreport` CLI workflow. The executed local regression calls the provider boundary directly. | | Earliest failure stage | Capture or observation: ADB writes the archive before REA inspects or registers the resulting evidence. | | Artifact context | A device-generated bugreport ZIP and an occupied host output filename. No Hopper, Ghidra, native-binary format, or browser dependency. | ### What triggers the problem The necessary condition is a collision between the name ADB chooses for its new archive and an existing host file in `output_directory`. Reusing a directory alone is not enough to demonstrate the bug; the destination filename must collide. The same problem applies if the conflicting file appears while collection is running, which is relevant because a bugreport can take minutes. For example, suppose `/tmp/rea-bugreports/bugreport.zip` already contains a previously collected report. A caller sends this tool request: ```json { "name": "collect_adb_bugreport", "arguments": { "serial": "device", "output_directory": "/tmp/rea-bugreports" } } ``` If ADB writes the next report as `bugreport.zip`, the affected implementation invokes: ```text adb -s device bugreport /tmp/rea-bugreports ``` An ADB completion such as `Bug report copied to /tmp/rea-bugreports/bugreport.zip` is then accepted, and REA hashes that path. The original archive can already have been replaced at this point. A controlled reproduction can use a caller-selected ADB fixture that responds to `version`, then writes `new bugreport bytes` to `<the directory passed to bugreport>/bugreport.zip` and returns that completion line. Pre-create `bugreport.zip` with distinct bytes, execute the request above against the affected revision, and compare the original file's bytes afterward. A second scenario creates the conflicting file during the fixture's collection step. These conditions isolate the host-side preservation problem without depending on a particular Android build's naming convention. The local verification described below uses simulated ADB replies; it does not establish a real-device reproduction. ### Where the behavior comes from The producing boundary is [`AdbProvider.collectBugreport`](https://github.com/morluto/rea/blob/58fb37207/src/adb/AdbProvider.ts#L2074-L2151). It prepares the caller's directory and passes that same directory to the external ADB command. It does not reserve an exclusive destination or isolate ADB's writes. After completion, it parses the reported path and hashes the file that now exists there. Detecting a collision at that later stage cannot restore the old archive. The [`output_directory` input contract](https://github.com/morluto/rea/blob/58fb37207/src/domain/adb/adbDeviceAnalysis.ts#L187-L194) says that REA writes exactly one new ZIP into the selected directory. Both the CLI and MCP tool reach the shared ADB provider, so the correction belongs at this producing boundary rather than in one adapter or in caller-side workarounds. ### Expected outcome An existing caller-owned file must retain its bytes, including when it appears during collection. A successful acquisition should still place one new ZIP directly in the caller's selected directory and return its actual local path, byte count, and SHA-256 digest. If publication needs a different name, the returned `local_path` should identify that new file while `adb_reported_path` preserves the path ADB actually reported. Cancellation and cleanup failures must keep meaningful failure information and any completed acquisition facts. For the example above, the prior `bugreport.zip` should remain available and the new archive can be published as `bugreport (1).zip`. If that file also exists, publication can try `bugreport (2).zip`. The archive bytes remain the device-generated report; choosing a host filename does not alter or reinterpret its contents. ### Proposed correction and evidence The corresponding implementation collects into an atomically allocated staging directory inside the selected output directory. It accepts only a reported regular archive directly within that staging directory, then publishes it with an exclusive copy. A collision selects the next numeric suffix instead of overwriting a file. An existence check alone is insufficient because a file can appear between the check and publication; exclusive creation is the operation that enforces preservation. The provider also owns staging cleanup. It uses the existing directory identity checks and cleanup retry mechanism, retaining a cleanup failure rather than letting it mask the original protocol/cancellation failure. If publication completed before a digest cancellation or cleanup failure, the error retains the final local path and the original producer path; completed digest facts are retained when available. The executed regression covers existing names before collection and names appearing during collection, including an occupied first suffix. It verifies unchanged old bytes, the newly published archive's bytes and digest, distinct local/producer paths, and removal of staging output. Additional cases exercise cancelled digest verification, staging allocation failure, an out-of-staging reported path, successful publication followed by failed cleanup and retry, and a protocol failure accompanied by failed cleanup. Local validation on Windows with Node 24.18.0 and npm 11.16.0: - `npm run test:focused -- tests/boundary/providers/adb/adbBugreportPublication.test.ts src/adb/AdbProvider.test.ts` — 7 regression tests passed. - `npm run check:fast` — passed. - `npm run verify:test-discovery` — passed; all 931 test files were discovered exactly once. - `npm run check` — passed on the rebased final implementation: all 6 Turbo tasks completed successfully, resolved module boundaries passed, and architecture guards passed across 1,210 source files. The regressions exercise the Windows filesystem with simulated ADB output. Real-device verification is outside local coverage.

bsakaguchi

2026-10-15 18:27:51 +00:00