Close FDs even when throwing SecurityException.

Bug: 187879195
Test: atest UpdatableSystemFontTest
Change-Id: I34d04f152210fe2f89f4d96e920978dde21d06c8

2 files changed, 56 insertions, 25 deletions

diff --git a/services/core/java/com/android/server/graphics/fonts/FontManagerService.java b/services/core/java/com/android/server/graphics/fonts/FontManagerService.java
index dc3dc468f763..ee799d502163 100644
--- a/services/core/java/com/android/server/graphics/fonts/FontManagerService.java
+++ b/services/core/java/com/android/server/graphics/fonts/FontManagerService.java
@@ -19,6 +19,7 @@ package com.android.server.graphics.fonts;
 import android.Manifest;
 import android.annotation.NonNull;
 import android.annotation.Nullable;
+import android.annotation.RequiresPermission;
 import android.content.Context;
 import android.graphics.Typeface;
 import android.graphics.fonts.FontFamily;
@@ -62,6 +63,7 @@ public final class FontManagerService extends IFontManager.Stub {
 
     private static final String FONT_FILES_DIR = "/data/fonts/files";
 
+    @RequiresPermission(Manifest.permission.UPDATE_FONTS)
     @Override
     public FontConfig getFontConfig() {
         getContext().enforceCallingPermission(Manifest.permission.UPDATE_FONTS,
@@ -69,28 +71,38 @@ public final class FontManagerService extends IFontManager.Stub {
         return getSystemFontConfig();
     }
 
+    @RequiresPermission(Manifest.permission.UPDATE_FONTS)
     @Override
     public int updateFontFamily(@NonNull List<FontUpdateRequest> requests, int baseVersion) {
-        Preconditions.checkArgumentNonnegative(baseVersion);
-        Objects.requireNonNull(requests);
-        getContext().enforceCallingPermission(Manifest.permission.UPDATE_FONTS,
-                "UPDATE_FONTS permission required.");
         try {
-            update(baseVersion, requests);
-            return FontManager.RESULT_SUCCESS;
-        } catch (SystemFontException e) {
-            Slog.e(TAG, "Failed to update font family", e);
-            return e.getErrorCode();
+            Preconditions.checkArgumentNonnegative(baseVersion);
+            Objects.requireNonNull(requests);
+            getContext().enforceCallingPermission(Manifest.permission.UPDATE_FONTS,
+                    "UPDATE_FONTS permission required.");
+            try {
+                update(baseVersion, requests);
+                return FontManager.RESULT_SUCCESS;
+            } catch (SystemFontException e) {
+                Slog.e(TAG, "Failed to update font family", e);
+                return e.getErrorCode();
+            }
         } finally {
-            for (FontUpdateRequest request : requests) {
-                ParcelFileDescriptor fd = request.getFd();
-                if (fd != null) {
-                    try {
-                        fd.close();
-                    } catch (IOException e) {
-                        Slog.w(TAG, "Failed to close fd", e);
-                    }
-                }
+            closeFileDescriptors(requests);
+        }
+    }
+
+    private static void closeFileDescriptors(@Nullable List<FontUpdateRequest> requests) {
+        // Make sure we close every passed FD, even if 'requests' is constructed incorrectly and
+        // some fields are null.
+        if (requests == null) return;
+        for (FontUpdateRequest request : requests) {
+            if (request == null) continue;
+            ParcelFileDescriptor fd = request.getFd();
+            if (fd == null) continue;
+            try {
+                fd.close();
+            } catch (IOException e) {
+                Slog.w(TAG, "Failed to close fd", e);
             }
         }
     }
diff --git a/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java b/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java
index 44f96c5a987b..80b0dfeb8804 100644
--- a/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java
+++ b/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java
@@ -20,6 +20,7 @@ import static android.os.ParcelFileDescriptor.MODE_READ_ONLY;
 
 import static com.google.common.truth.Truth.assertThat;
 
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assume.assumeTrue;
 
 import static java.util.concurrent.TimeUnit.SECONDS;
@@ -247,7 +248,6 @@ public class UpdatableSystemFontTest {
         assertThat(fontPathAfterReboot).isEqualTo(fontPath);
     }
 
-
     @Test
     public void fdLeakTest() throws Exception {
         long originalOpenFontCount =
@@ -273,6 +273,20 @@ public class UpdatableSystemFontTest {
         }
     }
 
+    @Test
+    public void fdLeakTest_withoutPermission() throws Exception {
+        Pattern patternEmojiVPlus1 =
+                Pattern.compile(Pattern.quote(TEST_NOTO_COLOR_EMOJI_VPLUS1_TTF));
+        byte[] signature = Files.readAllBytes(Paths.get(TEST_NOTO_COLOR_EMOJI_VPLUS1_TTF_FSV_SIG));
+        try (ParcelFileDescriptor fd = ParcelFileDescriptor.open(
+                new File(TEST_NOTO_COLOR_EMOJI_VPLUS1_TTF), MODE_READ_ONLY)) {
+            assertThrows(SecurityException.class,
+                    () -> updateFontFileWithoutPermission(fd, signature, 0));
+        }
+        List<String> openFiles = getOpenFiles("system_server");
+        assertThat(countMatch(openFiles, patternEmojiVPlus1)).isEqualTo(0);
+    }
+
     private static String insertCert(String certPath) throws Exception {
         Pair<String, String> result;
         try (InputStream is = new FileInputStream(certPath)) {
@@ -290,16 +304,21 @@ public class UpdatableSystemFontTest {
         try (ParcelFileDescriptor fd =
                 ParcelFileDescriptor.open(new File(fontPath), MODE_READ_ONLY)) {
             return SystemUtil.runWithShellPermissionIdentity(() -> {
-                FontConfig fontConfig = mFontManager.getFontConfig();
-                return mFontManager.updateFontFamily(
-                        new FontFamilyUpdateRequest.Builder()
-                                .addFontFileUpdateRequest(new FontFileUpdateRequest(fd, signature))
-                                .build(),
-                        fontConfig.getConfigVersion());
+                int configVersion = mFontManager.getFontConfig().getConfigVersion();
+                return updateFontFileWithoutPermission(fd, signature, configVersion);
             });
         }
     }
 
+    private int updateFontFileWithoutPermission(ParcelFileDescriptor fd, byte[] signature,
+            int configVersion) {
+        return mFontManager.updateFontFamily(
+                new FontFamilyUpdateRequest.Builder()
+                        .addFontFileUpdateRequest(new FontFileUpdateRequest(fd, signature))
+                        .build(),
+                configVersion);
+    }
+
     private String getFontPath(String psName) {
         return SystemUtil.runWithShellPermissionIdentity(() -> {
             FontConfig fontConfig = mFontManager.getFontConfig();