FileBridge: fix fd ownership mismanagement.

This patch switches everything over to ParcelFileDescriptor, but the
important part of the change is changing FileBridge.getClientSocket to
return a ParcelFileDescriptor. Previously, it returned a raw
FileDescriptor that was closed by FileBridge, and the only non-test
caller of that function was taking it and constructing a
ParcelFileDescriptor from it, which would also attempt to close the fd,
leading to an fdsan abort.

Bug: http://b/162811367
Test: atest FileBridgeTest
Test: treehugger
Change-Id: I724ea7601bb072c98895f68abc08bb0e339d1db0
(cherry picked from commit 4c392e8057d8b9950fbe51922136dcd70e9fb750)
diff --git a/core/java/android/os/FileBridge.java b/core/java/android/os/FileBridge.java
index 21fd819..ab5637c 100644
--- a/core/java/android/os/FileBridge.java
+++ b/core/java/android/os/FileBridge.java
@@ -16,7 +16,6 @@
 
 package android.os;
 
-import static android.system.OsConstants.AF_UNIX;
 import static android.system.OsConstants.SOCK_STREAM;
 
 import android.system.ErrnoException;
@@ -58,17 +57,19 @@
     /** CMD_CLOSE */
     private static final int CMD_CLOSE = 3;
 
-    private FileDescriptor mTarget;
+    private ParcelFileDescriptor mTarget;
 
-    private final FileDescriptor mServer = new FileDescriptor();
-    private final FileDescriptor mClient = new FileDescriptor();
+    private ParcelFileDescriptor mServer;
+    private ParcelFileDescriptor mClient;
 
     private volatile boolean mClosed;
 
     public FileBridge() {
         try {
-            Os.socketpair(AF_UNIX, SOCK_STREAM, 0, mServer, mClient);
-        } catch (ErrnoException e) {
+            ParcelFileDescriptor[] fds = ParcelFileDescriptor.createSocketPair(SOCK_STREAM);
+            mServer = fds[0];
+            mClient = fds[1];
+        } catch (IOException e) {
             throw new RuntimeException("Failed to create bridge");
         }
     }
@@ -80,15 +81,14 @@
     public void forceClose() {
         IoUtils.closeQuietly(mTarget);
         IoUtils.closeQuietly(mServer);
-        IoUtils.closeQuietly(mClient);
         mClosed = true;
     }
 
-    public void setTargetFile(FileDescriptor target) {
+    public void setTargetFile(ParcelFileDescriptor target) {
         mTarget = target;
     }
 
-    public FileDescriptor getClientSocket() {
+    public ParcelFileDescriptor getClientSocket() {
         return mClient;
     }
 
@@ -96,32 +96,33 @@
     public void run() {
         final byte[] temp = new byte[8192];
         try {
-            while (IoBridge.read(mServer, temp, 0, MSG_LENGTH) == MSG_LENGTH) {
+            while (IoBridge.read(mServer.getFileDescriptor(), temp, 0, MSG_LENGTH) == MSG_LENGTH) {
                 final int cmd = Memory.peekInt(temp, 0, ByteOrder.BIG_ENDIAN);
                 if (cmd == CMD_WRITE) {
                     // Shuttle data into local file
                     int len = Memory.peekInt(temp, 4, ByteOrder.BIG_ENDIAN);
                     while (len > 0) {
-                        int n = IoBridge.read(mServer, temp, 0, Math.min(temp.length, len));
+                        int n = IoBridge.read(mServer.getFileDescriptor(), temp, 0,
+                                              Math.min(temp.length, len));
                         if (n == -1) {
                             throw new IOException(
                                     "Unexpected EOF; still expected " + len + " bytes");
                         }
-                        IoBridge.write(mTarget, temp, 0, n);
+                        IoBridge.write(mTarget.getFileDescriptor(), temp, 0, n);
                         len -= n;
                     }
 
                 } else if (cmd == CMD_FSYNC) {
                     // Sync and echo back to confirm
-                    Os.fsync(mTarget);
-                    IoBridge.write(mServer, temp, 0, MSG_LENGTH);
+                    Os.fsync(mTarget.getFileDescriptor());
+                    IoBridge.write(mServer.getFileDescriptor(), temp, 0, MSG_LENGTH);
 
                 } else if (cmd == CMD_CLOSE) {
                     // Close and echo back to confirm
-                    Os.fsync(mTarget);
-                    Os.close(mTarget);
+                    Os.fsync(mTarget.getFileDescriptor());
+                    mTarget.close();
                     mClosed = true;
-                    IoBridge.write(mServer, temp, 0, MSG_LENGTH);
+                    IoBridge.write(mServer.getFileDescriptor(), temp, 0, MSG_LENGTH);
                     break;
                 }
             }
@@ -143,17 +144,11 @@
             mClient = clientPfd.getFileDescriptor();
         }
 
-        public FileBridgeOutputStream(FileDescriptor client) {
-            mClientPfd = null;
-            mClient = client;
-        }
-
         @Override
         public void close() throws IOException {
             try {
                 writeCommandAndBlock(CMD_CLOSE, "close()");
             } finally {
-                IoBridge.closeAndSignalBlockedThreads(mClient);
                 IoUtils.closeQuietly(mClientPfd);
             }
         }
diff --git a/core/tests/coretests/src/android/os/FileBridgeTest.java b/core/tests/coretests/src/android/os/FileBridgeTest.java
index d4f6b1f..708bfa6 100644
--- a/core/tests/coretests/src/android/os/FileBridgeTest.java
+++ b/core/tests/coretests/src/android/os/FileBridgeTest.java
@@ -16,6 +16,9 @@
 
 package android.os;
 
+import static android.os.ParcelFileDescriptor.MODE_CREATE;
+import static android.os.ParcelFileDescriptor.MODE_READ_WRITE;
+
 import android.os.FileBridge.FileBridgeOutputStream;
 import android.test.AndroidTestCase;
 import android.test.MoreAsserts;
@@ -25,7 +28,6 @@
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.Random;
@@ -33,7 +35,7 @@
 public class FileBridgeTest extends AndroidTestCase {
 
     private File file;
-    private FileOutputStream fileOs;
+    private ParcelFileDescriptor outputFile;
     private FileBridge bridge;
     private FileBridgeOutputStream client;
 
@@ -44,17 +46,17 @@
         file = getContext().getFileStreamPath("meow.dat");
         file.delete();
 
-        fileOs = new FileOutputStream(file);
+        outputFile = ParcelFileDescriptor.open(file, MODE_CREATE | MODE_READ_WRITE);
 
         bridge = new FileBridge();
-        bridge.setTargetFile(fileOs.getFD());
+        bridge.setTargetFile(outputFile);
         bridge.start();
         client = new FileBridgeOutputStream(bridge.getClientSocket());
     }
 
     @Override
     protected void tearDown() throws Exception {
-        fileOs.close();
+        outputFile.close();
         file.delete();
     }
 
diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java
index ea53132..33193df 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerSession.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java
@@ -947,6 +947,23 @@
         }
     }
 
+    private ParcelFileDescriptor openTargetInternal(String path, int flags, int mode)
+            throws IOException, ErrnoException {
+        // TODO: this should delegate to DCS so the system process avoids
+        // holding open FDs into containers.
+        final FileDescriptor fd = Os.open(path, flags, mode);
+        return new ParcelFileDescriptor(fd);
+    }
+
+    private ParcelFileDescriptor createRevocableFdInternal(RevocableFileDescriptor fd,
+            ParcelFileDescriptor pfd) throws IOException {
+        int releasedFdInt = pfd.detachFd();
+        FileDescriptor releasedFd = new FileDescriptor();
+        releasedFd.setInt$(releasedFdInt);
+        fd.init(mContext, releasedFd);
+        return fd.getRevocableFileDescriptor();
+    }
+
     private ParcelFileDescriptor doWriteInternal(String name, long offsetBytes, long lengthBytes,
             ParcelFileDescriptor incomingFd) throws IOException {
         // Quick sanity check of state, and allocate a pipe for ourselves. We
@@ -979,21 +996,20 @@
                 Binder.restoreCallingIdentity(identity);
             }
 
-            // TODO: this should delegate to DCS so the system process avoids
-            // holding open FDs into containers.
-            final FileDescriptor targetFd = Os.open(target.getAbsolutePath(),
+            ParcelFileDescriptor targetPfd = openTargetInternal(target.getAbsolutePath(),
                     O_CREAT | O_WRONLY, 0644);
             Os.chmod(target.getAbsolutePath(), 0644);
 
             // If caller specified a total length, allocate it for them. Free up
             // cache space to grow, if needed.
             if (stageDir != null && lengthBytes > 0) {
-                mContext.getSystemService(StorageManager.class).allocateBytes(targetFd, lengthBytes,
+                mContext.getSystemService(StorageManager.class).allocateBytes(
+                        targetPfd.getFileDescriptor(), lengthBytes,
                         PackageHelper.translateAllocateFlags(params.installFlags));
             }
 
             if (offsetBytes > 0) {
-                Os.lseek(targetFd, offsetBytes, OsConstants.SEEK_SET);
+                Os.lseek(targetPfd.getFileDescriptor(), offsetBytes, OsConstants.SEEK_SET);
             }
 
             if (incomingFd != null) {
@@ -1003,8 +1019,9 @@
                 // inserted above to hold the session active.
                 try {
                     final Int64Ref last = new Int64Ref(0);
-                    FileUtils.copy(incomingFd.getFileDescriptor(), targetFd, lengthBytes, null,
-                            Runnable::run, (long progress) -> {
+                    FileUtils.copy(incomingFd.getFileDescriptor(), targetPfd.getFileDescriptor(),
+                            lengthBytes, null, Runnable::run,
+                            (long progress) -> {
                                 if (params.sizeBytes > 0) {
                                     final long delta = progress - last.value;
                                     last.value = progress;
@@ -1015,7 +1032,7 @@
                                 }
                             });
                 } finally {
-                    IoUtils.closeQuietly(targetFd);
+                    IoUtils.closeQuietly(targetPfd);
                     IoUtils.closeQuietly(incomingFd);
 
                     // We're done here, so remove the "bridge" that was holding
@@ -1031,12 +1048,11 @@
                 }
                 return null;
             } else if (PackageInstaller.ENABLE_REVOCABLE_FD) {
-                fd.init(mContext, targetFd);
-                return fd.getRevocableFileDescriptor();
+                return createRevocableFdInternal(fd, targetPfd);
             } else {
-                bridge.setTargetFile(targetFd);
+                bridge.setTargetFile(targetPfd);
                 bridge.start();
-                return new ParcelFileDescriptor(bridge.getClientSocket());
+                return bridge.getClientSocket();
             }
 
         } catch (ErrnoException e) {