termux-usb: fix issue with usb fd being closed before sent over socket

[dougvj]

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 termux-usb -e broken with empty non-fd response #643

2. 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.

[agnostic-apollo]

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

• https://developer.android.com/reference/android/net/LocalSocket#setFileDescriptorsForSend(java.io.FileDescriptor[])

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.

• termux-api/app/src/main/java/com/termux/api/apis/UsbAPI.java

  Lines 69 to 70 in 3bea194

  	this.setFd(result);
  	out.append("@"); // has to be non-empty

termux-api/app/src/main/java/com/termux/api/util/ResultReturner.java

Lines 173 to 181 in 3c1a6be

	} 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);

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 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.

• https://cs.android.com/android/platform/superproject/+/android-14.0.0_r1:libcore/ojluni/src/main/java/java/io/OutputStream.java;l=180

• https://cs.android.com/android/_/android/platform/frameworks/base/+/refs/tags/android-14.0.0_r1:core/java/android/net/LocalSocketImpl.java;l=121

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]

Hi, when can this be merged? It breaks at least one downstream project.

[dougvj]

I made the changes requested and verified that the fix still works.

[agnostic-apollo]

Looks good, thanks. Will merge it soon.

[nohajc]

Hi, any particular reason this hasn't been merged yet? Thanks.

[agnostic-apollo]

Sorry, got unexpectedly busy with family stuff and then got sick, am back now, will merge it within next couple of days. Thanks for the ping.

[agnostic-apollo]

Thanks for working on the fix.