Merge "Move RevocableFileDescriptor creation outside the locked section." into rvc-dev

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)

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) {
+        try {
-            return createRevocableFdLocked(fd, callingPackage);
+            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);

apex/blobstore/service/java/com/android/server/blob/BlobStoreSession.java

@@ -59,6 +59,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;
@@ -207,27 +209,37 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
                 throw new IllegalStateException("Not allowed to write in state: "
                         + stateToString(mState));
             }
+        }
 
-            try {
+        FileDescriptor fd = null;
-                return openWriteLocked(offsetBytes, lengthBytes);
+        try {
-            } catch (IOException e) {
+            fd = openWriteInternal(offsetBytes, lengthBytes);
-                throw ExceptionUtils.wrap(e);
+            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) {
@@ -238,10 +250,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
@@ -253,29 +265,40 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
                 throw new IllegalStateException("Not allowed to read in state: "
                         + stateToString(mState));
             }
+        }
 
-            try {
+        FileDescriptor fd = null;
-                return openReadLocked();
+        try {
-            } catch (IOException e) {
+            fd = openReadInternal();
-                throw ExceptionUtils.wrap(e);
+            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 {
+    private FileDescriptor openReadInternal() throws IOException {
-        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_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
@@ -396,7 +419,7 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
             }
 
             mState = state;
-            revokeAllFdsLocked();
+            revokeAllFds();
 
             if (sendCallback) {
                 mListener.onStateChanged(this);
@@ -433,20 +456,17 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
         }
     }
 
-    @GuardedBy("mSessionLock")
+    private void revokeAllFds() {
-    private void revokeAllFdsLocked() {
+        synchronized (mRevocableFds) {
-        for (int i = mRevocableFds.size() - 1; i >= 0; --i) {
+            for (int i = mRevocableFds.size() - 1; i >= 0; --i) {
-            mRevocableFds.get(i).revoke();
+                mRevocableFds.get(i).revoke();
+            }
+            mRevocableFds.clear();
         }
-        mRevocableFds.clear();
     }
 
     @GuardedBy("mSessionLock")
-    @NonNull
+    private void trackRevocableFdLocked(RevocableFileDescriptor revocableFd) {
-    private ParcelFileDescriptor createRevocableFdLocked(FileDescriptor fd)
-            throws IOException {
-        final RevocableFileDescriptor revocableFd =
-                new RevocableFileDescriptor(mContext, fd);
         synchronized (mRevocableFds) {
             mRevocableFds.add(revocableFd);
         }
@@ -455,7 +475,6 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
                 mRevocableFds.remove(revocableFd);
             }
         });
-        return revocableFd.getRevocableFileDescriptor();
     }
 
     @Nullable