From 8310b083c0a70444ff779d78d52996da4fa0f36d Mon Sep 17 00:00:00 2001 From: saimskywalker Date: Thu, 27 Aug 2026 14:58:01 +0800 Subject: [PATCH 1/2] Close four paths that reported the wrong exit code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An outside-in pass over the repo, following the README literally and then exercising the whole exit-code contract. Six things came back. The important one first: a 64-bit ABI directory holding no .so printed PASS and exited 0. lib// can legitimately hold files that are not libraries — wrap.sh, for one — and such a run measured nothing at all while returning a green result, without naming the ABI anywhere in the report. That is exactly the failure the tool exists to refuse. It exits 2 now, and an ABI directory with no library in it is reported as [none] rather than omitted. objdump exiting non-zero was reported as [FAIL] no LOAD segments readable, under "Play will reject this upload" — a measurement announced about a file the tool never managed to open. That case is 2. A library where objdump SUCCEEDS and prints nothing usable is still [FAIL], which is the distinction CONTRIBUTING already draws. --ndk "" and --bundletool "" were treated as if the flag had not been passed, so --ndk "$UNSET_VAR" fell through to auto-detection after being handed an explicit NDK — the silent fallback --ndk exists to prevent. The --ndk= form already errored; the two forms agree now. Auto-detection picked the wrong NDK. The versions were sorted as whole paths on '.', which puts the directory prefix in field 1, where a numeric compare reads it as 0 on both BSD and GNU sort — so the major version was never compared. With 26.3.x, 27.0.x and 28.2.x installed it chose 26.3.x. There is no portable sort spelling of this (sort -V is GNU-only), so version_gt does it in bash. CI on main is red, and has been since the first push. test/run.sh disables SC2329 for its indirectly-invoked test bodies, but ubuntu-latest still ships shellcheck 0.9, which reports that finding as SC2317. Clean on a current local shellcheck, red on CI. Both codes are disabled now, and CONTRIBUTING notes the version gap. Finally, the install command in the README failed as written: it writes to root-owned /usr/local/bin without sudo, so a non-root user gets curl: (56) on macOS and curl: (23) on Ubuntu. It has sudo now, plus a ~/.local/bin form for people who would rather not. Six tests, each failing before its fix. Verified on macOS with shellcheck 0.11 and in ubuntu:24.04 with the 0.9 that CI installs: 40 passed, 0 failed and a clean lint on both. --- CHANGELOG.md | 27 ++++++++ CONTRIBUTING.md | 21 +++++- README.md | 26 ++++++-- android-16kb-check | 105 +++++++++++++++++++++++++----- test/fixtures/README.md | 1 + test/fixtures/empty-64bit-abi.apk | Bin 0 -> 518 bytes test/fixtures/make-fixtures.py | 8 +++ test/run.sh | 84 +++++++++++++++++++++++- 8 files changed, 247 insertions(+), 25 deletions(-) create mode 100644 test/fixtures/empty-64bit-abi.apk diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e32b7e..505fa49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,33 @@ looks barely different. ## [Unreleased] +### Fixed + +- An artifact whose 64-bit ABI directory contains no `.so` no longer reports + `PASS` and exits `0`. `lib//` can hold files that are not libraries — + `wrap.sh`, for one — and such a run measured nothing at all while printing + a green result. It exits `2`, and the ABI directory is now reported as + `[none]` instead of being omitted. +- `objdump` exiting non-zero is now `2` rather than `1`. A library the tool + could not read was being reported as `[FAIL] no LOAD segments readable` + under "Play will reject this upload", which claims a measurement that never + happened. A library where objdump *succeeds* and prints no readable LOAD + segments is still `[FAIL]`. +- `--ndk ""` and `--bundletool ""` are rejected instead of being treated as if + the flag were absent. `--ndk "$UNSET_VAR"` used to fall through to + auto-detection, which is the silent fallback `--ndk` exists to prevent. The + `--ndk=` form already errored; the two forms now agree. +- Auto-detection picks the highest installed NDK again. Versions were sorted as + whole paths on `.`, which put the directory prefix in field 1 where a numeric + compare reads it as `0`, so the major version was never compared: with + 26.3.x, 27.0.x and 28.2.x installed it chose **26.3.x**. +- CI is green on `ubuntu-latest` again. `test/run.sh` disabled only `SC2329` + for its indirectly-invoked test bodies; the shellcheck 0.9 that Ubuntu ships + reports the same finding as `SC2317`, so the lint step failed there while + being clean on a current local shellcheck. +- The documented install command works as written. It wrote to root-owned + `/usr/local/bin` without `sudo`, so it failed with `curl: (56)`. + ## [1.0.0] - 2026-08-27 First release. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9569078..20e1ff1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -20,7 +20,14 @@ CI runs the same two commands on `ubuntu-latest`, plus a check that the script is still executable and that a run with no toolchain exits 2. shellcheck must be clean — not "clean apart from the informational ones". If a finding is genuinely wrong, disable it by number with a comment saying why, the way -`test/run.sh` does for `SC2329`. +`test/run.sh` does for `SC2317,SC2329`. + +Mind the version gap while you are there: `ubuntu-latest` still installs +shellcheck 0.9, and a current local shellcheck is 0.11. They do not always use +the same code for the same finding — indirectly-invoked functions are SC2317 +on 0.9 and SC2329 from 0.10 onward — so a disable that names only one of a +pair is clean on the laptop and red on CI. Disabling both is harmless; an +unknown code is ignored rather than reported. ## The exit codes are the interface @@ -77,7 +84,17 @@ authoritative, and quietly using a different one is worse than failing. **Parsing objdump output.** The ELF check reads `align 2**N` out of `objdump -p`. GNU objdump and llvm-objdump both print that form today; a change in either could silently produce zero matches, which is why a library with no -readable LOAD segments is a `[FAIL]` and not a shrug. +readable LOAD segments is a `[FAIL]` and not a shrug. That is specifically the +case where objdump *ran* and its output held nothing usable. objdump exiting +non-zero is the other case and is a `2`: nothing was measured, so there is no +verdict to report. Keep those two apart — reading a non-zero exit as `[FAIL]` +announces "Play will reject this upload" about a file the tool never opened. + +**Version comparison.** There is no portable `sort` spelling of it. `sort -V` +is GNU-only, and `sort -t. -k1,1n` silently compares nothing but the minor +field when the input is a path rather than a bare version, because the +directory prefix in field 1 reads as `0` under both BSD and GNU. `version_gt` +does it in bash for that reason; prefer it to adding a `sort` call. **Anything printed to stdout on the passing path.** People pipe this into build scripts. Keep failures and diagnostics on stderr. diff --git a/README.md b/README.md index fba7bcd..17b408e 100644 --- a/README.md +++ b/README.md @@ -80,15 +80,33 @@ Cases that deliberately return `2` rather than `0`: measure. - An artifact containing only 32-bit ABIs. That proves nothing about the 64-bit build users will install. +- An artifact whose 64-bit ABI directory holds no `.so` at all — `lib//` + can legitimately contain other files, such as `wrap.sh`. The directory is + reported as `[none]`, because an ABI that appears nowhere in the report + reads exactly like one that passed. +- `objdump` present but unable to read a library. That is a tool that could + not look, not a library that failed; a failure to measure never becomes a + measured failure. ## Install It is one file with no dependencies beyond the Android toolchain it calls. ```bash -curl -fsSLo /usr/local/bin/android-16kb-check \ +sudo curl -fsSLo /usr/local/bin/android-16kb-check \ https://raw.githubusercontent.com/saimskywalker/android-16kb-check/main/android-16kb-check -chmod +x /usr/local/bin/android-16kb-check +sudo chmod +x /usr/local/bin/android-16kb-check +``` + +`/usr/local/bin` is root-owned on macOS and on a stock Ubuntu, so without +`sudo` curl exits `56` having written nothing. To install without root, put it +somewhere you own and make sure that directory is on `PATH`: + +```bash +mkdir -p ~/.local/bin +curl -fsSLo ~/.local/bin/android-16kb-check \ + https://raw.githubusercontent.com/saimskywalker/android-16kb-check/main/android-16kb-check +chmod +x ~/.local/bin/android-16kb-check ``` Or clone the repository and run `./android-16kb-check` in place. @@ -111,8 +129,8 @@ android-16kb-check --bundletool ~/tools/bundletool-all.jar app-release.aab | option | effect | |---|---| -| `--ndk ` | Take `llvm-objdump` from this NDK. Overrides auto-detection; if the directory has no `llvm-objdump` the run fails rather than falling back to some other one. | -| `--bundletool ` | Executable, or a path ending in `.jar` which is run as `java -jar`. AAB only. | +| `--ndk ` | Take `llvm-objdump` from this NDK. Overrides auto-detection; if the directory has no `llvm-objdump` the run fails rather than falling back to some other one. An empty value is an error, not "no `--ndk` given" — `--ndk "$UNSET_VAR"` must not silently become auto-detection. | +| `--bundletool ` | Executable, or a path ending in `.jar` which is run as `java -jar`. AAB only. An empty value is an error, as for `--ndk`. | | `-h`, `--help` | Usage, then exit 0. | | `-V`, `--version` | Version, then exit 0. | diff --git a/android-16kb-check b/android-16kb-check index 508c230..5c60f6b 100755 --- a/android-16kb-check +++ b/android-16kb-check @@ -110,8 +110,13 @@ elif [[ -n "${ANDROID_NDK_ROOT:-}" ]]; then fi need_value() { - # $1 = flag name, $2 = number of args remaining after the flag - if (( $2 == 0 )); then + # $1 = flag name, $2 = number of args remaining after the flag, $3 = the + # value itself. An EMPTY value is rejected as hard as an absent one: an + # unset variable in a caller's script (--ndk "$NDK_HOME") would otherwise + # look like "no --ndk at all", and the run would quietly fall back to + # auto-detection after being told which toolchain to use. That is the + # silent-fallback failure this tool exists to refuse. + if (( $2 == 0 )) || [[ -z "${3:-}" ]]; then cannot_verify "$1 requires a value." "Run ${PROGRAM} --help for usage." fi } @@ -126,10 +131,10 @@ while (( $# )); do --) end_of_options=1 ;; -h|--help) usage; exit "${EXIT_ALIGNED}" ;; -V|--version) printf '%s %s\n' "${PROGRAM}" "${VERSION}"; exit "${EXIT_ALIGNED}" ;; - --ndk) need_value --ndk $(( $# - 1 )); NDK_DIR="$2"; NDK_SOURCE="--ndk"; shift ;; + --ndk) need_value --ndk $(( $# - 1 )) "${2:-}"; NDK_DIR="$2"; NDK_SOURCE="--ndk"; shift ;; --ndk=*) NDK_DIR="${1#--ndk=}"; NDK_SOURCE="--ndk" [[ -n "${NDK_DIR}" ]] || cannot_verify "--ndk requires a value." ;; - --bundletool) need_value --bundletool $(( $# - 1 )); BUNDLETOOL_SPEC="$2"; shift ;; + --bundletool) need_value --bundletool $(( $# - 1 )) "${2:-}"; BUNDLETOOL_SPEC="$2"; shift ;; --bundletool=*) BUNDLETOOL_SPEC="${1#--bundletool=}" [[ -n "${BUNDLETOOL_SPEC}" ]] || cannot_verify "--bundletool requires a value." ;; -*) cannot_verify "unknown option: $1" "Run ${PROGRAM} --help for usage." ;; @@ -195,18 +200,48 @@ objdump_in_ndk() { return 1 } -# Highest installed NDK, by numeric version fields rather than string order, so -# 28.2.x sorts above 9.x. sort -V is deliberately avoided; it is not portable. +# Is dotted version $1 strictly greater than dotted version $2? +# +# Done in bash rather than with `sort`, because every portable spelling of this +# has a trap in it. `sort -V` is a GNU extension. `sort -t. -k1,1n` looks right +# but only works on bare versions: fed whole PATHS it puts the directory prefix +# in field 1, where a numeric compare reads it as 0 on both BSD and GNU, so the +# major version is never compared at all and 26.3.x beats 28.2.x. +version_gt() { + local -a a b + IFS=. read -r -a a <<<"$1" + IFS=. read -r -a b <<<"$2" + local i x y n=$(( ${#a[@]} > ${#b[@]} ? ${#a[@]} : ${#b[@]} )) + for (( i = 0; i < n; i++ )); do + x="${a[i]:-0}" + y="${b[i]:-0}" + [[ "${x}" =~ ^[0-9]+$ ]] || x=0 + [[ "${y}" =~ ^[0-9]+$ ]] || y=0 + # 10# so a zero-padded field is not read as octal. + if (( 10#${x} > 10#${y} )); then return 0; fi + if (( 10#${x} < 10#${y} )); then return 1; fi + done + return 1 +} + +# Highest installed NDK, compared by numeric version fields rather than string +# order, so 28.2.x wins over 9.x and over 26.3.x. # Prints nothing when the SDK has no ndk directory. highest_ndk() { - local root="$1" dir - local -a dirs=() + local root="$1" dir name best="" best_name="" for dir in "${root}"/ndk/*/; do [[ -d "${dir}" ]] || continue - dirs+=("${dir%/}") + dir="${dir%/}" + name="${dir##*/}" + if [[ -z "${best}" ]] || version_gt "${name}" "${best_name}"; then + best="${dir}" + best_name="${name}" + fi done - (( ${#dirs[@]} )) || return 0 - printf '%s\n' "${dirs[@]}" | sort -t. -k1,1n -k2,2n -k3,3n | tail -1 + if [[ -n "${best}" ]]; then + printf '%s\n' "${best}" + fi + return 0 } OBJDUMP="" @@ -370,7 +405,8 @@ fi printf '==> ELF LOAD segment alignment (must be >= 2**14)\n' fail=0 -found64=0 +abis64=0 # 64-bit ABI directories present in the archive +measured=0 # 64-bit .so files actually read for abi_dir in "${LIB_ROOT}"/*; do [[ -d "${abi_dir}" ]] || continue abi="$(basename "${abi_dir}")" @@ -382,18 +418,37 @@ for abi_dir in "${LIB_ROOT}"/*; do printf ' %-14s [SKIP] 32-bit, exempt (%s libs)\n' "${abi}" "${n}" continue fi - found64=1 + abis64=$(( abis64 + 1 )) + in_this_abi=0 for so in "${abi_dir}"/*.so; do [[ -f "${so}" ]] || continue so_name="$(basename "${so}")" + in_this_abi=$(( in_this_abi + 1 )) + measured=$(( measured + 1 )) # objdump -p prints one 'align 2**N' per program header. Only LOAD # matters; anything below 2**14 breaks on a 16 KB-page device. - if ! aligns="$("${OBJDUMP}" -p "${so}" 2>/dev/null \ - | awk '$1=="LOAD"{for(i=1;i<=NF;i++) if($i=="align") print $(i+1)}')"; then - aligns="" + # + # objdump's exit status is read on its own, before any parsing. The two + # cases below look identical in the output — both produce no 'align' + # tokens — but they are opposite answers: + # + # objdump ran and found no readable LOAD segments -> the ARTIFACT is + # broken, which is a [FAIL]. + # objdump could not run at all -> nothing was measured, which is a 2. + # + # Collapsing the second into the first reports "Play will reject this + # upload" about a library the tool never managed to look at. + if ! raw="$("${OBJDUMP}" -p "${so}" 2>/dev/null)"; then + cannot_verify "${OBJDUMP##*/} could not read ${abi}/${so_name}." \ + "Command: ${OBJDUMP} -p <${so_name}>" \ + "Nothing was measured for this library, so the artifact is unproven." \ + "Check that the objdump above runs on this machine, and that the" \ + "artifact is not truncated or corrupt." fi + aligns="$(printf '%s\n' "${raw}" \ + | awk '$1=="LOAD"{for(i=1;i<=NF;i++) if($i=="align") print $(i+1)}')" worst=99 while read -r token; do @@ -416,14 +471,30 @@ EOF printf ' %-14s %-38s [ok] min align 2**%s\n' "${abi}" "${so_name}" "${worst}" fi done + + # A 64-bit ABI directory holding no .so is not the same as one that passed, + # and it must not be reported by saying nothing. lib// can legitimately + # hold non-library files (wrap.sh, for one), so this shape does occur. + if (( in_this_abi == 0 )); then + printf ' %-14s [none] directory present but holds no .so\n' "${abi}" >&2 + fi done printf '\n' -if (( found64 == 0 )); then +if (( abis64 == 0 )); then cannot_verify "the artifact contains no 64-bit ABI, so there is nothing to check." \ "16 KB pages only apply to arm64-v8a and x86_64." \ "A 32-bit-only build is not proof of anything about the 64-bit one." fi +# A 64-bit directory that yielded no library leaves the run with a verdict on +# nothing. Reporting PASS here would be the exact failure this tool exists to +# refuse: a green result from an artifact that was never measured. +if (( measured == 0 )); then + cannot_verify "no 64-bit library was read, so nothing was measured." \ + "The artifact has a 64-bit ABI directory, but it contains no .so files." \ + "Either this is not the artifact you meant to check, or its 64-bit" \ + "native code is missing. Neither is evidence of alignment." +fi if (( fail )); then printf 'FAIL — at least one 64-bit library is not 16 KB aligned.\n' >&2 printf ' Play will reject this upload.\n' >&2 diff --git a/test/fixtures/README.md b/test/fixtures/README.md index e2c593d..00a60a7 100644 --- a/test/fixtures/README.md +++ b/test/fixtures/README.md @@ -10,6 +10,7 @@ matters here is only where the entries sit inside the archive. | `sample.aab` | the same, nested under `base/lib/` as an AAB stores it | | `no-native-libs.apk` | no `lib/` directory at all | | `only-32bit.apk` | `lib/armeabi-v7a/` only, no 64-bit ABI | +| `empty-64bit-abi.apk` | `lib/arm64-v8a/` holding only a `wrap.sh`, so there is a 64-bit ABI directory but no 64-bit library to measure | Committed as binaries so the suite needs no zip tool. Regenerate with `make-fixtures.py` if the layout ever has to change. diff --git a/test/fixtures/empty-64bit-abi.apk b/test/fixtures/empty-64bit-abi.apk new file mode 100644 index 0000000000000000000000000000000000000000..230e0dc4c064c1be3042b219af8563e09b0ea306 GIT binary patch literal 518 zcmWIWW@Zs#0D;+M(y?oF5=uFMY!DU(V#mCcqWsJh-^9GkwAA7fy^7qNw9JZ<(xOzZ z02GZ;QIP`DKn)-)3dA{?N&1OJxn?H1WfqD0Y0mJmZX06nY_ m23P`_SVIoo2=o9#7}1U}0&8#uc(byBR5JtNIUubBb~ONJ185Qe literal 0 HcmV?d00001 diff --git a/test/fixtures/make-fixtures.py b/test/fixtures/make-fixtures.py index e006483..04bd004 100644 --- a/test/fixtures/make-fixtures.py +++ b/test/fixtures/make-fixtures.py @@ -33,3 +33,11 @@ def make(path, entries): ("AndroidManifest.xml", b"fixture\n"), ("lib/armeabi-v7a/liblegacy.so", SO), ]) +# A 64-bit ABI directory that holds no .so. wrap.sh is a real file Android +# looks for at lib//wrap.sh, so this shape is not hypothetical. There is +# no 64-bit library here to measure, which makes it a 2 and not a pass. +make("empty-64bit-abi.apk", [ + ("AndroidManifest.xml", b"fixture\n"), + ("lib/arm64-v8a/wrap.sh", b"#!/system/bin/sh\nexec \"$@\"\n"), + ("lib/armeabi-v7a/liblegacy.so", SO), +]) diff --git a/test/run.sh b/test/run.sh index 0035fc2..976ab6c 100755 --- a/test/run.sh +++ b/test/run.sh @@ -15,8 +15,11 @@ # Test bodies and assertion helpers are invoked indirectly, by name, from # check(). ShellCheck cannot see that, so it reports every one of them as dead -# code. -# shellcheck disable=SC2329 +# code. BOTH codes are needed: shellcheck <= 0.9 reports it as SC2317, and +# 0.10 onward split the function case out into SC2329. Disabling only SC2329 +# is clean on a current local shellcheck and red on ubuntu-latest, which still +# ships 0.9. +# shellcheck disable=SC2317,SC2329 set -uo pipefail @@ -302,6 +305,77 @@ t_artifact_without_native_libs_is_two() { assert_not_out "PASS" || return 1 } +t_sixtyfour_bit_dir_with_no_libs_is_two() { + # lib/arm64-v8a/ exists but holds no .so — only a wrap.sh. Nothing 64-bit was + # measured, so there is no basis for a pass. Before this was fixed the run + # printed PASS and exited 0 without naming arm64-v8a anywhere: a green gate + # over an artifact the tool had not read. + run_tool "${FIXTURES}/empty-64bit-abi.apk" + assert_rc 2 || return 1 + assert_out "nothing was measured" || return 1 + assert_not_out "PASS" || return 1 +} + +t_sixtyfour_bit_dir_with_no_libs_is_reported() { + # And it has to be said out loud. A 64-bit ABI mentioned nowhere in the + # report reads exactly like one that passed. + run_tool "${FIXTURES}/empty-64bit-abi.apk" + assert_out "arm64-v8a" || return 1 + assert_out "[none]" || return 1 +} + +t_objdump_that_cannot_run_is_two_not_one() { + # objdump present but unable to read the file. Nothing was measured, so this + # is "could not verify", not "Play will reject this upload" — the second + # sends someone hunting an alignment bug that was never observed. + TEST_ENV="STUB_OBJDUMP_FAIL=1" + run_tool "${FIXTURES}/sample.apk" + assert_rc 2 || return 1 + assert_out "could not read" || return 1 + assert_not_out "Play will reject" || return 1 + assert_not_out "PASS" || return 1 +} + +t_empty_ndk_value_is_rejected() { + # --ndk "$UNSET_VAR" must not degrade into "no --ndk given". Falling back to + # auto-detection after being handed an explicit NDK is the silent-fallback + # behaviour --ndk exists to prevent. + run_tool --ndk "" "${FIXTURES}/sample.apk" + assert_rc 2 || return 1 + assert_out "--ndk requires a value" || return 1 + assert_not_out "PASS" || return 1 +} + +t_empty_bundletool_value_is_rejected() { + run_tool --bundletool "" "${FIXTURES}/sample.aab" + assert_rc 2 || return 1 + assert_out "--bundletool requires a value" || return 1 + assert_not_out "PASS" || return 1 +} + +t_highest_ndk_wins() { + # 26.3.x must not beat 28.2.x. Sorting whole paths on '.' put the directory + # prefix in field 1, where a numeric compare reads it as 0 on both BSD and + # GNU sort, so the major version was never compared and the run silently + # used an older NDK than the one it advertises. The '==> objdump' header + # names the NDK that was chosen, so assert on that. + local sdk="${TMPROOT}/sdk-many-ndks" + local v bin + rm -rf "${sdk}" + for v in 9.0.8775105 26.3.11579264 27.0.12077973 28.2.13676358; do + bin="${sdk}/ndk/${v}/toolchains/llvm/prebuilt/linux-x86_64/bin" + mkdir -p "${bin}" + ln -sf "${STUBS}/llvm-objdump" "${bin}/llvm-objdump" + done + # No objdump on PATH, so the only way to resolve one is through the SDK. + TEST_PATH="${BARE_BIN}" + TEST_ENV="ANDROID_HOME=${sdk} BUNDLETOOL=${STUBS}/bundletool" + run_tool "${FIXTURES}/sample.aab" + assert_rc 0 || return 1 + assert_out "ndk/28.2.13676358/" || return 1 + assert_not_out "ndk/26.3.11579264/" || return 1 +} + t_only_32bit_is_two_not_zero() { # A 32-bit-only artifact proves nothing about the 64-bit build users install. run_tool "${FIXTURES}/only-32bit.apk" @@ -428,6 +502,12 @@ check "missing zipalign exits 2" t_missing_zipalign_is_two check "missing bundletool jar exits 2" t_missing_bundletool_jar_is_two check "an artifact with no .so exits 2" t_artifact_without_native_libs_is_two check "a 32-bit-only artifact exits 2, not 0" t_only_32bit_is_two_not_zero +check "a 64-bit dir with no .so exits 2, not 0" t_sixtyfour_bit_dir_with_no_libs_is_two +check "a 64-bit dir with no .so is reported" t_sixtyfour_bit_dir_with_no_libs_is_reported +check "an objdump that cannot run exits 2, not 1" t_objdump_that_cannot_run_is_two_not_one +check "an empty --ndk value is rejected" t_empty_ndk_value_is_rejected +check "an empty --bundletool value is rejected" t_empty_bundletool_value_is_rejected +check "the highest installed NDK is chosen" t_highest_ndk_wins check "an aligned APK exits 0" t_apk_aligned_passes check "32-bit ABIs are reported as SKIP" t_apk_reports_32bit_as_skip check "over-aligned libraries pass" t_apk_over_aligned_passes From f31c58cded30f89bfe12d6b17bd58af86b00d17d Mon Sep 17 00:00:00 2001 From: saimskywalker Date: Thu, 27 Aug 2026 15:44:02 +0800 Subject: [PATCH 2/2] Read zipalign's and unzip's exit status, not just their output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more call sites had the failure-to-measure/measured-failure conflation this branch already fixed for objdump. zipalign: any non-zero status was read as "not aligned". Build-tools older than 35 has no -P flag and exits 2 on the usage error, so a genuinely 16 KB-aligned APK came back as exit 1 under "Play will reject this upload" from a tool that had never checked it. Verified against a real build-tools;34.0.0: it prints "illegal option -- P" and exits 2, while 36.1.0 returns 1 for a real misalignment. 1 stays exit 1; anything else is now exit 2. unzip: its status was discarded with `|| true`, so an archive it could only partly extract was measured on whatever came out. An APK with two 64-bit .so, one of them stored with a compression method unzip cannot decode, extracted one library and reported PASS with exit 0 — reproduced identically on macOS and on ubuntu:24.04. 0, 1 and 11 (nothing matched the glob, which the missing-lib-root check reports properly) still pass through; anything else is exit 2. Two tests, both confirmed failing before the change: FAIL a zipalign that cannot run exits 2, not 1 expected exit 2, got 1 FAIL a partly-extractable archive exits 2 expected exit 2, got 0 No new path reaches 0, and no path that reached 0 legitimately stopped: a real aligned APK still exits 0 and a real unaligned one still exits 1 against NDK 28.2.13676358 and build-tools 36.1.0. --- CHANGELOG.md | 10 ++++++++ CONTRIBUTING.md | 7 ++++++ README.md | 5 ++++ android-16kb-check | 37 ++++++++++++++++++++++++++--- test/fixtures/README.md | 1 + test/fixtures/make-fixtures.py | 32 +++++++++++++++++++++++++ test/fixtures/unreadable-entry.apk | Bin 0 -> 590 bytes test/run.sh | 27 +++++++++++++++++++++ test/stubs/zipalign | 17 +++++++++---- 9 files changed, 128 insertions(+), 8 deletions(-) create mode 100644 test/fixtures/unreadable-entry.apk diff --git a/CHANGELOG.md b/CHANGELOG.md index 505fa49..563158d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,16 @@ looks barely different. whole paths on `.`, which put the directory prefix in field 1 where a numeric compare reads it as `0`, so the major version was never compared: with 26.3.x, 27.0.x and 28.2.x installed it chose **26.3.x**. +- `zipalign` failing to run is now `2` rather than `1`. Its status separates + the two cases — `1` means it read the archive and found it misaligned, + anything else means it never got that far — and build-tools older than 35 + has no `-P` flag at all, so it exits `2` on the usage error. A correctly + aligned APK was being reported as "Play will reject this upload" by a tool + that had measured nothing. Same distinction the `objdump` entry above draws. +- A partial extraction is `2` rather than a `PASS` over what did come out. + `unzip`'s exit status was discarded, so an archive holding an entry unzip + refuses — an unsupported compression method, a truncated file, a full + `${TMPDIR}` — was measured on the subset that extracted and reported green. - CI is green on `ubuntu-latest` again. `test/run.sh` disabled only `SC2329` for its indirectly-invoked test bodies; the shellcheck 0.9 that Ubuntu ships reports the same finding as `SC2317`, so the lint step failed there while diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 20e1ff1..10ba803 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -90,6 +90,13 @@ non-zero is the other case and is a `2`: nothing was measured, so there is no verdict to report. Keep those two apart — reading a non-zero exit as `[FAIL]` announces "Play will reject this upload" about a file the tool never opened. +**Any tool's exit status.** The objdump rule above is the general one, and it +applies to `zipalign` and `unzip` too: a status that means "I could not do the +check" must never be read as "the check failed". `zipalign` returns `1` for a +misaligned archive and something else when it never looked — build-tools below +35 has no `-P` and exits `2`. `unzip` returning anything but `0`, `1` or `11` +means it skipped entries, so the extracted tree is a subset of the artifact. + **Version comparison.** There is no portable `sort` spelling of it. `sort -V` is GNU-only, and `sort -t. -k1,1n` silently compares nothing but the minor field when the input is a path rather than a bare version, because the diff --git a/README.md b/README.md index 17b408e..df25bf5 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,11 @@ Cases that deliberately return `2` rather than `0`: - `objdump` present but unable to read a library. That is a tool that could not look, not a library that failed; a failure to measure never becomes a measured failure. +- `zipalign` present but unable to run the check. Build-tools older than 35 + has no `-P` flag, so its zipalign exits on a usage error rather than + reporting an alignment; that is not an answer about the archive. +- An archive `unzip` could only partly extract. Whatever came out is a subset + of the artifact, and a verdict on a subset is not a verdict on the upload. ## Install diff --git a/android-16kb-check b/android-16kb-check index 5c60f6b..10c3cb9 100755 --- a/android-16kb-check +++ b/android-16kb-check @@ -368,15 +368,30 @@ if [[ "${KIND}" == "aab" ]]; then exit "${EXIT_UNALIGNED}" fi else - if "${ZIPALIGN}" -c -P 16 4 "${ARTIFACT}" >/dev/null 2>&1; then + # zipalign's status is read on its own, for the same reason objdump's is + # below: 1 means it checked the archive and found it misaligned, and + # anything above that means it never got as far as checking. The case that + # actually happens is build-tools older than 35, whose zipalign has no -P + # flag at all and exits 2 on the usage error — reading that as "not + # aligned" announces a rejection about an archive nobody measured. + zipalign_status=0 + "${ZIPALIGN}" -c -P 16 4 "${ARTIFACT}" >/dev/null 2>&1 || zipalign_status=$? + if (( zipalign_status == 0 )); then printf ' [ok] zipalign -c -P 16 4: verification successful\n' - else + elif (( zipalign_status == 1 )); then printf ' [FAIL] zipalign reports the APK is not 16 KB aligned\n' >&2 "${ZIPALIGN}" -v -c -P 16 4 "${ARTIFACT}" 2>&1 | grep -i 'bad\|fail' >&2 || true printf '\n' >&2 printf 'FAIL — native libraries are not stored 16 KB-aligned in the zip.\n' >&2 printf ' This comes from the Android Gradle Plugin. Upgrade to AGP 8.5.1 or newer.\n' >&2 exit "${EXIT_UNALIGNED}" + else + cannot_verify "zipalign could not perform the check (exit ${zipalign_status})." \ + "It returns 1 when it has read the archive and found it misaligned;" \ + "any other status means it never got that far, so nothing was measured." \ + "The usual cause is build-tools older than 35, whose zipalign has no" \ + "-P flag: ${ZIPALIGN}" \ + "Install a newer one with: sdkmanager 'build-tools;35.0.0'" fi fi printf '\n' @@ -394,7 +409,23 @@ else LIB_GLOB='lib/*' LIB_ROOT="${WORK}/lib" fi -unzip -q "${ARTIFACT}" "${LIB_GLOB}" -d "${WORK}" >/dev/null 2>&1 || true +# unzip's status is not discarded. If it refuses even one entry — an +# unsupported compression method, a truncated archive, a full ${TMPDIR} — the +# tree below is a SUBSET of the artifact, and measuring a subset while +# printing PASS is the same green-without-looking answer as measuring +# nothing. 0 is a clean extraction, 1 is "extracted, with warnings", and 11 +# is "nothing matched the glob", which the missing-LIB_ROOT check just below +# reports properly. +unzip_status=0 +unzip -q "${ARTIFACT}" "${LIB_GLOB}" -d "${WORK}" >/dev/null 2>&1 || unzip_status=$? +case "${unzip_status}" in + 0|1|11) ;; + *) cannot_verify "unzip could not read every ${LIB_GLOB} entry out of the artifact (exit ${unzip_status})." \ + "Part of the archive was skipped, so a verdict here would cover some" \ + "of the native libraries rather than all of them." \ + "Usual causes: a compression method this unzip does not support, a" \ + "truncated or corrupt archive, or no space left in \${TMPDIR}." ;; +esac if [[ ! -d "${LIB_ROOT}" ]]; then cannot_verify "no native libraries found in the artifact." \ diff --git a/test/fixtures/README.md b/test/fixtures/README.md index 00a60a7..238e0e3 100644 --- a/test/fixtures/README.md +++ b/test/fixtures/README.md @@ -11,6 +11,7 @@ matters here is only where the entries sit inside the archive. | `no-native-libs.apk` | no `lib/` directory at all | | `only-32bit.apk` | `lib/armeabi-v7a/` only, no 64-bit ABI | | `empty-64bit-abi.apk` | `lib/arm64-v8a/` holding only a `wrap.sh`, so there is a 64-bit ABI directory but no 64-bit library to measure | +| `unreadable-entry.apk` | two 64-bit `.so`, one of them stored with a compression method `unzip` cannot decode, so extraction is necessarily partial | Committed as binaries so the suite needs no zip tool. Regenerate with `make-fixtures.py` if the layout ever has to change. diff --git a/test/fixtures/make-fixtures.py b/test/fixtures/make-fixtures.py index 04bd004..7e25b15 100644 --- a/test/fixtures/make-fixtures.py +++ b/test/fixtures/make-fixtures.py @@ -4,6 +4,7 @@ The .so payloads are deliberately not valid ELF. The tests stub llvm-objdump, so the only thing these fixtures need to get right is the archive layout. """ +import struct import zipfile SO = b"\x7fELF\x02\x01\x01\x00" + b"\x00" * 56 + b"not-a-real-elf-body-fixture-only\n" @@ -41,3 +42,34 @@ def make(path, entries): ("lib/arm64-v8a/wrap.sh", b"#!/system/bin/sh\nexec \"$@\"\n"), ("lib/armeabi-v7a/liblegacy.so", SO), ]) + +# An archive holding a 64-bit .so that unzip will refuse to extract: its +# compression method is patched to one Info-ZIP does not implement, which is +# how a partial extraction is produced without depending on a corrupt file. +# The point is only that unzip skips an entry and says so in its exit status; +# a full disk or a truncated archive produce the same shape. One 64-bit +# library is readable and one is not, so a run that ignores unzip's status +# measures half the artifact and calls it a pass. +make("unreadable-entry.apk", [ + ("AndroidManifest.xml", b"fixture\n"), + ("lib/arm64-v8a/libgood.so", SO), + ("lib/arm64-v8a/libopaque.so", SO), +]) + +def patch_method(path, entry, method=14): + """Rewrite one entry's compression method, in both headers that carry it.""" + data = bytearray(open(path, "rb").read()) + for magic, name_off, method_off in ((b"PK\x03\x04", 26, 8), (b"PK\x01\x02", 28, 10)): + i = 0 + while True: + i = data.find(magic, i) + if i < 0: + break + nlen = struct.unpack_from("DV&2 + echo "ERROR: unknown flag -?" >&2 + echo "Usage: zipalign [-f] [-p] [-v] [-z] infile.zip outfile.zip" >&2 ;; +esac exit "${rc}"