Merge "Close FDs even when throwing SecurityException." into sc-dev

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,8 +71,10 @@ public final class FontManagerService extends IFontManager.Stub {
         return getSystemFontConfig();
     }
 
+    @RequiresPermission(Manifest.permission.UPDATE_FONTS)
     @Override
     public int updateFontFamily(@NonNull List<FontUpdateRequest> requests, int baseVersion) {
+        try {
             Preconditions.checkArgumentNonnegative(baseVersion);
             Objects.requireNonNull(requests);
             getContext().enforceCallingPermission(Manifest.permission.UPDATE_FONTS,
@@ -81,10 +85,20 @@ public final class FontManagerService extends IFontManager.Stub {
             } catch (SystemFontException e) {
                 Slog.e(TAG, "Failed to update font family", e);
                 return e.getErrorCode();
+            }
         } finally {
+            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) {
+            if (fd == null) continue;
             try {
                 fd.close();
             } catch (IOException e) {
@@ -92,8 +106,6 @@ public final class FontManagerService extends IFontManager.Stub {
             }
         }
     }
-        }
-    }
 
     /* package */ static class SystemFontException extends AndroidException {
         private final int mErrorCode;

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,14 +304,19 @@ public class UpdatableSystemFontTest {
         try (ParcelFileDescriptor fd =
                 ParcelFileDescriptor.open(new File(fontPath), MODE_READ_ONLY)) {
             return SystemUtil.runWithShellPermissionIdentity(() -> {
-                FontConfig fontConfig = mFontManager.getFontConfig();
+                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(),
-                        fontConfig.getConfigVersion());
+                configVersion);
-            });
-        }
     }
 
     private String getFontPath(String psName) {