fix: interleaved datagrams on the controller socket - #379
Open
noamtu123 wants to merge 2 commits into
Open
Conversation
The 64-bit and 32-bit daemons share the controller socket, and each sent its multi-field messages as several datagrams. The two could interleave, so the controller paired one daemon's length with the other's payload and derived a bogus length from it, then waited for bytes that were never sent. Serialize each message and send it as a single datagram, which is delivered atomically, and parse the fields out of that datagram instead of reading them from the socket one at a time. Parsing from the buffer also bounds every field against the datagram size.
read_loop() is also called from the ptracer's single-threaded epoll callback on a non-blocking socket. Retrying EAGAIN without a bound cannot succeed there, as the loop that would deliver the remaining bytes is the one held by the retry, so a short read becomes a permanent stall rather than a failed message. Wait for readability with poll() and give up after a deadline, returning the bytes read so far so the caller's existing error handling runs. This is independent of the previous commit and can be dropped without affecting it.
|
All Contributors have signed the CLA. The PR is now allowed to be merged. |
Author
|
I have read the CLA Document and I hereby sign the CLA |
performanc-bot
added a commit
to PerformanC/CLA-Signatures
that referenced
this pull request
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Commit 1 —
fix: interleaved datagrams on the controller socketzygiskdserializesDAEMON_SET_INFOandDAEMON_SET_ERROR_INFOinto a singlebuffer and sends each as one datagram. The controller reads a whole datagram and
parses the fields out of it, with bounds checks, instead of reading each field
from the socket separately.
Commit 2 —
improve: bound the EAGAIN retry in read_loop_offset(optional)Bounds the
EAGAINretry inread_loop_offset()withpoll()and a deadline.This one is independent and can be dropped without affecting the first; it is
included because it removes the severity of the failure, not just this cause.
Why
zygiskd_start()sent each field of a message as its own datagram:and
rezygiskd_listener_callback()read them back one field at a time. Both the64-bit and the 32-bit daemon send to this socket, and nothing makes either the
sequence or the socket exclusive to one of them, so the two sequences can
interleave in the receive queue. When they do, a
read_uint32_t()can start onone daemon's 1-byte command datagram and take its remaining 3 bytes from the
next datagram, yielding a length nobody sent.
The lengths observed on a device where this happens decode exactly as
[command][real length][0][0]:06 0D 00 00DAEMON64_SET_INFO(6) + 13 ("KernelSU Next")07 0D 00 00DAEMON32_SET_INFO(7) + 13 ("KernelSU Next")07 0B 00 00DAEMON32_SET_INFO(7) + 11 ("treat_wheel")The controller then waits for those bytes, which nobody will send.
read_loop()retries on
EAGAIN, and since the socket is non-blocking and serviced by themonitor's single-threaded
epollloop, that retry cannot succeed: the loop thatwould deliver more data is the one blocked inside the retry.
Because the monitor holds init under
PTRACE_O_TRACEFORK, the result is a boothang. With the loop stuck it never reaches
epoll_wait(), never drains theSIGCHLDreporting init'sPTRACE_EVENT_FORK, and never sendsPTRACE_CONT,so init stays in
t (tracing stop)and the device does not finish booting. On anon-GKI 4.9 arm64 device this happened on roughly 1 boot in 6.
Datagrams are delivered atomically, so sending each message as one removes the
interleaving. Parsing from the received buffer also bounds every field against
the datagram size, so a malformed message can no longer yield a length that is
not backed by data.
Checkmarks
Additional information
Testing, on the device that reproduces this:
poll()bound)Commit 1 was deliberately tested without commit 2, so the hang could still
occur if the interleaving were not the real cause.
state.jsonalso reportedrootand the full module list on 31/31 boots afterwards; before the change,the boots where the desynchronization happened without hanging left it unwritten,
because the read-failure path returns before
update_status().Notes on the change itself:
could not reduce it to one file: with only the sender changed, the
controller's 1-byte command read consumes and discards the rest of the
datagram; with only the controller changed, there is no atomic message to read.
DAEMON_SET_ERROR_INFOis included for the same reason — leaving it asseveral datagrams while the controller parses from one buffer would silently
break error reporting.
monitor.cno longer uses anything fromsocket_utils.h. Ileft the include alone rather than mix a cleanup in; note it also provides
<stdint.h>transitively, so removing it needs that added.TEMP_FAILURE_RETRYis kept where it already was, for consistency with thesurrounding calls, though the syntax standard lists GNU extensions as
prohibited.
zygiskdbuilds clean under its-Werror -Wconversion -Wpedantic, and the newcode adds no warnings under
-Wconversion -Wshadowon the loader side either.If you would rather take only the first commit, or implement it differently, that
is completely fine — the important part is the decode table above, which should
be reproducible anywhere both daemons start close together.