Move RevocableFileDescriptor creation outside the locked section.

If creating the RevocableFileDescriptor while holding the lock gets
stuck, it could crash the system. So, take it out of of the locked
section. Also, close the underlying fd if RevocableFileDescriptor
creation fails.

Bug: 157535024
Test: atest --test-mapping apex/blobstore
Change-Id: I9948307f7e5cdaa1ac886531cf6e67f5a7dd0547

3 files changed, 60 insertions, 37 deletions

diff --git a/apex/blobstore/service/java/com/android/server/blob/BlobAccessMode.java b/apex/blobstore/service/java/com/android/server/blob/BlobAccessMode.java
index ec7ba287acec..96bb5ef4e45e 100644
--- a/apex/blobstore/service/java/com/android/server/blob/BlobAccessMode.java
+++ b/apex/blobstore/service/java/com/android/server/blob/BlobAccessMode.java
@@ -44,7 +44,7 @@ import java.util.Objects;
 /**
  * Class for representing how a blob can be shared.
  *
- * Note that this class is not thread-safe, callers need to take of synchronizing access.
+ * Note that this class is not thread-safe, callers need to take care of synchronizing access.
  */
 class BlobAccessMode {
     @Retention(RetentionPolicy.SOURCE)
diff --git a/apex/blobstore/service/java/com/android/server/blob/BlobMetadata.java b/apex/blobstore/service/java/com/android/server/blob/BlobMetadata.java
index cea7fcca6e20..03573d966cd0 100644
--- a/apex/blobstore/service/java/com/android/server/blob/BlobMetadata.java
+++ b/apex/blobstore/service/java/com/android/server/blob/BlobMetadata.java
@@ -61,6 +61,8 @@ import com.android.internal.util.IndentingPrintWriter;
 import com.android.internal.util.XmlUtils;
 import com.android.server.blob.BlobStoreManagerService.DumpArgs;
 
+import libcore.io.IoUtils;
+
 import org.xmlpull.v1.XmlPullParser;
 import org.xmlpull.v1.XmlPullParserException;
 import org.xmlpull.v1.XmlSerializer;
@@ -349,14 +351,16 @@ class BlobMetadata {
         } catch (ErrnoException e) {
             throw e.rethrowAsIOException();
         }
-        synchronized (mMetadataLock) {
-            return createRevocableFdLocked(fd, callingPackage);
+        try {
+            return createRevocableFd(fd, callingPackage);
+        } catch (IOException e) {
+            IoUtils.closeQuietly(fd);
+            throw e;
         }
     }
 
-    @GuardedBy("mMetadataLock")
     @NonNull
-    private ParcelFileDescriptor createRevocableFdLocked(FileDescriptor fd,
+    private ParcelFileDescriptor createRevocableFd(FileDescriptor fd,
             String callingPackage) throws IOException {
         final RevocableFileDescriptor revocableFd =
                 new RevocableFileDescriptor(mContext, fd);
diff --git a/apex/blobstore/service/java/com/android/server/blob/BlobStoreSession.java b/apex/blobstore/service/java/com/android/server/blob/BlobStoreSession.java
index 62701e52781d..7dc1bd089afb 100644
--- a/apex/blobstore/service/java/com/android/server/blob/BlobStoreSession.java
+++ b/apex/blobstore/service/java/com/android/server/blob/BlobStoreSession.java
@@ -57,6 +57,8 @@ import com.android.internal.util.XmlUtils;
 import com.android.server.blob.BlobStoreManagerService.DumpArgs;
 import com.android.server.blob.BlobStoreManagerService.SessionStateChangeListener;
 
+import libcore.io.IoUtils;
+
 import org.xmlpull.v1.XmlPullParser;
 import org.xmlpull.v1.XmlPullParserException;
 import org.xmlpull.v1.XmlSerializer;
@@ -190,27 +192,37 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
                 throw new IllegalStateException("Not allowed to write in state: "
                         + stateToString(mState));
             }
+        }
 
-            try {
-                return openWriteLocked(offsetBytes, lengthBytes);
-            } catch (IOException e) {
-                throw ExceptionUtils.wrap(e);
+        FileDescriptor fd = null;
+        try {
+            fd = openWriteInternal(offsetBytes, lengthBytes);
+            final RevocableFileDescriptor revocableFd = new RevocableFileDescriptor(mContext, fd);
+            synchronized (mSessionLock) {
+                if (mState != STATE_OPENED) {
+                    IoUtils.closeQuietly(fd);
+                    throw new IllegalStateException("Not allowed to write in state: "
+                            + stateToString(mState));
+                }
+                trackRevocableFdLocked(revocableFd);
+                return revocableFd.getRevocableFileDescriptor();
             }
+        } catch (IOException e) {
+            IoUtils.closeQuietly(fd);
+            throw ExceptionUtils.wrap(e);
         }
     }
 
-    @GuardedBy("mSessionLock")
     @NonNull
-    private ParcelFileDescriptor openWriteLocked(@BytesLong long offsetBytes,
+    private FileDescriptor openWriteInternal(@BytesLong long offsetBytes,
             @BytesLong long lengthBytes) throws IOException {
         // TODO: Add limit on active open sessions/writes/reads
-        FileDescriptor fd = null;
         try {
             final File sessionFile = getSessionFile();
             if (sessionFile == null) {
                 throw new IllegalStateException("Couldn't get the file for this session");
             }
-            fd = Os.open(sessionFile.getPath(), O_CREAT | O_RDWR, 0600);
+            final FileDescriptor fd = Os.open(sessionFile.getPath(), O_CREAT | O_RDWR, 0600);
             if (offsetBytes > 0) {
                 final long curOffset = Os.lseek(fd, offsetBytes, SEEK_SET);
                 if (curOffset != offsetBytes) {
@@ -221,10 +233,10 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
             if (lengthBytes > 0) {
                 mContext.getSystemService(StorageManager.class).allocateBytes(fd, lengthBytes);
             }
+            return fd;
         } catch (ErrnoException e) {
-            e.rethrowAsIOException();
+            throw e.rethrowAsIOException();
         }
-        return createRevocableFdLocked(fd);
     }
 
     @Override
@@ -236,29 +248,40 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
                 throw new IllegalStateException("Not allowed to read in state: "
                         + stateToString(mState));
             }
+        }
 
-            try {
-                return openReadLocked();
-            } catch (IOException e) {
-                throw ExceptionUtils.wrap(e);
+        FileDescriptor fd = null;
+        try {
+            fd = openReadInternal();
+            final RevocableFileDescriptor revocableFd = new RevocableFileDescriptor(mContext, fd);
+            synchronized (mSessionLock) {
+                if (mState != STATE_OPENED) {
+                    IoUtils.closeQuietly(fd);
+                    throw new IllegalStateException("Not allowed to read in state: "
+                            + stateToString(mState));
+                }
+                trackRevocableFdLocked(revocableFd);
+                return revocableFd.getRevocableFileDescriptor();
             }
+
+        } catch (IOException e) {
+            IoUtils.closeQuietly(fd);
+            throw ExceptionUtils.wrap(e);
         }
     }
 
-    @GuardedBy("mSessionLock")
     @NonNull
-    private ParcelFileDescriptor openReadLocked() throws IOException {
-        FileDescriptor fd = null;
+    private FileDescriptor openReadInternal() throws IOException {
         try {
             final File sessionFile = getSessionFile();
             if (sessionFile == null) {
                 throw new IllegalStateException("Couldn't get the file for this session");
             }
-            fd = Os.open(sessionFile.getPath(), O_RDONLY, 0);
+            final FileDescriptor fd = Os.open(sessionFile.getPath(), O_RDONLY, 0);
+            return fd;
         } catch (ErrnoException e) {
-            e.rethrowAsIOException();
+            throw e.rethrowAsIOException();
         }
-        return createRevocableFdLocked(fd);
     }
 
     @Override
@@ -379,7 +402,7 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
             }
 
             mState = state;
-            revokeAllFdsLocked();
+            revokeAllFds();
 
             if (sendCallback) {
                 mListener.onStateChanged(this);
@@ -415,20 +438,17 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
         }
     }
 
-    @GuardedBy("mSessionLock")
-    private void revokeAllFdsLocked() {
-        for (int i = mRevocableFds.size() - 1; i >= 0; --i) {
-            mRevocableFds.get(i).revoke();
+    private void revokeAllFds() {
+        synchronized (mRevocableFds) {
+            for (int i = mRevocableFds.size() - 1; i >= 0; --i) {
+                mRevocableFds.get(i).revoke();
+            }
+            mRevocableFds.clear();
         }
-        mRevocableFds.clear();
     }
 
     @GuardedBy("mSessionLock")
-    @NonNull
-    private ParcelFileDescriptor createRevocableFdLocked(FileDescriptor fd)
-            throws IOException {
-        final RevocableFileDescriptor revocableFd =
-                new RevocableFileDescriptor(mContext, fd);
+    private void trackRevocableFdLocked(RevocableFileDescriptor revocableFd) {
         synchronized (mRevocableFds) {
             mRevocableFds.add(revocableFd);
         }
@@ -437,7 +457,6 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
                 mRevocableFds.remove(revocableFd);
             }
         });
-        return revocableFd.getRevocableFileDescriptor();
     }
 
     @Nullable