Skip to content

Commit 3646e40

Browse files
widgetiiclaude
andcommitted
Fix #154: hisi_gen2_read_exit_cb segfault that emptied jxf22 traces
Root cause: unsigned char *buf = alloca(nbyte); memset(&buf, 0, sizeof(nbyte)); // <-- bug copy_from_process(proc->pid, remote_addr, buf, nbyte); `memset(&buf, 0, sizeof(nbyte))` zeroed the local POINTER variable (not the buffer it pointed at), so `buf` became NULL. The next `copy_from_process(..., buf, ..)` then SIGSEGV'd the tracer at `buf[i / sizeof(size_t)] = ret`. On HISI_V2 / V2A targets where the sensor driver does an I2C read of the chip-ID register right after `I2C_SLAVE_FORCE` (jxf22, sc2235, ar0130, ...) ipctool died immediately after the first `i2c_read()` line. The streamer kept running untraced and finished its 79 register writes with no observer; the captured log stopped at ~10 lines. That is the empty-trace symptom of #154 - and an inflight hazard for other V2/V2A sensor families that follow the same probe pattern. The memset was redundant - copy_from_process overwrites buf in full - so just drop it. Supporting ptrace.c hardening (matches strace's setup): * PTRACE_O_TRACESYSGOOD + gate syscall stops on (SIGTRAP|0x80). Defends against a stray real SIGTRAP being processed as a syscall enter, which would flip per-PID enter/exit parity and corrupt every subsequent register read. * PTRACE_O_TRACEEXEC + a PTRACE_EVENT_EXEC handler. Without it the kernel signals execve completion with a legacy plain SIGTRAP that the wait loop would inject back, killing the tracee. * Drop the redundant PTRACE_ATTACH on the main tracee - it always returned EPERM because the child PTRACE_TRACEME'd itself first. * Drop the early `ptrace(PTRACE_SYSCALL, new_child, 0, 0)` in the CLONE/FORK/VFORK handler - it raced with the kernel's auto-attach and is unnecessary because the new tracee's SIGSTOP arrives through the wait loop on its own. * NULL-guard `syscall_open` against `copy_from_process_str()` failure (would otherwise NULL-deref in strcmp/IS_PREFIX). New diagnostic env knob: * `IPCTOOL_TRACE_DEBUG=1` makes `syscall_open` and `syscall_write_exit` log to stderr (filename, fd, callback wired). Off by default, zero overhead unless set. Used to triage cases where /proc/<pid>/fd shows a device open but the trace contains no banner/writes for it. New segmenter pattern: * `tools/trace_segment.py` adds `soi_jx` (reg 0x12, init=0x40, stream-on=0x00) for SOI/JX 8-bit-register sensors (JXF22, JXF23, JXH62, ...). Tried after smartsens/sony_imx so existing detection is unchanged. Documentation: * `docs/sensor-driver-extraction.md`: document the segfault-induced empty trace in "When the trace is empty anyway" with a core-dump + gdb recipe; document `IPCTOOL_TRACE_DEBUG=1` in Troubleshooting; add the soi_jx row to the family table. Verification (Definition-of-Done from #154): [x] Hi3518EV200 + Majestic + libsns_jxf22.so capture: 79 writes (target >=50, matches strace -f baseline). [x] tools/trace_segment.py emits init_pattern=soi_jx with 75 init events (was 0). [x] tools/trace_to_driver.py emits jxf22_linear_init that passes `gcc -Wall -Wextra -fsyntax-only`. [x] tools/trace_diff.py vs OpenIPC/glutinium hi35xx_sensor_jxf22 reports 96.1% address / 100% value match (target >=90/>=80). [x] tools/test_pipeline.sh (CI) still passes. [x] SC2315E pattern detection unchanged: synthetic SmartSens trace still hits init_pattern=smartsens (soi_jx is tried after smartsens). Trace_to_driver/trace_diff scripts are unchanged so an unchanged SC2315E trace produces an unchanged diff. End-to-end re-verification on real SC2315E hardware not run (no access from this environment). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 6084896 commit 3646e40

3 files changed

Lines changed: 111 additions & 7 deletions

File tree

docs/sensor-driver-extraction.md

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,7 @@ the segments JSON.
309309
|---|---|---|---|---|
310310
| `smartsens` | `0x0100` | `0x00` (reset) | `0x01` (stream-on) | SC2315E, SC2335, SC*, SmartSens generally |
311311
| `sony_imx` | `0x3000` | `0x01` (standby) | `0x00` (release) | IMX291, IMX385, IMX307, Sony IMX line |
312+
| `soi_jx` | `0x0012` | `0x40` (standby/reset) | `0x00` (stream-on) | JXF22, JXF23, JXH62, SOI / JX 8-bit register family |
312313

313314
Adding a family is one entry in `INIT_PATTERNS` at the top of
314315
`trace_segment.py`. If your trace is recognised but no init phase is
@@ -350,6 +351,28 @@ write on a fd opened by the parent silently drops to no callback.
350351
A V1/V2 capture that shows `0` `sensor_write_register` lines despite
351352
the streamer reporting init success usually means one of:
352353

354+
* **`ipctool` segfaulted mid-trace.** Symptom signature: trace ends
355+
at exactly `i2c_read()` (or shortly after the `i2c-N` banner), no
356+
further events, `wait $!` reports exit status 139 (SIGSEGV). The
357+
streamer keeps running untraced, so its real register-write burst
358+
goes unobserved and the captured log stays at ~10 lines. To confirm,
359+
capture a core dump and inspect the backtrace:
360+
361+
```bash
362+
ssh root@<camera> "ulimit -c unlimited; cd /tmp; \
363+
/tmp/ipctool trace --output=/tmp/dumps/trace.log /usr/bin/majestic"
364+
scp root@<camera>:/tmp/core /tmp/core
365+
arm-linux-gdb --batch -ex 'core /tmp/core' -ex 'bt full' \
366+
build-arm-ci/ipctool
367+
```
368+
369+
If the backtrace lands inside a decoder callback, the bug is in
370+
`src/ptrace.c`'s callback (most likely a NULL-deref on the
371+
`copy_from_process` buffer or filename argument). The historical
372+
example was `hisi_gen2_read_exit_cb` doing `memset(&buf, 0, …)`
373+
instead of `memset(buf, 0, …)`, nullifying the pointer before
374+
`copy_from_process` was called - this killed Hi3518EV200 +
375+
libsns_jxf22.so traces immediately after the first sensor-ID read.
353376
* **Sensor `.so` opens its own I2C handle in a path our trace
354377
doesn't see.** Check `/proc/<streamer-pid>/fd` while it's running:
355378
if the live `/dev/i2c-N` fd in the running process is different
@@ -695,6 +718,21 @@ from. Some vendors reorder writes between releases without changing
695718
behaviour. The address+value match is what matters; sequence is a
696719
helpful proxy that breaks down across versions.
697720

721+
### Narrowing "fd N is open live but not in trace" with `IPCTOOL_TRACE_DEBUG=1`
722+
723+
Setting this in the tracee's environment makes `ipctool trace` log
724+
every `open()` and `write()` syscall to stderr (filename, fd, callback
725+
state). Off by default, zero overhead unless set. Useful when
726+
`/proc/<streamer-pid>/fd` shows `/dev/i2c-N` open but the trace
727+
contains no banner/writes for it - the dbg log will say whether
728+
`syscall_open` ever ran for that fd and what filename it resolved.
729+
730+
```bash
731+
IPCTOOL_TRACE_DEBUG=1 ipctool trace --output=/tmp/trace.log \
732+
/usr/bin/majestic 2>/tmp/trace.dbg
733+
grep -E "i2c|fd=26" /tmp/trace.dbg
734+
```
735+
698736
## File layout
699737

700738
```

src/ptrace.c

Lines changed: 70 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ typedef struct process {
100100
} process_t;
101101

102102
HashTable pids;
103+
static bool trace_debug = false;
103104

104105
static void dump_regs(struct user const *regs, FILE *outfp) {
105106
fprintf(outfp, "r0 = 0x%08lx, r1 = 0x%08lx\n", regs->regs.uregs[0],
@@ -503,8 +504,16 @@ static void hisi_vi_ioctl_exit_cb(process_t *proc, int fd, unsigned int cmd,
503504

504505
static void hisi_gen2_read_exit_cb(process_t *proc, int fd, size_t remote_addr,
505506
size_t nbyte, ssize_t sysret) {
507+
// The previous version did `memset(&buf, 0, sizeof(nbyte))` here,
508+
// which zeroed the local POINTER (not the buffer it pointed at),
509+
// turning buf into NULL. The next copy_from_process(..., buf, ..)
510+
// then segfaulted the tracer. Hi3518EV200 + libsns_jxf22.so hit
511+
// this immediately after the first I2C_SLAVE_FORCE: ipctool
512+
// crashed silently and the trace stopped at 10 lines, which is
513+
// exactly the empty-trace symptom of issue #154. The memset was
514+
// redundant in any case - copy_from_process overwrites buf in
515+
// full - so just drop it.
506516
unsigned char *buf = alloca(nbyte);
507-
memset(&buf, 0, sizeof(nbyte));
508517
copy_from_process(proc->pid, remote_addr, buf, nbyte);
509518
// reg_width
510519
if (nbyte == 2) {
@@ -916,10 +925,20 @@ static void syscall_open(process_t *proc, int fd, int offset) {
916925
#endif
917926
size_t remote_addr = proc->regs.regs.uregs[0 + offset];
918927
char *filename = copy_from_process_str(proc, remote_addr);
928+
if (trace_debug)
929+
fprintf(stderr, "open_dbg pid=%d fd=%d offset=%d file='%s'\n",
930+
proc->pid, fd, offset, filename ? filename : "(null)");
919931
#if 0
920932
printf("open('%s')\n", filename);
921933
#endif
922934

935+
// copy_from_process_str returns NULL when PEEKTEXT fails (tracee
936+
// unmapped the page mid-syscall, or the open() argument is junk
937+
// and the kernel is about to return EFAULT). Without this guard,
938+
// the strcmp() / IS_PREFIX() calls below dereference NULL.
939+
if (!filename)
940+
goto done;
941+
923942
proc->fds[fd].file = new_arc_str(filename);
924943
proc->fds[fd].ioctl_exit = null_ioctl_exit_cb; // dump_ioctl_exit_cb;
925944
proc->fds[fd].read_exit = default_read_exit_cb;
@@ -1010,6 +1029,15 @@ static void syscall_write_exit(process_t *proc, ssize_t sysret) {
10101029
size_t remote_addr = proc->regs.regs.uregs[1];
10111030
size_t nbyte = proc->regs.regs.uregs[2];
10121031

1032+
if (trace_debug)
1033+
fprintf(stderr,
1034+
"write_dbg pid=%d fd=%d nbyte=%zu sysret=%zd file='%s' "
1035+
"have_cb=%d\n",
1036+
proc->pid, fd, nbyte, sysret,
1037+
proc->fds[fd].file ? arc_cstr(proc->fds[fd].file) : "(none)",
1038+
proc->fds[fd].write_exit != NULL &&
1039+
proc->fds[fd].write_exit != default_write_exit_cb);
1040+
10131041
if (proc->fds[fd].write_exit)
10141042
proc->fds[fd].write_exit(proc, fd, remote_addr, nbyte, sysret);
10151043
}
@@ -1173,10 +1201,12 @@ static void do_trace(pid_t tracee) {
11731201

11741202
ht_setup(&pids, sizeof(pid_t), sizeof(process_t), 10);
11751203
pid_t tracer = getpid();
1204+
trace_debug = getenv("IPCTOOL_TRACE_DEBUG") != NULL;
11761205

11771206
printf("\n[%d] child %d created\n", tracer, tracee);
1178-
ptrace(PTRACE_ATTACH, tracee, NULL,
1179-
NULL); // child is the main thread
1207+
// The child has already PTRACE_TRACEME'd itself in do_child(); a
1208+
// PTRACE_ATTACH on top would only return EPERM. Just wait for its
1209+
// post-execv() SIGTRAP.
11801210
process_t *mthread = &(process_t){.pid = tracee};
11811211
mthread->fds[0].file = new_arc_str(strdup("stdin"));
11821212
mthread->fds[1].file = new_arc_str(strdup("stdout"));
@@ -1189,8 +1219,21 @@ static void do_trace(pid_t tracee) {
11891219
// streamers). TRACEFORK/TRACEVFORK catch genuine forked children;
11901220
// not strictly necessary for any tested target so far but cheap
11911221
// defensive coverage in case a streamer spawns a worker via fork().
1192-
long ptraceOption =
1193-
PTRACE_O_TRACECLONE | PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK;
1222+
// TRACESYSGOOD sets bit 7 of the syscall-stop signal so we can tell
1223+
// syscall stops apart from a real SIGTRAP delivered to the tracee.
1224+
// Without it, the wait loop conflates the two: a real SIGTRAP would
1225+
// be processed as a syscall enter, flipping the per-PID enter/exit
1226+
// parity, and subsequent write decodes would read garbage from
1227+
// proc->regs. Defensive against execve / dlopen / debug-trap paths.
1228+
// TRACEEXEC: without it, the kernel signals execve completion with
1229+
// a legacy plain SIGTRAP delivered to the tracee. Our wait loop
1230+
// would then fall to the "real signal delivery" else-branch and
1231+
// inject the SIGTRAP back, killing the tracee. With TRACEEXEC the
1232+
// kernel emits a PTRACE_EVENT_EXEC stop instead, which we recognise
1233+
// below and skip.
1234+
long ptraceOption = PTRACE_O_TRACECLONE | PTRACE_O_TRACEFORK |
1235+
PTRACE_O_TRACEVFORK | PTRACE_O_TRACESYSGOOD |
1236+
PTRACE_O_TRACEEXEC;
11941237
ptrace(PTRACE_SETOPTIONS, tracee, NULL, ptraceOption);
11951238
ptrace(PTRACE_SYSCALL, tracee, 0, 0);
11961239

@@ -1212,6 +1255,14 @@ static void do_trace(pid_t tracee) {
12121255
// CLONE/FORK/VFORK all create a new tracee that needs the same
12131256
// bookkeeping: pull its pid from the kernel, look up the parent's
12141257
// process_t, copy its fd state, register the new tracee.
1258+
if (event == PTRACE_EVENT_EXEC) {
1259+
// execve completed in this tracee; just resume. The
1260+
// syscall-exit for execve is delivered separately as a
1261+
// TRACESYSGOOD-tagged stop, so no per-process_t fixup
1262+
// is required here.
1263+
ptrace(PTRACE_SYSCALL, child_waited, 0, 0);
1264+
continue;
1265+
}
12151266
if (event == PTRACE_EVENT_CLONE || event == PTRACE_EVENT_FORK ||
12161267
event == PTRACE_EVENT_VFORK) {
12171268
pid_t new_child;
@@ -1231,7 +1282,15 @@ static void do_trace(pid_t tracee) {
12311282
fprintf(stderr, "Cannot find parent %d\n", ppid);
12321283
}
12331284
}
1234-
ptrace(PTRACE_SYSCALL, new_child, 0, 0);
1285+
// Do NOT PTRACE_SYSCALL new_child here. With
1286+
// PTRACE_O_TRACECLONE the kernel auto-attaches the new
1287+
// tracee and group-stops it with SIGSTOP; it expects
1288+
// the tracer to first observe that stop via waitpid
1289+
// before further ptrace ops are well-defined.
1290+
// new_child's SIGSTOP arrives through the wait loop
1291+
// on its own and the suppression branch below plus
1292+
// the bottom-of-loop PTRACE_SYSCALL resume it
1293+
// cleanly. (Matches strace's clone-handling pattern.)
12351294

12361295
printf("\nparent %d created child %d\n", ppid, new_child);
12371296
}
@@ -1258,7 +1317,11 @@ static void do_trace(pid_t tracee) {
12581317
continue; // don't try to restart a dead pid
12591318
} else if (WIFSTOPPED(status)) {
12601319
int stopCode = WSTOPSIG(status);
1261-
if (stopCode == SIGTRAP) {
1320+
// PTRACE_O_TRACESYSGOOD makes syscall stops carry SIGTRAP|0x80,
1321+
// distinguishing them from real SIGTRAPs delivered to the tracee
1322+
// (which still come through as plain SIGTRAP and fall to the
1323+
// forward-signal else-branch below).
1324+
if (stopCode == (SIGTRAP | 0x80)) {
12621325
process_t *proc = ht_lookup(&pids, &child_waited);
12631326
if (proc != NULL) {
12641327
if (!proc->syscall_num) {

tools/trace_segment.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,9 @@ def collapse_struct(events):
132132
("smartsens", 0x100, 0, 1),
133133
# Sony IMX (IMX291, IMX385, IMX307, ...) - standby register at 0x3000
134134
("sony_imx", 0x3000, 1, 0),
135+
# SOI / JX (JXF22, JXF23, JXH62, ...) - 8-bit reg+data, standby reg at
136+
# 0x12 with bit 0x40 = standby/reset, 0x00 = stream on.
137+
("soi_jx", 0x12, 0x40, 0x00),
135138
]
136139

137140

0 commit comments

Comments
 (0)