Add patch-audit reference; extend pitfalls to P30; unify script docs in English
- references/patch-audit.md (new): two independent questions for any self-made patch — did it land, and is it legal. Length-vs-bytes comparison and the ~800-false-positive signal that makes byte diffing useless; the equal-length-replacement blind spot that hides patches from length-only audits (and why tier-2 checking must be driven by the patch set's own declared targets, not by tier-1 output); verifier-level legality (move-result must immediately follow its producer) checked as a set-difference so it runs before the device; text-matching patch traps (whitespace, line endings, instruction names, anchor uniqueness, re-emitted old fragments, one block per doc); and auditing the patch set rather than a patch. - pitfalls P28-P30: stale events-buffer records that read as this build's crash; a patch that lands and changes nothing because the edited data is not the data the UI consumes; two observations that cannot distinguish the hypotheses reported as a result. - verification.md: point the known code-item blind spot at the new reference. - Unify the four remaining script docstrings (and their inline comments) into English, and generalise a sample-specific class name out of one of them.
This commit is contained in:
inliver233
@@ -0,0 +1,151 @@
|
||||
# Auditing your own patch — proving it landed, and proving it is legal
|
||||
|
||||
Two independent questions, and they fail independently:
|
||||
|
||||
1. **Did the edit land?** Did the bytes you intended actually reach the artifact?
|
||||
2. **Is the result legal?** Will the runtime's verifier accept it?
|
||||
|
||||
A patch can land and be illegal (the class fails to load). It can also fail to land while every
|
||||
build step reports success. Both are silent, and each needs its own check.
|
||||
|
||||
## 1. Did it land? Compare *length*, not bytes
|
||||
|
||||
The naive check is a byte-level diff of the target method against the original.
|
||||
**It produces a flood of false positives.** A disassemble→reassemble round-trip rebuilds the
|
||||
string pool — the assembler **de-duplicates identical strings** — which shifts every later
|
||||
string index by a small delta. Hundreds of methods then look "changed" while their behaviour is
|
||||
identical.
|
||||
|
||||
Observed shape of that noise: **~800 methods reported as differing, 84% of them by only 1–4
|
||||
bytes**, all of them constant-pool index shifts, with the method byte-length unchanged.
|
||||
|
||||
**Use instruction-stream length as the first-order predicate.** A real patch that changes control
|
||||
flow or constant structure changes the method's `code_item.insns_size`. Pure index shifts and
|
||||
debug-info reordering do not.
|
||||
|
||||
### The blind spot, and how to close it
|
||||
|
||||
**An equal-length replacement changes nothing measurable by length.**
|
||||
|
||||
Examples that bite: `move-result vX` → `const/4 vX, 0x0` (both one code unit); swapping one
|
||||
`const` for another of the same width; any edit that keeps the instruction count identical.
|
||||
|
||||
Length-only auditing therefore **silently under-reports**: an audit reported 20 changed methods
|
||||
on a build whose patch set actually touched 24 — the four missing ones were all equal-length
|
||||
replacements.
|
||||
|
||||
**Two-tier rule:**
|
||||
|
||||
1. **Tier 1 (length):** scan every method. This yields candidates with essentially zero false
|
||||
positives.
|
||||
2. **Tier 2 (opcode-level):** compare the two versions opcode by opcode **for every method the
|
||||
patch set declares as a target** — not merely the Tier-1 candidates.
|
||||
|
||||
**Tier 2's input cannot come from Tier 1's output.** Equal-length replacements produce no Tier-1
|
||||
candidate at all, so the only reliable index of "what to check at Tier 2" is the patch set's own
|
||||
declarations.
|
||||
|
||||
⇒ **Corollary, which is really a documentation requirement: every patch must declare its target
|
||||
method signature.** A patch that is both equal-length and undeclared is invisible to both tiers;
|
||||
finding it requires opcode-comparing every class, which is possible but expensive.
|
||||
|
||||
### A sharper Tier-2 shape for equal-length replacements
|
||||
|
||||
Compare the **position set of one opcode** inside the method:
|
||||
|
||||
```
|
||||
target method, `move-result` (0x0a) position sets
|
||||
original : [25, 43, 66, 72, 216, ... , 899] (16 entries)
|
||||
patched : [25, 43, 72, 216, ... , 899] (15 entries) <- exactly one removed, pc=66
|
||||
```
|
||||
|
||||
This proves two things at once: the intended site was hit, **and** the other same-shaped call
|
||||
sites in that method were not. Asking "did anything differ" cannot distinguish those.
|
||||
|
||||
## 2. Is it legal? The verifier is a layer static checks cannot see
|
||||
|
||||
An assembler accepting your smali proves the file is **syntactically** valid dex. It does not run
|
||||
the bytecode verifier. A build can pass assembly, pass every class-table check, and still be
|
||||
rejected at class load:
|
||||
|
||||
```
|
||||
java.lang.VerifyError: Verifier rejected class X: void X.<clinit>() failed to verify:
|
||||
[0x1C8] copyRes1 v0<- result0 type=Undefined
|
||||
```
|
||||
|
||||
**Root cause in the observed case:** padding instructions were inserted **between** a producer and
|
||||
its `move-result`. DEX requires `move-result*` to **immediately follow** the instruction that
|
||||
produces the result (`invoke-*`, `filled-new-array*`). The padding broke adjacency, the result
|
||||
type became `Undefined`, and the whole method — and therefore the class — failed to verify.
|
||||
The length arithmetic ("the shorter form plus two padding units preserves the total") was
|
||||
**correct** and still produced an illegal method.
|
||||
|
||||
**Four layers, cheapest first:**
|
||||
|
||||
| Layer | Check | Catches |
|
||||
|---|---|---|
|
||||
| 1 | assembler exit code | syntax, register overflow, bad labels |
|
||||
| 2 | class-table diff (`scripts/dex_classdiff.py`) | class-set / access-flag drift |
|
||||
| 3 | **predecessor legality of `move-result*`** | result consumers detached from their producer |
|
||||
| 4 | device: first execution of that method | everything above, plus real semantics |
|
||||
|
||||
**Layer 3 is a set-difference, not an absolute count.**
|
||||
|
||||
- For both builds, collect every `move-result*` whose immediately preceding instruction is **not**
|
||||
a producer → the "offenders" set.
|
||||
- Compare the sets. **New offenders must be 0.**
|
||||
- **Never use the absolute size as a signal.** A perfectly healthy build can show hundreds of
|
||||
offenders under this definition — payload pseudo-instructions (`packed-switch-payload`,
|
||||
`fill-array-data-payload`, …) are normally read as ordinary instructions by a naive linear
|
||||
scan — so the absolute number is dominated by scanner artifacts. Only the delta means anything.
|
||||
|
||||
Worked consequence: a build whose sole new offender was one `move-result` following padding
|
||||
crashed at class load on device. After moving the padding to **after** the `move-result`, the
|
||||
delta returned to 0 and the same build ran. **The delta predicted the device result before the
|
||||
device was touched.**
|
||||
|
||||
## 3. When patches are applied by text matching
|
||||
|
||||
Some toolchains apply patches by literal match against a text form (smali) rather than through a
|
||||
dex API. The failure modes are textual and unforgiving:
|
||||
|
||||
- **Whitespace is content.** A fragment indented with 5 spaces will not match a file using 4.
|
||||
There is no fuzzy matching; one misplaced space is a zero-hit.
|
||||
- **Line endings.** If the reader normalises CRLF→LF and the writer does not restore it, the
|
||||
patched file becomes LF-only. A later byte-level comparison against the CRLF source then shows
|
||||
the *entire file* as changed. Diff with an ignore-CR-at-EOL option, or compare structure.
|
||||
- **Instruction names must be complete.** `move-result vX` and `move-result-object vX` are
|
||||
different instructions; an anchor missing the suffix matches the wrong thing or nothing.
|
||||
- **Operand punctuation matters.** `instance-of p3, p0, LType` is not the source text when the
|
||||
source reads `LType;`.
|
||||
- **Uniqueness is a hard requirement.** A matcher that finds several hits must fail loudly rather
|
||||
than patch the first. When a one-line anchor is not unique, extend it with a second unique line;
|
||||
**when the *combination* is still not unique, add a third anchor that discriminates the
|
||||
homographs** — usually the branch target label, since identical call shapes often differ in
|
||||
where they jump.
|
||||
- **A re-emitted old fragment is not a failed write.** A patch may legitimately re-emit the old
|
||||
text inside the new one (inserting a guard *before* the lines it matched). A read-back check
|
||||
asserting "the old text is gone" then reports failure on that patch forever. Assert instead
|
||||
that the **new, unique** part of the replacement is present.
|
||||
- **One patch document, one patch block.** A parser that treats every fenced block as a candidate
|
||||
will apply an *example* diff as a real patch — silently cancelling the real one, or editing
|
||||
unrelated code. Keep examples in prose, never in the same fence format.
|
||||
|
||||
## 4. Audit the patch set, not a patch
|
||||
|
||||
For a multi-patch build:
|
||||
|
||||
```
|
||||
expected = every method declared as a target by the patch set
|
||||
tier1 = methods whose insns_size changed
|
||||
tier2 = methods whose opcode stream differs (run over `expected`)
|
||||
missing = expected - (tier1 U tier2) # must be empty
|
||||
```
|
||||
|
||||
**`missing` is the number that matters.** A build can be green on every build step and still be
|
||||
missing a patch. Report `missing` explicitly; do not report the total as if the total were the
|
||||
goal.
|
||||
|
||||
**And re-verify the artifact, not the intent.** If the tool says "applied" but the build on the
|
||||
device is unchanged, you have a stale build, a second copy, or the wrong file
|
||||
(`pitfalls.md` P24). A patch tool's success message describes the tool, not the artifact.
|
||||
@@ -690,3 +690,107 @@ runtime value (777 bytes) yields a build in which every signing parameter is `-1
|
||||
5. **Check the OAID/device-id path too** — it frequently hashes the same certificate separately.
|
||||
|
||||
Full treatment: `references/signature-derived-keys.md`.
|
||||
|
||||
---
|
||||
|
||||
## P28. `logcat -c` does not clear the events buffer, so the previous build's crash looks like this build's
|
||||
|
||||
**Symptom**
|
||||
|
||||
After installing a fix, a search for the crash signature returns a hit. It looks like the fix did
|
||||
not work — and the "hit" is convincing, because it is the exact exception and the exact frame you
|
||||
were fixing.
|
||||
|
||||
**Root cause**
|
||||
|
||||
`adb logcat -c` clears `main`, `system` and `crash` **by default — not `events`**. The
|
||||
system's own crash record (`am_crash`, `am_proc_died`) lives in `events`. Reading `-b all`
|
||||
therefore pulls in the previous build's crash, which was never cleared.
|
||||
|
||||
**How to tell it apart**
|
||||
|
||||
Correlate **PID and wall-clock time** against the current process:
|
||||
|
||||
- The PID in the record belongs to a process that is no longer running.
|
||||
- The timestamp **predates** the current process's start.
|
||||
|
||||
Both together mean history, not a finding. (In the observed case the stale record's PID was the
|
||||
previous build's, and its timestamp was minutes before the current process started.)
|
||||
|
||||
**Do instead**
|
||||
|
||||
Pick one, and say which you used:
|
||||
|
||||
- Bound the query by timestamp — accept only records after the current process started, or
|
||||
- Judge per buffer — `crash` is a fresh window after `-c`; `events` requires PID/time correlation.
|
||||
|
||||
**The evidence window is itself a claim that needs support.** An unexamined stale window turns
|
||||
"fixed" into "still broken", and this one is invisible in the log's own text.
|
||||
|
||||
---
|
||||
|
||||
## P29. The patch landed and changed nothing, because the static data it edited is not the data the UI consumes
|
||||
|
||||
**Symptom**
|
||||
|
||||
Build is green. The patch is verifiably present in the artifact. The behaviour is unchanged.
|
||||
|
||||
**Root cause**
|
||||
|
||||
**The same type had two construction sites** — one for a static/default template, one inside the
|
||||
runtime conversion of a server response — and only the second one feeds the UI. Editing the first
|
||||
is a no-op that verifies cleanly and audits cleanly.
|
||||
|
||||
**How to see it before shipping**
|
||||
|
||||
From the type's constructor, count the call sites (`scripts/find_refs.py`), then ask of each:
|
||||
|
||||
> **Is this one on the path the UI actually reads?**
|
||||
|
||||
"It is constructed here" is not "it is consumed here". Two constructors of the same type can have
|
||||
completely different fates.
|
||||
|
||||
**Do instead**
|
||||
|
||||
- Patch the **consumer** — the loop that converts response items into the UI model — not the
|
||||
static table.
|
||||
- Prefer a predicate over a **semantic discriminator** (a business code, a task type) rather than
|
||||
a display string. The literal in the template is **not** necessarily the literal the server
|
||||
sends: the string you want to match may not exist anywhere in the response.
|
||||
- If you cannot confirm the server-side value, **predicate on both candidate fields** rather than
|
||||
betting on one.
|
||||
|
||||
---
|
||||
|
||||
## P30. Two observations that cannot distinguish the hypotheses, reported as a result
|
||||
|
||||
**Symptom**
|
||||
|
||||
A verification step "passes" or "fails" while having tested nothing.
|
||||
|
||||
**Root cause**
|
||||
|
||||
The observable is identical under both hypotheses, so the measurement carries no information.
|
||||
Real instance: a promotional banner was absent in the logged-in state, and that run was used to
|
||||
"verify" its removal — but the same banner is also absent in the logged-out state. Neither run
|
||||
could distinguish an effective patch from a no-op, and the second one was skipped as
|
||||
"uninformative" — correctly, since it could not have been informative either.
|
||||
|
||||
**Do instead**
|
||||
|
||||
Before running a verification, ask:
|
||||
|
||||
> **What would this look like if the patch were absent?**
|
||||
|
||||
If the answer is the same, the measurement is void. Record it as **"not applicable under this
|
||||
condition"**, never as "passed".
|
||||
|
||||
Two close variants of the same error:
|
||||
|
||||
- Treating **"the entry point is unreachable"** as proof of removal. The honest test is whether
|
||||
the **request is still issued**: a UI element that no longer renders can still fire its network
|
||||
call from elsewhere. Verify at the layer the behaviour actually lives on.
|
||||
- Treating **"the screenshot hash changed"** as proof that something rendered. A hash proves
|
||||
*change*, never *what* changed — and it never proves *absence*. Look at the images: a status-bar
|
||||
clock tick or a line of text reflowing changes the hash while a full-screen overlay would not
|
||||
have been missed if the frames had actually been inspected.
|
||||
|
||||
@@ -48,6 +48,8 @@ access_flags diff (same interface-ness): 0
|
||||
|
||||
Caveat, and it is an important one: **this check cannot detect code-item damage.** A whole-tree smali round-trip can pass every table check and still crash with `IncompatibleClassChangeError` (`pitfalls.md` P3). Passing this check is necessary, not sufficient.
|
||||
|
||||
The checks that *can* see further — instruction-length auditing, the equal-length-replacement blind spot, and verifier-level legality (`move-result*` adjacency) — are in `references/patch-audit.md`. Use them before claiming a patch landed.
|
||||
|
||||
Also confirm you changed what you intended and nothing more:
|
||||
- Reverse the patched dex and inspect the target method.
|
||||
- Compare unmodified classes byte-for-byte against the original where feasible.
|
||||
|
||||
Reference in New Issue
Block a user