Fixed: Fix usb fd not being sent due to refactoring in 3c1a6be

dougvj · agnostic-apollo · commit 4c6a5196c969 · 2024-03-20T05:41:33.000+05:00
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
diff --git a/app/src/main/java/com/termux/api/apis/UsbAPI.java b/app/src/main/java/com/termux/api/apis/UsbAPI.java
@@ -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");
                                 }
diff --git a/app/src/main/java/com/termux/api/util/ResultReturner.java b/app/src/main/java/com/termux/api/util/ResultReturner.java
@@ -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);
+            }
+          }
         }
     }
 
@@ -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())
@@ -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());
@@ -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);

Original file line number	Diff line number	Diff line change
`@@ -66,8 +66,7 @@ public void writeResult(PrintWriter out) {`
`66`	`66`	`if (result < 0) {`
`67`	`67`	`out.append("Failed to open device\n");`
`68`	`68`	`} else {`
`69`		`- this.setFd(result);`
`70`		`- out.append("@"); // has to be non-empty`
	`69`	`+ this.sendFd(out, result);`
`71`	`70`	`}`
`72`	`71`	`} else out.append("No permission\n");`
`73`	`72`	`}`