Fix UIAutomation.executeShellCommand()

UIAutomation.executeShellCommand() creates pipe()'ed FDs and transfers
ownership to the service and the caller. So, UIAutomationConnection
needs to close the passed sink FD and the caller of
UIAutomation.executeShellCommand() should close the returned source FD.

However, when Runtime.exec() throws an exception FDs are not properly
closed. There are two types of exceptions it can throw: a) one that is
propagated the caller, and b) the other that is missing in the middle.

The server side doesn't close the sink FD in both cases. The caller side
behaves differently:
For a), UIAutomation.executeShellCommand() throws the exception and the
source FD is lost without closing.
For b), UIAutomation.executeShellCommand() returns the source FD, but
since the other end of this FD isn't closed in the server, the caller's
read() hangs.

This change fixes:
- UIAutomationConnection.executeShellCommand() method closes FDs on
  exception.
- UIAutomation.executeShellCommand() method closes FDs on exception.

Bug: 349038571
Test: UiAutomationShellTest
Change-Id: Id46a42cbe7fe330627e31389595562af79301a84

2 files changed, 31 insertions, 10 deletions

diff --git a/core/java/android/app/UiAutomation.java b/core/java/android/app/UiAutomation.java
index 348d4d8fd809..a249c394467d 100644
--- a/core/java/android/app/UiAutomation.java
+++ b/core/java/android/app/UiAutomation.java
@@ -1647,10 +1647,13 @@ public final class UiAutomation {
 
             // Calling out without a lock held.
             mUiAutomationConnection.executeShellCommand(command, sink, null);
-        } catch (IOException ioe) {
-            Log.e(LOG_TAG, "Error executing shell command!", ioe);
-        } catch (RemoteException re) {
-            Log.e(LOG_TAG, "Error executing shell command!", re);
+        } catch (IOException | RemoteException e) {
+            Log.e(LOG_TAG, "Error executing shell command!", e);
+        } catch (IllegalArgumentException | NullPointerException | SecurityException e) {
+            // An exception of these types is propagated from the server.
+            // Rethrow it to keep the old behavior. To avoid FD leak, close the source.
+            IoUtils.closeQuietly(source);
+            throw e;
         } finally {
             IoUtils.closeQuietly(sink);
         }
@@ -1734,10 +1737,15 @@ public final class UiAutomation {
             // Calling out without a lock held.
             mUiAutomationConnection.executeShellCommandWithStderr(
                     command, sink_read, source_write, stderr_sink_read);
-        } catch (IOException ioe) {
-            Log.e(LOG_TAG, "Error executing shell command!", ioe);
-        } catch (RemoteException re) {
-            Log.e(LOG_TAG, "Error executing shell command!", re);
+        } catch (IOException | RemoteException e) {
+            Log.e(LOG_TAG, "Error executing shell command!", e);
+        } catch (IllegalArgumentException | SecurityException | NullPointerException e) {
+            // An exception of these types is propagated from the server.
+            // Rethrow it to keep the old behavior. To avoid FD leaks, close the sources.
+            IoUtils.closeQuietly(sink_write);
+            IoUtils.closeQuietly(source_read);
+            IoUtils.closeQuietly(stderr_source_read);
+            throw e;
         } finally {
             IoUtils.closeQuietly(sink_read);
             IoUtils.closeQuietly(source_write);
diff --git a/core/java/android/app/UiAutomationConnection.java b/core/java/android/app/UiAutomationConnection.java
index 3c4bd9eb0747..5e21e05193fc 100644
--- a/core/java/android/app/UiAutomationConnection.java
+++ b/core/java/android/app/UiAutomationConnection.java
@@ -550,8 +550,21 @@ public final class UiAutomationConnection extends IUiAutomationConnection.Stub {
 
         try {
             process = Runtime.getRuntime().exec(command);
-        } catch (IOException exc) {
-            throw new RuntimeException("Error running shell command '" + command + "'", exc);
+        } catch (IOException ex) {
+            // Make sure the passed FDs are closed.
+            IoUtils.closeQuietly(sink);
+            IoUtils.closeQuietly(source);
+            IoUtils.closeQuietly(stderrSink);
+            // No to need to wrap in RuntimeException. Only to keep the old behavior.
+            // This is just logged and not propagated to the remote caller anyway.
+            throw new RuntimeException("Error running shell command '" + command + "'", ex);
+        } catch (IllegalArgumentException | NullPointerException | SecurityException ex) {
+            // Make sure the passed FDs are closed.
+            IoUtils.closeQuietly(sink);
+            IoUtils.closeQuietly(source);
+            IoUtils.closeQuietly(stderrSink);
+            // Rethrow the exception. This will be propagated to the remote caller.
+            throw ex;
         }
 
         // Read from process and write to pipe