Skip to content

Commit

Permalink
Fixed: Fix usb fd not being sent due to refactoring in 3c1a6be
Browse files Browse the repository at this point in the history
This is a minor refactor of the WithAncillaryFd ResultReturner that achieves two goals:

1. The usb fd is no longer closed before the message is sent over the socket. This resolves #643

2. We queue the fd to be sent (using setFileDescriptorsForSend) and then write `@` as data and then flush the output message in a single operation so that fd is actually sent.

Writing `@` is required because as per docs "The file descriptors will be sent with the next write of normal data, and will be delivered in a single ancillary message.". The `@` character is not special, it is just the chosen character expected as the message by the native `termux-api` command when a fd is sent.
- https://developer.android.com/reference/android/net/LocalSocket#setFileDescriptorsForSend(java.io.FileDescriptor[])
- https://github.com/termux/termux-api-package/blob/e62bdadea3f26b60430bb85248f300fee68ecdcc/termux-api.c#L358

Explanation for why this got broken in 3c1a6be by @agnostic-apollo at #644 (comment):

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.

- https://github.com/termux/termux-api/blob/3bea194249586a7dcb143e66b46c1694cb6ca21a/app/src/main/java/com/termux/api/apis/UsbAPI.java#L69-L70
- https://github.com/termux/termux-api/blob/3c1a6be86ff0768fa8be029267fbe96dd7fbfb7f/app/src/main/java/com/termux/api/util/ResultReturner.java#L173-L181

Previously, before 3c1a6be, 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 the 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

Closes #643
  • Loading branch information
dougvj authored and agnostic-apollo committed Mar 20, 2024
1 parent a9abc96 commit 4c6a519
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 18 deletions.
3 changes: 1 addition & 2 deletions app/src/main/java/com/termux/api/apis/UsbAPI.java
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,7 @@ public void writeResult(PrintWriter out) {
if (result < 0) {
out.append("Failed to open device\n");
} else {
this.setFd(result);
out.append("@"); // has to be non-empty
this.sendFd(out, result);
}
} else out.append("No permission\n");
}
Expand Down
65 changes: 49 additions & 16 deletions app/src/main/java/com/termux/api/util/ResultReturner.java
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,53 @@ public final void setInput(InputStream inputStream) throws Exception {
}

public static abstract class WithAncillaryFd implements ResultWriter {
private int fd = -1;
private LocalSocket outputSocket = null;
private final ParcelFileDescriptor[] pfds = { null };

public final void setFd(int newFd) {
fd = newFd;
public final void setOutputSocketForFds(LocalSocket outputSocket) {
this.outputSocket = outputSocket;
}

public final int getFd() {
return fd;
public final void sendFd(PrintWriter out, int fd) {
// If fd already sent, then error out as we only support sending one currently.
if (this.pfds[0] != null) {
Logger.logStackTraceWithMessage(LOG_TAG, "File descriptor already sent", new Exception());
return;
}

this.pfds[0] = ParcelFileDescriptor.adoptFd(fd);
FileDescriptor[] fds = { pfds[0].getFileDescriptor() };

// Set fd to be sent
outputSocket.setFileDescriptorsForSend(fds);

// As per the docs:
// > The file descriptors will be sent with the next write of normal data, and will be
// delivered in a single ancillary message.
// - https://developer.android.com/reference/android/net/LocalSocket#setFileDescriptorsForSend(java.io.FileDescriptor[])
// So we write the `@` character. It is not special, it is just the chosen character
// expected as the message by the native `termux-api` command when a fd is sent.
// - https://github.com/termux/termux-api-package/blob/e62bdadea3f26b60430bb85248f300fee68ecdcc/termux-api.c#L358
out.print("@");

// Actually send the by fd by flushing the data previously written (`@`) as PrintWriter is buffered.
out.flush();

// Clear existing fd after it has been sent, otherwise it will get sent for every data write,
// even though we are currently not writing anything else. Android will not clear it automatically.
// - 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
outputSocket.setFileDescriptorsForSend(null);
}

public final void cleanupFds() {
if (this.pfds[0] != null) {
try {
this.pfds[0].close();
} catch (IOException e) {
Logger.logStackTraceWithMessage(LOG_TAG, "Failed to close file descriptor", e);
}
}
}
}

Expand Down Expand Up @@ -152,7 +191,6 @@ public static void returnData(Object context, final Intent intent, final ResultW
PrintWriter writer = null;
LocalSocket outputSocket = null;
try {
final ParcelFileDescriptor[] pfds = { null };
outputSocket = new LocalSocket();
String outputSocketAdress = intent.getStringExtra(SOCKET_OUTPUT_EXTRA);
if (outputSocketAdress == null || outputSocketAdress.isEmpty())
Expand All @@ -162,6 +200,9 @@ public static void returnData(Object context, final Intent intent, final ResultW
writer = new PrintWriter(outputSocket.getOutputStream());

if (resultWriter != null) {
if(resultWriter instanceof WithAncillaryFd) {
((WithAncillaryFd) resultWriter).setOutputSocketForFds(outputSocket);
}
if (resultWriter instanceof BinaryOutput) {
BinaryOutput bout = (BinaryOutput) resultWriter;
bout.setOutput(outputSocket.getOutputStream());
Expand All @@ -178,19 +219,11 @@ public static void returnData(Object context, final Intent intent, final ResultW
} else {
resultWriter.writeResult(writer);
}
if(resultWriter instanceof WithAncillaryFd) {
int fd = ((WithAncillaryFd) resultWriter).getFd();
if (fd >= 0) {
pfds[0] = ParcelFileDescriptor.adoptFd(fd);
FileDescriptor[] fds = { pfds[0].getFileDescriptor() };
outputSocket.setFileDescriptorsForSend(fds);
}
if (resultWriter instanceof WithAncillaryFd) {
((WithAncillaryFd) resultWriter).cleanupFds();
}
}

if(pfds[0] != null) {
pfds[0].close();
}

if (asyncResult != null && receiver.isOrderedBroadcast()) {
asyncResult.setResultCode(0);
Expand Down

0 comments on commit 4c6a519

Please sign in to comment.