ADFA-4539: Stop must kill the rsync daemon's per-connection children#131
Merged
Merged
Conversation
The Share daemon runs 'librsync.so --daemon --no-detach' and forks a child per connection. stop() only called rsyncProcess.destroy(), which SIGTERMs the parent; the connection child kept streaming the transfer and reparented to init. So a Stop mid-share (e.g. after accidentally sharing the 80 GB module instead of the 15 GB one) did not actually stop the transfer. stop() now, after destroying the parent, sweeps /proc and SIGKILLs every rsync process that is ours -- identified by our app-private librsync.so path in the cmdline (unique to this app, so it matches the daemon parent + all connection children and nothing else, reparent-proof). Same-UID kills only, never our own pid. Covers both the share (daemon) and receive (client) sides. - New pure sync/domain/RsyncProcessMatcher.isOurRsyncProcess(cmdline, binPath), unit-tested. - RsyncManager remembers the binary path and adds killOurRsyncProcesses()/ readCmdline() (minSdk 24: no Process.pid()/waitFor(timeout), hence the /proc cmdline match rather than a pid tree).
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
1 participant
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.
The bug
The Share daemon runs
librsync.so --daemon --no-detach. rsync forks a child per connection to serve it.stop()only didrsyncProcess.destroy(), which SIGTERMs the parent daemon; the forked connection child kept streaming the transfer and reparented to init when the parent died. So hitting Stop mid-share (reported case: you accidentally shared the 80 GB module instead of the 15 GB one) did not stop the transfer.The fix
After destroying the parent,
stop()sweeps/procand SIGKILLs every rsync process that is ours, matched by our app-privatelibrsync.sopath in the process cmdline:nativeLibraryDir, unique to this app → matches the daemon parent and every connection child, nothing else. Reparent-proof (cmdline doesn't change on reparent), unlike a ppid/pgid walk.android.os.Process.killProcess= SIGKILL); never our own pid.Ordering: destroy the parent first (stops accepting/forking), then sweep survivors.
Why /proc-cmdline, not a pid tree
minSdk 24;Process.pid()/waitFor(timeout)are API 26+ (the codebase notes this inPRootEngine.killProcess()). Matching our unique binary path in/proc/<pid>/cmdlineis portable from 24 and robust.Changes
sync/domain/RsyncProcessMatcher.isOurRsyncProcess(cmdline, binPath)— the testable decision. Unit-tested (5 cases).RsyncManager— remembersrsyncBinPath;stop()→killOurRsyncProcesses()scans/proc, reads eachcmdline(readCmdline, hand-rolled sinceFiles.readAllBytesis API 26+) and SIGKILLs matches.Lint / CI
android.os.Processfully-qualified. Nojavacin sandbox — please build:app:testDebugUnitTest+:app:lintDebug.Test (device)
logcat:ADFA-4539: killed lingering rsync pid <n>.adb shell ps -A | grep rsyncon the host after Stop → no lingering rsync.