fix: keep the module verification command parseable - #15
Conversation
The count-invariant message used single quotes inside the single-quoted phone command, truncating the assembled string at the device shell and failing every update at the verification step. The message is double-quoted, and the test fake now runs dash -n over the captured phone command so an unparseable string can never pass the suite again. Ports the Pixel 11 fix (observed live on its rc.11 update).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| push "$work/manifest" /data/local/tmp/eip-module-manifest | ||
| push "$work/names" /data/local/tmp/eip-module-names | ||
| phone 'tar -xf /data/local/tmp/eip-module-files.tar -C /data/adb/modules/eip-pixel8a-forge && chown -R 0:0 /data/adb/modules/eip-pixel8a-forge && cd /data/adb/modules/eip-pixel8a-forge && /data/adb/ksu/bin/busybox sha256sum -c /data/local/tmp/eip-module-manifest -s && find . -type f | sed "s|^\\./||" | LC_ALL=C sort | LC_ALL=C comm -23 - /data/local/tmp/eip-module-names | while IFS= read -r stale; do case "$stale" in disable|remove|update|skip_mount) continue ;; esac; rm -f "$stale"; done; n_names=$(wc -l < /data/local/tmp/eip-module-names); n_files=$(find . -type f | wc -l); n_markers=0; for m in disable remove update skip_mount; do [ -f "$m" ] && n_markers=$((n_markers + 1)); done; if [ "$n_files" -ne $((n_names + n_markers)) ]; then printf 'module tree contains unexpected files beyond the payload and module-state markers\n' >&2; exit 5; fi; rm -f /data/local/tmp/eip-module-files.tar /data/local/tmp/eip-module-manifest /data/local/tmp/eip-module-names; exit 0' \ | ||
| phone 'tar -xf /data/local/tmp/eip-module-files.tar -C /data/adb/modules/eip-pixel8a-forge && chown -R 0:0 /data/adb/modules/eip-pixel8a-forge && cd /data/adb/modules/eip-pixel8a-forge && /data/adb/ksu/bin/busybox sha256sum -c /data/local/tmp/eip-module-manifest -s && find . -type f | sed "s|^\\./||" | LC_ALL=C sort | LC_ALL=C comm -23 - /data/local/tmp/eip-module-names | while IFS= read -r stale; do case "$stale" in disable|remove|update|skip_mount) continue ;; esac; rm -f "$stale"; done; n_names=$(wc -l < /data/local/tmp/eip-module-names); n_files=$(find . -type f | wc -l); n_markers=0; for m in disable remove update skip_mount; do [ -f "$m" ] && n_markers=$((n_markers + 1)); done; if [ "$n_files" -ne $((n_names + n_markers)) ]; then printf "module tree contains unexpected files beyond the payload and module-state markers" >&2; exit 5; fi; rm -f /data/local/tmp/eip-module-files.tar /data/local/tmp/eip-module-manifest /data/local/tmp/eip-module-names; exit 0' \ |
There was a problem hiding this comment.
Minor regression introduced by this fix: switching the printf message from single to double quotes also dropped the trailing \n (printf "...markers" >&2 vs. the old printf '...markers\n' >&2). Since phone() runs this over adb shell -T with output passed straight through, the missing newline means this message will now run together with the subsequent die 'installed module files do not match the payload' output on the same terminal line when verification fails. Not functionally broken, but worth restoring the \n (e.g. printf "...markers\n" >&2) so the two error lines stay legible.
|
Reviewed against The fix is correct and addresses a real, pre-existing bug. In the old
That matches the PR's described symptom exactly (device-side "unmatched if", update failing closed) and I confirmed the truncation point by extracting the pre-fix blob directly ( One minor nit posted inline: the double-quote conversion also dropped the trailing No other correctness or regression issues found. |
| push "$work/manifest" /data/local/tmp/eip-module-manifest | ||
| push "$work/names" /data/local/tmp/eip-module-names | ||
| phone 'tar -xf /data/local/tmp/eip-module-files.tar -C /data/adb/modules/eip-pixel8a-forge && chown -R 0:0 /data/adb/modules/eip-pixel8a-forge && cd /data/adb/modules/eip-pixel8a-forge && /data/adb/ksu/bin/busybox sha256sum -c /data/local/tmp/eip-module-manifest -s && find . -type f | sed "s|^\\./||" | LC_ALL=C sort | LC_ALL=C comm -23 - /data/local/tmp/eip-module-names | while IFS= read -r stale; do case "$stale" in disable|remove|update|skip_mount) continue ;; esac; rm -f "$stale"; done; n_names=$(wc -l < /data/local/tmp/eip-module-names); n_files=$(find . -type f | wc -l); n_markers=0; for m in disable remove update skip_mount; do [ -f "$m" ] && n_markers=$((n_markers + 1)); done; if [ "$n_files" -ne $((n_names + n_markers)) ]; then printf 'module tree contains unexpected files beyond the payload and module-state markers\n' >&2; exit 5; fi; rm -f /data/local/tmp/eip-module-files.tar /data/local/tmp/eip-module-manifest /data/local/tmp/eip-module-names; exit 0' \ | ||
| phone 'tar -xf /data/local/tmp/eip-module-files.tar -C /data/adb/modules/eip-pixel8a-forge && chown -R 0:0 /data/adb/modules/eip-pixel8a-forge && cd /data/adb/modules/eip-pixel8a-forge && /data/adb/ksu/bin/busybox sha256sum -c /data/local/tmp/eip-module-manifest -s && find . -type f | sed "s|^\\./||" | LC_ALL=C sort | LC_ALL=C comm -23 - /data/local/tmp/eip-module-names | while IFS= read -r stale; do case "$stale" in disable|remove|update|skip_mount) continue ;; esac; rm -f "$stale"; done; n_names=$(wc -l < /data/local/tmp/eip-module-names); n_files=$(find . -type f | wc -l); n_markers=0; for m in disable remove update skip_mount; do [ -f "$m" ] && n_markers=$((n_markers + 1)); done; if [ "$n_files" -ne $((n_names + n_markers)) ]; then printf "module tree contains unexpected files beyond the payload and module-state markers\n" >&2; exit 5; fi; rm -f /data/local/tmp/eip-module-files.tar /data/local/tmp/eip-module-manifest /data/local/tmp/eip-module-names; exit 0' \ |
There was a problem hiding this comment.
Verified this fix and it's correct.
Before this change, the printf '...' inside the (already single-quoted) phone '...' argument closed the outer quote early. Locally in bash, that split the intended one-argument command into several word-split arguments; since phone() only reads $1, the remote script that actually reached the device was truncated right at ...then printf — an unterminated if. On a real device shell that's a syntax error, so su -c fails non-zero and the local || die 'installed module files do not match the payload' fires. Worse, by the time the parser hits that error, the preceding tar -xf, chown -R 0:0, and the stale-file rm -f "$stale" pruning loop have already run — so the module tree gets partially mutated even though the size/count safety check (the very if block meant to catch unexpected leftover files) and the final cleanup/exit 0 never execute.
Switching the inner printf to double quotes removes the only other literal ' in the string, so the whole thing is now one well-formed single-quoted argument again — I scanned it and confirmed there's exactly one opening and one closing '.
The new /bin/dash -n parse-check in tests/simple-installer.test.mjs (line 323) is well-targeted: it validates the exact string phone() hands to the device shell, which is precisely what would have caught this regression. ubuntu-latest (used in .github/workflows/check.yml) always ships /bin/dash, so no portability concern there.
Scope-wise this PR only touches these two files — no DEVICE.json/build-target, kernel config, Wi-Fi policy, Docker startup, or packaging changes, and nothing that looks like firmware/credentials/generated-artifact inclusion. No further issues found.
1 participant
Ports the Pixel 11's live-observed fix: single quotes inside the single-quoted phone command truncated the module verification string at the device shell ('unmatched if' on a real rc.11 update; the update failed closed). The message is double-quoted here too, and the test fake now runs dash -n over the captured phone command before simulating verification, so a quoting break can never pass the suite.