agnostic-apollo
commented
7 months ago Thanks for the pull. Some details for clarity...
The setFileDescriptorsForSend() docs state
The file descriptors will be sent with the next write of normal data, and will be delivered in a single ancillary message
Previously, in UsbApi, the fd was only temporarily stored in WithAncillaryFd, but not actually sent or passed to LocalSocket, and then a @ was written, possibly in attempts to send it, even though like I said, it wasn't passed to LocalSocket with setFileDescriptorsForSend() yet, so it was a pointless write, but worked due to autoflush on close() (check below). After this, when writeResult() returned, then fd was passed to LocalSocket, but still not sent, since no data was written after it.
Previously, before https://github.com/termux/termux-api/commit/3c1a6be86ff0768fa8be029267fbe96dd7fbfb7f, the PrintWriter in ResultReturner was using java try-with-resources, which automatically closes when try gets out of scope. Additionally, when new PrintWriter(outputSocket.getOutputStream() is called, it uses a BufferedWriter internally, which when closed, automatically flushes. What this would do is actually send the socket that was previously passed to LocalSocket. Moreover, PrintWriter was also closed before fd was closed since try-with-resources finished before, but after the commit, it was closed afterwards, causing your issue.
- https://cs.android.com/android/platform/superproject/+/android-14.0.0_r1:libcore/ojluni/src/main/java/java/io/PrintWriter.java;l=164
- https://cs.android.com/android/platform/superproject/+/android-14.0.0_r1:libcore/ojluni/src/main/java/java/io/BufferedWriter.java;l=268
- https://cs.android.com/android/platform/superproject/main/+/main:libcore/ojluni/src/main/java/java/io/Writer.java;l=412
Note that calling flush() on SocketOutputStream does nothing, its not overridden by it, and base OutputStream does nothing.
You should passed PrintWriter to sendFd(), then after setFileDescriptorsForSend(fd), write @ (remove it from UsbApi), then run setFileDescriptorsForSend(null) to clear existing fd, otherwise it will get written for every write, even though we are currently not writing anything. Android will not clear it automatically. The @ character is used by termux-api, so do not change it.
Also fix typo in "otherwise we mysterious socket issues", preferably remove that part, and replace it completely with "Make sure we send the file descriptors before closing them by flushing them and data (@)". Also add comment before writing @ with reference to docs that "The file descriptors will be sent with the next write of normal data...".
- https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/core/java/android/net/LocalSocketImpl.java;l=523?q=setFileDescriptorsForSend
- https://cs.android.com/android/_/android/platform/frameworks/base/+/refs/tags/android-14.0.0_r1:core/jni/android_net_LocalSocketImpl.cpp;l=194
- https://github.com/termux/termux-api-package/blob/e62bdadea3f26b60430bb85248f300fee68ecdcc/termux-api.c#L358
You should also follow commit guidelines for format, check git history and https://github.com/termux/termux-app?tab=readme-ov-file#commit-messages-guidelines
Will need to update the commit title and description as well with this new info, probably should link this pull in the message.
nohajc
dougvj
This is a minor refactor of the WithAncillaryFd ResultReturner that achieves two goals:
The usb fd is no longer closed before the message is sent over the socket. This resolves #643
We queue the fds to be sent (using setFileDescriptorsForSend) Before printing an output message to ensure the order is correct. This was working before by virtue of the buffering mechanism, but would break if there were no buffering.