diff --git a/AGENTS.md b/AGENTS.md index 24eb97e..d23fecd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -304,6 +304,43 @@ Known limitation: no on-disk fixture reproduces the exact live failure (all real captures contain a 372 ATR), so end-to-end proof needs a re-captured `.sr` from the failing phone; the unit tests cover the loop-escape logic. +### v1.7.0: spurious end-of-decode hardening + start.sh session resilience + +A live capture could end without Ctrl+C (the session simply stopped). The +frontend symptom was a flood of `sr: session: sr_session_stop: session was +NULL`, but the decoder also had latent paths that could end `decode()` early +on an idle/busy live line. Fixed: + +1. **`_at_eof()`.** `decode_step()` treated `samplenum == _last_sn` as EOF, + but a step can legitimately consume only queued bytes (`_replay` / + `peeked_byte`) without advancing the stream -- e.g. the v1.5.0 invalid-INS + re-frame. Such steps are now allowed up to 64 in a row; only a genuinely + stuck stream with nothing queued ends the decode. +2. **`_stall_check(track=False)`.** The idle-LEVEL wait in + `wait_data_falling()` returns immediately at the current sample whenever + DATA is already idle -- normal on a live idle line, not EOF. Stall + counting is now disabled for that wait (only `pins is None` means EOF); + sample-stall detection is kept for the start-bit EDGE wait. +3. **`resync_idle()` null guard.** It indexed `pins[self.DATA_IDX]` after + `wait()` with no `None` check; at stream end that raised `TypeError`. Now + sets `_eof` and returns. + +**Effect:** `samsung_phone_sample.sr` had been halting at sample ~21.9M of +320M (1 APDU). It now decodes through sample ~293M to a true end: **939 +APDUs**, `CHKSUM=176`. The extra 6 `CONCAT` packets are real mis-framed STK +bursts that were simply never reached before -- the known Samsung +de-concatenation family; the rest of the suite is unchanged. + +**start.sh:** auto-restart is now the **default** (`--no-loop` disables), so +an intermittent FX2 USB stop reconnects instead of ending the session; +per-session pcap suffix (`.N.pcap`) so a restart never overwrites the +previous capture; and the repetitive `sr_session_stop: session was NULL` +teardown line is filtered from stderr (real decoder errors still pass). + +Validation: test_8 56/0/OK, test_7 65/0/OK, xiaomi_phone 640, xiaomi_cold +473, 4gmodem 327, A55 407/395, S21p 399/375, target S21p 369 -- all +`RESULT: OK`, 0 CONCAT. 65/65 unit tests (10 new). VERSION -> 1.7.0. + ### v1.5.0: invalid-INS one-byte re-frame (residual CONCATs → 0) The last CONCAT family (4gmodem/A55_sim2/S21p_sim2) is now **reframed at @@ -570,7 +607,7 @@ official gsmtap.h reserves — a simtrace2-sniff convention. ### Versioning -The decoder version is defined in `pd.py` as `VERSION = '1.5.0'`. +The decoder version is defined in `pd.py` as `VERSION = '1.7.0'`. The version is printed to the log on decoder startup. ### Testing after decoder changes diff --git a/README.md b/README.md index 718fcc1..501d8a2 100644 --- a/README.md +++ b/README.md @@ -192,11 +192,16 @@ is the usual suspect. `start.sh` captures a continuous stream at the configured rate (default 16 MHz) until Ctrl+C (`rc=130`). A healthy capture never ends by itself; if the FX2 drops the USB transfer mid-session (a flaky device, `rc=0` "device -stopped streaming"), pass `--loop` to auto-restart — it re-launches the -capture on every `rc=0` until you press Ctrl+C or hit `--max-restarts=N` -(default 20; `0` = unlimited), with a `--restart-delay=S` gap (default 2 s). +stopped streaming"), the capture auto-restarts so the session is not lost — +it re-launches on every `rc=0` until you press Ctrl+C or hit +`--max-restarts=N` (default 20; `0` = unlimited), with a `--restart-delay=S` +gap (default 2 s). When restarting with `--pcap=FILE`, each session's capture +is written to `FILE..` so a restart never overwrites the previous one, +and the repetitive `sr_session_stop: session was NULL` teardown line is +filtered from stderr. -- `--loop` auto-restarts the capture when it ends on its own (`rc=0`). +- `--no-loop` disables auto-restart (single capture only). +- `--loop` forces auto-restart on (this is the default). - `--max-restarts=N` caps restarts (default 20; `0` = unlimited). - `--restart-delay=S` seconds to wait between restarts (default 2). - `--log` captures the `rc=` verdict line for the session end. diff --git a/pd.py b/pd.py index 273b9b1..bb033aa 100644 --- a/pd.py +++ b/pd.py @@ -28,7 +28,7 @@ from .gsmtap_stream import (GsmtapStreamSender, GSMTAP_SIM_RST_EVENT, GSMTAP_SIM_VCC_EVENT, GSMTAP_FLAG_BAD_FCS) -VERSION = '1.6.0' +VERSION = '1.7.0' @@ -452,6 +452,11 @@ class Decoder(srd.Decoder): self._start_fall = None self._edge_read = True self._last_sn = -1 + # Bounded allowance for steps that consume only queued bytes + # (_replay / peeked_byte) and therefore do not advance the sample + # counter. Without it the "stuck sample number == EOF" test below + # would mistake a re-frame step for end-of-capture. + self._no_advance = 0 self._eof = False self._min_pulse_samples = 2 # tuned for 16 MHz sample rate self._glitch_rejects = 0 @@ -1389,7 +1394,7 @@ class Decoder(srd.Decoder): cond.append({self.VCC_IDX: 'e'}) while True: pins = self.wait(cond) - if self._stall_check(pins): + if self._stall_check(pins, track=False): self._eof = True return if (self.line_events(pins)): @@ -1427,12 +1432,22 @@ class Decoder(srd.Decoder): self._start_fall = self.samplenum return - def _stall_check(self, pins): + def _stall_check(self, pins, track=True): '''Detect end-of-capture: libsigrokdecode stops advancing the sample counter once the feed is exhausted, so a wait that returns the same - sample number repeatedly means there is no more data.''' + sample number repeatedly means there is no more data. + + `track=False` is used for idle-LEVEL waits: a level condition that is + already satisfied returns immediately at the current sample without + advancing, which is normal on an idle line and NOT end-of-capture. + Only a ``None`` pin set (true end-of-stream) is treated as EOF there; + the stall window is reset so a following edge wait starts fresh.''' if pins is None: return True + if not track: + self._prev_wait_sn = None + self._stall = 0 + return False if self._prev_wait_sn is not None and self.samplenum == self._prev_wait_sn: self._stall += 1 else: @@ -1556,6 +1571,11 @@ class Decoder(srd.Decoder): idle_level = 0 if self._inverse_convention else 1 for _ in range(loops): pins = self.wait({'skip': 0}) + if pins is None: + # End of capture / session stopped mid-resync: stop cleanly + # instead of raising TypeError on the None pin state. + self._eof = True + return if pins[self.DATA_IDX] == idle_level: return # Could not find an idle gap -- resume normal decode anyway. @@ -2002,12 +2022,29 @@ class Decoder(srd.Decoder): self.emit_packet(GSMTAP_SIM_PPS, bytes(pps_req + pps_rsp), ss, self.samplenum) + def _at_eof(self): + '''True when the sample stream is exhausted. + + libsigrokdecode stops advancing the sample counter once the feed is + gone, so a stuck sample number normally means end-of-capture. But a + step may legitimately consume only queued bytes (_replay / + peeked_byte) without touching the stream (e.g. the invalid-INS + one-byte re-frame); allow a bounded number of those so a re-frame is + never mistaken for EOF.''' + if self._eof: + return True + if self.samplenum != self._last_sn: + self._no_advance = 0 + return False + if ((self._replay or self.peeked_byte is not None) + and self._no_advance < 64): + self._no_advance += 1 + return False + return True + def decode_step(self): '''Run one state-machine step. Returns False when decoding ends.''' - # End-of-capture detection: libsigrokdecode stops advancing the sample - # counter once the feed is exhausted, so a stuck sample number means - # we have consumed everything (and any pending wait is non-blocking). - if self.samplenum == self._last_sn or self._eof: + if self._at_eof(): return False self._last_sn = self.samplenum # Prime the initial RST/VCC levels from the actual first sample (no diff --git a/start.sh b/start.sh index 441aab3..4a3cd16 100755 --- a/start.sh +++ b/start.sh @@ -18,7 +18,9 @@ DEBUG="" ANNOT="iso7816=apdus,iso7816=warnings" # default: decoded APDUs + warnings only LOG="" KLOG="" -LOOP=0 # --loop: restart on rc=0 (device stopped streaming) +LOOP=1 # Restart on rc=0 (device stopped streaming) by default + # so an intermittent FX2 USB drop does not end the + # session. --no-loop disables; --max-restarts caps it. MAX_RESTARTS=20 # --max-restarts=N (0 = unlimited) RESTART_DELAY=2 # --restart-delay=S seconds between restarts PROTOCOL="T=0" # --protocol: T=0 (default; live phone captures start @@ -69,11 +71,12 @@ Options: T=1, or auto. Live phone captures start mid-session, so a fixed protocol lets the decoder emit raw bursts instead of hunting for a (non-existent) ATR. - --loop Auto-restart when the capture ends on its own (rc=0, device - stopped streaming). Loops until you press Ctrl+C (rc=130) - or a real error. Useful for a flaky FX2 that drops the USB - transfer mid-session. - --max-restarts=N Cap --loop restarts (default: 20; 0 = unlimited). + --no-loop Disable auto-restart. By default the capture restarts + when it ends on its own (rc=0, device stopped streaming, + e.g. an intermittent FX2 USB drop). Restarts stop on + Ctrl+C or a real error. + --loop Force auto-restart on (this is already the default). + --max-restarts=N Cap auto-restarts (default: 20; 0 = unlimited). --restart-delay=S Seconds to wait between restarts (default: 2). -h, --help Show this help @@ -122,6 +125,7 @@ for arg in "$@"; do --starts-with-atr=*) echo "WARNING: --starts-with-atr is obsolete (removed in v1.3.0); the decoder always runs mid-session" >&2 ;; --loop) LOOP=1 ;; + --no-loop) LOOP=0 ;; --max-restarts=*) MAX_RESTARTS="${arg#*=}" ;; --restart-delay=*) RESTART_DELAY="${arg#*=}" ;; -h|--help) usage ;; @@ -237,10 +241,21 @@ FIFO="" FIFO_PID="" run_once() { - # $1 = samplerate, $2 = pcap file for this session (may be empty) + # $1 = samplerate, $2 = pcap file (may be empty), $3 = session number + # (may be empty; only set when auto-restarting). local rate="$1" + local pcap_file="$2" + local session="${3:-}" + # Suffix the pcap per session when restarting so a restart never + # overwrites the previous session's capture. + if [ -n "${pcap_file}" ] && [ -n "${session}" ]; then + case "${pcap_file}" in + *.*) pcap_file="${pcap_file%.*}.${session}.${pcap_file##*.}" ;; + *) pcap_file="${pcap_file}.${session}" ;; + esac + fi local pcap_opt="" - [ -n "$2" ] && pcap_opt=":pcap_file=$2" + [ -n "${pcap_file}" ] && pcap_opt=":pcap_file=${pcap_file}" # Blocking, keypress-free stdin: sigrok-cli's "press any key to stop" # waits on stdin. /dev/null EOFs instantly and stops the capture (the # bug introduced when detaching stdin), and a real keypress would too. @@ -251,13 +266,18 @@ run_once() { mkfifo "${FIFO}" sleep 1000000 > "${FIFO}" & FIFO_PID=$! + # Filter the libsigrok teardown noise (sigrok-cli re-calls + # sr_session_stop on an already-stopped session after a device stop). + # Only that exact line is dropped; real decoder errors on stderr pass. sigrok-cli -d fx2lafw \ --config samplerate="${rate}" \ --continuous \ -C 'D0,D1,D2,D3,D4,D5,D6,D7' \ ${DEBUG:+-l 4} \ -P "iso7816:${OPTS}${pcap_opt}" \ - -A "${ANNOT}" < "${FIFO}" | tee "${ANNOT_TMP}" + -A "${ANNOT}" \ + 2> >(grep -vF 'sr_session_stop: session was NULL' >&2) < "${FIFO}" \ + | tee "${ANNOT_TMP}" local rc=${PIPESTATUS[0]} kill "${FIFO_PID}" 2>/dev/null || true rm -f "${FIFO}" 2>/dev/null || true @@ -301,7 +321,7 @@ if [ "${LOOP}" -eq 1 ]; then tries=0 while true; do tries=$((tries + 1)) - run_once "${SAMPLERATE}" "${PCAP}" + run_once "${SAMPLERATE}" "${PCAP}" "${tries}" rc=$? report_exit "${tries}" "$rc" # A non-zero rc means a genuine stop: Ctrl+C (130) or an error. diff --git a/tests/test_gsmtap.py b/tests/test_gsmtap.py index d581d5a..24451a7 100644 --- a/tests/test_gsmtap.py +++ b/tests/test_gsmtap.py @@ -6,6 +6,7 @@ the real pd.py / gsmtap_stream.py sources under the `iso7816` package name (mirroring how libsigrokdecode loads a decoder directory). """ +import collections import importlib.util import os import sys @@ -447,5 +448,87 @@ class TestEmbeddedExchanges(unittest.TestCase): self.assertFalse(self.pd.is_desynced(bytes.fromhex(a55u1))) +class TestStallCheck(unittest.TestCase): + """_stall_check distinguishes true EOF (None pins) from a live line that + is merely idle (a level wait returns immediately at the same sample).""" + + def setUp(self): + _, pd = _load_iso7816() + self.inst = object.__new__(pd.Decoder) + self.inst.samplenum = 100 + self.inst._prev_wait_sn = None + self.inst._stall = 0 + + def test_none_pins_is_eof(self): + self.assertTrue(self.inst._stall_check(None)) + + def test_advancing_sample_is_not_stall(self): + for sn in range(100, 110): + self.inst.samplenum = sn + self.assertFalse(self.inst._stall_check([1])) + + def test_repeated_same_sample_is_bounded_stall(self): + results = [] + for _ in range(10): + self.inst.samplenum = 100 + results.append(self.inst._stall_check([1])) + self.assertFalse(results[0]) + self.assertIn(True, results) + + def test_untracked_level_wait_never_stalls(self): + # An idle-level wait repeats at the same sample on a live idle line; + # with track=False it must not be flagged as EOF. + for _ in range(10): + self.inst.samplenum = 100 + self.assertFalse(self.inst._stall_check([1], track=False)) + self.assertEqual(self.inst._stall, 0) + self.assertIsNone(self.inst._prev_wait_sn) + + def test_untracked_resets_stall_window(self): + for _ in range(6): + self.inst.samplenum = 100 + self.inst._stall_check([1]) # would trip the tracked stall + self.assertTrue(self.inst._stall > 4) + self.inst._stall_check([1], track=False) + self.assertFalse(self.inst._stall_check([1])) # window reset + + +class TestAtEof(unittest.TestCase): + """decode_step must not treat a replay-only step (no sample progress) as + end-of-capture, but must still terminate a genuinely stuck stream.""" + + def setUp(self): + _, pd = _load_iso7816() + self.inst = object.__new__(pd.Decoder) + self.inst.samplenum = 500 + self.inst._last_sn = 500 + self.inst._no_advance = 0 + self.inst._eof = False + self.inst._replay = collections.deque() + self.inst.peeked_byte = None + + def test_advancing_is_not_eof(self): + self.inst.samplenum = 501 + self.assertFalse(self.inst._at_eof()) + + def test_stuck_with_nothing_queued_is_eof(self): + self.assertTrue(self.inst._at_eof()) + + def test_pending_replay_is_not_eof(self): + self.inst._replay.append(0x00) + self.assertFalse(self.inst._at_eof()) + + def test_pending_replay_is_bounded(self): + self.inst._replay.append(0x00) + for _ in range(64): + self.assertFalse(self.inst._at_eof()) + self.assertTrue(self.inst._at_eof()) + + def test_explicit_eof_wins(self): + self.inst._eof = True + self.inst.samplenum = 600 + self.assertTrue(self.inst._at_eof()) + + if __name__ == '__main__': unittest.main()