Skip to content

Commit d3f5425

Browse files
committed
Fix late injection bugs and drop dead code
handle_trace's standalone branch declared `auto pid = fork()`, shadowing the tracee PID it had just parsed, and reported a failed fork with `return false` -- which is 0, i.e. EXIT_SUCCESS -- from a function returning int. The child also left its failure paths on `return`, so instead of exiting it unwound back into main; had monitor.run() ever returned, it would have fallen through and attached to zygote a second time. Name the fork result, return EXIT_FAILURE, and terminate the child with _exit() on every path. The child additionally inherited handle_interrupt with g_traced_pid already armed, so a stray SIGTERM made it detach a process it never attached to; clear it after the fork. Scripts: quote the pidof substitutions -- an empty result silently shifted argv and fed `trace --standalone` a missing pid -- pick the tracer binary once instead of hardcoding zygisk-ptrace64 in service.sh, and bail with a log line when no zygote or no tracer is found. The 32-bit branch looked for "zygote32", but app_process only ever names itself "zygote64" or "zygote". Dead code: drop the commented-out copy_to_temp call site along with the use_temp plumbing that could never fire, and the commented-out libart lookup in JniAttachment that RTLD_DEFAULT already covers. Keep copy_to_temp itself, marked maybe_unused, documented as a debugging aid. Restore the DeleteLocalRef on the gids array. Also fix the --spwan usage typo, `sucess`, "No moniter handler actived", a missing newline, and replace the local PROP_VALUE_MAX define with the bionic header that defines it.
1 parent f2c658d commit d3f5425

8 files changed

Lines changed: 85 additions & 78 deletions

File tree

loader/src/include/trace.hpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,3 @@ enum TraceMode {
66
STANDALONE = 2,
77
SYSTEM_SERVER = 3,
88
};
9-

loader/src/injector/hook.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include <sys/mman.h>
66
#include <sys/mount.h>
77
#include <sys/resource.h>
8+
#include <sys/system_properties.h>
89
#include <unistd.h>
910
#include <unwind.h>
1011

@@ -15,8 +16,6 @@
1516
#include "module.hpp"
1617
#include "zygisk.hpp"
1718

18-
#define PROP_VALUE_MAX 92
19-
2019
using namespace std;
2120

2221
// *********************

loader/src/injector/system_server.cpp

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -21,27 +21,10 @@ namespace {
2121
class JniAttachment {
2222
public:
2323
JniAttachment() {
24-
// auto cached_map_infos = lsplt::MapInfo::Scan();
25-
// void* libart;
26-
// for (auto& map : cached_map_infos) {
27-
// if (map.path.ends_with("/libart.so")) {
28-
// LOGV("found path %s", map.path.data());
29-
// libart = dlopen(map.path.data(), RTLD_NOLOAD | RTLD_NOW);
30-
// if (!libart) {
31-
// libart = dlopen("libart.so", RTLD_NOW);
32-
// }
33-
// break;
34-
// }
35-
// }
36-
37-
// if (!libart) {
38-
// LOGE("failed to get libart.so handle");
39-
// return;
40-
// }
41-
4224
using JNI_GetCreatedJavaVMs_t = jint (*)(JavaVM**, jsize, jsize*);
4325

44-
// Pass RTLD_DEFAULT instead of a specific library handle
26+
// RTLD_DEFAULT searches the global scope, which already contains libart in a
27+
// running system_server, so there is no need to locate and dlopen it ourselves.
4528
auto get_vms =
4629
reinterpret_cast<JNI_GetCreatedJavaVMs_t>(dlsym(RTLD_DEFAULT, "JNI_GetCreatedJavaVMs"));
4730

@@ -167,7 +150,7 @@ void trigger_system_server_hooks() {
167150
ctx.server_specialize_post();
168151

169152
// 5. Clean up the local JNI reference to prevent memory leaks in the ART
170-
// if (gids) {
171-
// env->DeleteLocalRef(gids);
172-
// }
153+
if (gids) {
154+
env->DeleteLocalRef(gids);
155+
}
173156
}

loader/src/ptracer/main.cpp

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,8 @@ static void print_usage(const char *tool_name) {
151151
fprintf(stderr, "NeoZygisk Tracer %s\n", ZKSU_VERSION);
152152
fprintf(
153153
stderr,
154-
"usage: %s monitor | trace <pid> [--spwan | --standalone | --system_server] | ctl <start|stop|exit> | version\n",
154+
"usage: %s monitor | trace <pid> [--spawn | --standalone | --system_server] | ctl "
155+
"<start|stop|exit> | version\n",
155156
tool_name);
156157
}
157158

@@ -223,28 +224,47 @@ static int handle_trace(int argc, char **argv) {
223224
} else if (mode == TraceMode::STANDALONE) {
224225
printf("standalone mode: preparing injection and starting daemon...\n");
225226

226-
auto pid = fork();
227-
if (pid < 0) {
228-
PLOGE("init_monitor");
229-
return false;
230-
} else if (pid == 0) {
231-
// Change directory to $MODDIR BEFORE preparing the injection
227+
// Fork off the daemon supervisor. The child never returns from this branch: it
228+
// owns the control socket and the zygiskd process for the rest of the session,
229+
// while the parent falls through to inject into the already-running zygote.
230+
auto supervisor_pid = fork();
231+
if (supervisor_pid < 0) {
232+
PLOGE("fork daemon supervisor");
233+
return EXIT_FAILURE;
234+
}
235+
if (supervisor_pid == 0) {
236+
// The parent armed handle_interrupt with the tracee's PID before forking.
237+
// We are not the tracer, so disown it to keep the child from detaching a
238+
// process it never attached to.
239+
g_traced_pid = 0;
240+
241+
// prepare_environment() reads ./module.prop and ZygoteAbiManager execs
242+
// ./bin/zygiskd*, so both need $MODDIR as the working directory.
232243
cd_to_moddir();
233244
AppMonitor monitor;
234245
if (!monitor.prepare_environment()) {
235-
exit(1);
246+
fprintf(stderr, "error: failed to prepare the monitor environment\n");
247+
_exit(EXIT_FAILURE);
236248
}
237249
if (monitor.get_abi_manager().check_and_prepare_injection() == nullptr) {
238250
fprintf(stderr, "error: failed to start daemon and prepare injector\n");
239-
return EXIT_FAILURE;
251+
_exit(EXIT_FAILURE);
240252
}
253+
// Socket loop only: standalone mode does no ptrace supervision.
241254
monitor.run(true, false);
255+
_exit(EXIT_SUCCESS);
242256
}
243257
} else if (mode == TraceMode::SYSTEM_SERVER) {
244258
target = "system_server";
245-
printf("injecting into system_server");
259+
printf("injecting into system_server\n");
246260
}
247261

262+
// Note on standalone ordering: nothing synchronises the supervisor fork above with
263+
// the injection below, so the tracee may reach zygiskd::PingHeartbeat() before the
264+
// daemon has bound the control socket. This is covered by the retry budget in
265+
// zygiskd::Connect() -- PingHeartbeat uses Connect(5), i.e. four one-second retries
266+
// -- which comfortably outlasts a fork+exec of zygiskd. The retries run inside the
267+
// ptrace-stopped tracee, so the only cost of losing the race is a short stall.
248268
if (!trace_target(pid, mode)) {
249269
if (mode == TraceMode::SPAWN) {
250270
fprintf(stderr, "error: failed to trace zygote, killing process %d\n", pid);

loader/src/ptracer/monitor_impl.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ bool AppMonitor::prepare_environment() {
134134

135135
void AppMonitor::run(bool socket_loop, bool ptrace_loop) {
136136
if (!socket_loop && !ptrace_loop) {
137-
LOGD("No moniter handler actived, exiting...");
137+
LOGD("no monitor handler active, exiting...");
138138
return;
139139
}
140140
event_loop_.Init();

loader/src/ptracer/ptracer.cpp

Lines changed: 13 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ bool inject_after_start(int pid, const char *lib_path, TraceMode mode) {
363363
#endif
364364

365365
// Execute the shared remote injection logic
366-
bool sucess = execute_remote_injection(pid, regs, lib_path, mode);
366+
bool success = execute_remote_injection(pid, regs, lib_path, mode);
367367

368368
// Restore State directly.
369369
// The instruction pointer (REG_IP) is already correct in the backup.
@@ -373,7 +373,7 @@ bool inject_after_start(int pid, const char *lib_path, TraceMode mode) {
373373
return false;
374374
}
375375

376-
return sucess;
376+
return success;
377377
}
378378

379379
// Macro helper to check for specific ptrace stop events.
@@ -392,10 +392,17 @@ static bool wait_for_process(int pid, int *status) {
392392

393393
/**
394394
* @brief Copies the library to a world-readable temporary file to bypass DAC restrictions.
395-
* @param src_path The original path (e.g., /data/adb/neozygisk/lib64/libzygisk.so)
395+
*
396+
* Kept for debugging: when the work directory is somewhere the target cannot read, point
397+
* perform_injection() at this instead of the packaged library to find out whether a failed
398+
* injection is a permission problem or something else. It is deliberately not wired into
399+
* the normal path -- it only relocates the library, while the control socket stays where it
400+
* is, so it cannot on its own make an unreachable work directory usable.
401+
*
402+
* @param src_path The original path (e.g., /data/system/neozygisk/lib64/libzygisk.so)
396403
* @return The path to the temporary file, or an empty string on failure.
397404
*/
398-
static std::string copy_to_temp(const std::string &src_path) {
405+
[[maybe_unused]] static std::string copy_to_temp(const std::string &src_path) {
399406
char tmp_path[] = "/data/local/tmp/zygisk_XXXXXX.so";
400407

401408
// mkstemps securely creates the file with a random 6-character string replacing XXXXXX.
@@ -449,32 +456,10 @@ static bool perform_injection(int pid, TraceMode mode) {
449456
lib_path += "/lib" LP_SELECT("", "64") "/libzygisk.so";
450457
bool process_started = mode == TraceMode::STANDALONE || mode == TraceMode::SYSTEM_SERVER;
451458

452-
std::string inject_path = lib_path;
453-
bool use_temp = false;
454-
455-
// if (mode == TraceMode::SYSTEM_SERVER) {
456-
// inject_path = copy_to_temp(lib_path);
457-
// if (inject_path.empty()) {
458-
// LOGE("aborting injection: could not create accessible library copy");
459-
// return false;
460-
// }
461-
// use_temp = true;
462-
// }
463-
464-
bool success = false;
465-
466459
if (process_started) {
467-
success = inject_after_start(pid, inject_path.c_str(), mode);
468-
} else {
469-
success = inject_before_start(pid, inject_path.c_str(), mode);
470-
}
471-
472-
if (use_temp) {
473-
LOGV("cleaning up temporary library: %s", inject_path.c_str());
474-
unlink(inject_path.c_str());
460+
return inject_after_start(pid, lib_path.c_str(), mode);
475461
}
476-
477-
return success;
462+
return inject_before_start(pid, lib_path.c_str(), mode);
478463
}
479464

480465
/**

module/src/post-fs-data.sh

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -51,16 +51,29 @@ fi
5151

5252
[ "$DEBUG" = true ] && export RUST_BACKTRACE=1
5353

54-
if [ -z $(pidof system_server) ]; then
55-
if [ -f $MODDIR/bin/zygisk-ptrace64 ]; then
56-
$MODDIR/bin/zygisk-ptrace64 monitor &
57-
elif [ -f $MODDIR/bin/zygisk-ptrace32 ]; then
58-
$MODDIR/bin/zygisk-ptrace32 monitor &
59-
fi
54+
# app_process names itself "zygote64" when built LP64 and "zygote" otherwise, so the
55+
# 32-bit branch only applies to 32-bit-only devices. On a 64/32 device the secondary
56+
# 32-bit zygote is not covered by the standalone path; only the monitor path sees it.
57+
if [ -f "$MODDIR/bin/zygisk-ptrace64" ]; then
58+
TRACER="$MODDIR/bin/zygisk-ptrace64"
59+
ZYGOTE="zygote64"
60+
elif [ -f "$MODDIR/bin/zygisk-ptrace32" ]; then
61+
TRACER="$MODDIR/bin/zygisk-ptrace32"
62+
ZYGOTE="zygote"
63+
else
64+
log -p e -t "zygisk-sh" "No tracer binary found in $MODDIR/bin"
65+
exit 1
66+
fi
67+
68+
if [ -z "$(pidof system_server)" ]; then
69+
# Normal boot: supervise zygote from the start and inject as it spawns.
70+
"$TRACER" monitor &
6071
else
61-
if [ -f $MODDIR/bin/zygisk-ptrace64 ]; then
62-
$MODDIR/bin/zygisk-ptrace64 trace $(pidof zygote64) --standalone &
63-
elif [ -f $MODDIR/bin/zygisk-ptrace32 ]; then
64-
$MODDIR/bin/zygisk-ptrace32 trace $(pidof zygote32) --standalone &
72+
# Late injection: the system is already up, so attach to the running zygote.
73+
ZYGOTE_PID="$(pidof "$ZYGOTE")"
74+
if [ -z "$ZYGOTE_PID" ]; then
75+
log -p e -t "zygisk-sh" "Cannot inject: no running $ZYGOTE found"
76+
exit 1
6577
fi
78+
"$TRACER" trace "$ZYGOTE_PID" --standalone &
6679
fi

module/src/service.sh

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ if [ "$(which magisk)" ]; then
1717
if [ -f "$file/service.sh" ]; then
1818
cd "$file"
1919
log -p i -t "zygisk-sh" "Manually trigger service.sh for $file"
20-
if [ -z $system_server_pid ]; then
20+
if [ -z "$system_server_pid" ]; then
2121
sh "$(realpath ./service.sh)" &
2222
else
2323
sh "$(realpath ./service.sh)" --late-inject &
@@ -28,7 +28,15 @@ if [ "$(which magisk)" ]; then
2828
done
2929
fi
3030

31-
if [ ! -z $system_server_pid ]; then
32-
log -p i -t "zygisk-sh" "Maually inject into system_server $system_server_pid"
33-
./bin/zygisk-ptrace64 trace $system_server_pid --system_server
31+
if [ -n "$system_server_pid" ]; then
32+
if [ -f "$MODDIR/bin/zygisk-ptrace64" ]; then
33+
TRACER="$MODDIR/bin/zygisk-ptrace64"
34+
elif [ -f "$MODDIR/bin/zygisk-ptrace32" ]; then
35+
TRACER="$MODDIR/bin/zygisk-ptrace32"
36+
else
37+
log -p e -t "zygisk-sh" "No tracer binary found in $MODDIR/bin"
38+
exit 1
39+
fi
40+
log -p i -t "zygisk-sh" "Manually inject into system_server $system_server_pid"
41+
"$TRACER" trace "$system_server_pid" --system_server
3442
fi

0 commit comments

Comments
 (0)