Close FDs / mmap handles promptly.
Currently we rely on GC to close them.
This may cause system_server crash depending on the timing of GC.
Bug: 187879195
Test: atest UpdatableSystemFontTest
Change-Id: I09ac3f349e5ec100e4164320cbf27977474cc4bb
diff --git a/graphics/java/android/graphics/fonts/SystemFonts.java b/graphics/java/android/graphics/fonts/SystemFonts.java
index 255f9e6..1d392d2 100644
--- a/graphics/java/android/graphics/fonts/SystemFonts.java
+++ b/graphics/java/android/graphics/fonts/SystemFonts.java
@@ -262,8 +262,14 @@
*/
@VisibleForTesting
public static Map<String, FontFamily[]> buildSystemFallback(FontConfig fontConfig) {
+ return buildSystemFallback(fontConfig, new ArrayMap<>());
+ }
+
+ /** @hide */
+ @VisibleForTesting
+ public static Map<String, FontFamily[]> buildSystemFallback(FontConfig fontConfig,
+ ArrayMap<String, ByteBuffer> outBufferCache) {
final Map<String, FontFamily[]> fallbackMap = new ArrayMap<>();
- final ArrayMap<String, ByteBuffer> bufferCache = new ArrayMap<>();
final List<FontConfig.FontFamily> xmlFamilies = fontConfig.getFontFamilies();
final ArrayMap<String, ArrayList<FontFamily>> fallbackListMap = new ArrayMap<>();
@@ -273,7 +279,7 @@
if (familyName == null) {
continue;
}
- appendNamedFamily(xmlFamily, bufferCache, fallbackListMap);
+ appendNamedFamily(xmlFamily, outBufferCache, fallbackListMap);
}
// Then, add fallback fonts to the each fallback map.
@@ -282,7 +288,7 @@
// The first family (usually the sans-serif family) is always placed immediately
// after the primary family in the fallback.
if (i == 0 || xmlFamily.getName() == null) {
- pushFamilyToFallback(xmlFamily, fallbackListMap, bufferCache);
+ pushFamilyToFallback(xmlFamily, fallbackListMap, outBufferCache);
}
}
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 2654530..dc3dc46 100644
--- a/services/core/java/com/android/server/graphics/fonts/FontManagerService.java
+++ b/services/core/java/com/android/server/graphics/fonts/FontManagerService.java
@@ -25,12 +25,14 @@
import android.graphics.fonts.FontManager;
import android.graphics.fonts.FontUpdateRequest;
import android.graphics.fonts.SystemFonts;
+import android.os.ParcelFileDescriptor;
import android.os.ResultReceiver;
import android.os.SharedMemory;
import android.os.ShellCallback;
import android.system.ErrnoException;
import android.text.FontConfig;
import android.util.AndroidException;
+import android.util.ArrayMap;
import android.util.IndentingPrintWriter;
import android.util.Slog;
@@ -46,6 +48,9 @@
import java.io.FileDescriptor;
import java.io.IOException;
import java.io.PrintWriter;
+import java.nio.ByteBuffer;
+import java.nio.DirectByteBuffer;
+import java.nio.NioUtils;
import java.util.Collections;
import java.util.List;
import java.util.Map;
@@ -76,6 +81,17 @@
} 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);
+ }
+ }
+ }
}
}
@@ -176,13 +192,7 @@
private void initialize() {
synchronized (mUpdatableFontDirLock) {
if (mUpdatableFontDir == null) {
- synchronized (mSerializedFontMapLock) {
- try {
- mSerializedFontMap = Typeface.serializeFontMap(Typeface.getSystemFontMap());
- } catch (IOException | ErrnoException e) {
- mSerializedFontMap = null;
- }
- }
+ setSerializedFontMap(serializeSystemServerFontMap());
return;
}
mUpdatableFontDir.loadFontFileMap();
@@ -278,36 +288,57 @@
/**
* Makes new serialized font map data and updates mSerializedFontMap.
*/
- public void updateSerializedFontMap() {
+ private void updateSerializedFontMap() {
+ SharedMemory serializedFontMap = serializeFontMap(getSystemFontConfig());
+ if (serializedFontMap == null) {
+ // Fallback to the preloaded config.
+ serializedFontMap = serializeSystemServerFontMap();
+ }
+ setSerializedFontMap(serializedFontMap);
+ }
+
+ @Nullable
+ private static SharedMemory serializeFontMap(FontConfig fontConfig) {
+ final ArrayMap<String, ByteBuffer> bufferCache = new ArrayMap<>();
try {
- final FontConfig fontConfig = getSystemFontConfig();
- final Map<String, FontFamily[]> fallback = SystemFonts.buildSystemFallback(fontConfig);
+ final Map<String, FontFamily[]> fallback =
+ SystemFonts.buildSystemFallback(fontConfig, bufferCache);
final Map<String, Typeface> typefaceMap =
SystemFonts.buildSystemTypefaces(fontConfig, fallback);
-
- SharedMemory serializeFontMap = Typeface.serializeFontMap(typefaceMap);
- synchronized (mSerializedFontMapLock) {
- mSerializedFontMap = serializeFontMap;
- }
- return;
+ return Typeface.serializeFontMap(typefaceMap);
} catch (IOException | ErrnoException e) {
Slog.w(TAG, "Failed to serialize updatable font map. "
+ "Retrying with system image fonts.", e);
- }
-
- try {
- final FontConfig fontConfig = SystemFonts.getSystemPreinstalledFontConfig();
- final Map<String, FontFamily[]> fallback = SystemFonts.buildSystemFallback(fontConfig);
- final Map<String, Typeface> typefaceMap =
- SystemFonts.buildSystemTypefaces(fontConfig, fallback);
-
- SharedMemory serializeFontMap = Typeface.serializeFontMap(typefaceMap);
- synchronized (mSerializedFontMapLock) {
- mSerializedFontMap = serializeFontMap;
+ return null;
+ } finally {
+ // Unmap buffers promptly, as we map a lot of files and may hit mmap limit before
+ // GC collects ByteBuffers and unmaps them.
+ for (ByteBuffer buffer : bufferCache.values()) {
+ if (buffer instanceof DirectByteBuffer) {
+ NioUtils.freeDirectBuffer(buffer);
+ }
}
- } catch (IOException | ErrnoException e) {
- Slog.e(TAG, "Failed to serialize SystemServer system font map", e);
}
}
+ @Nullable
+ private static SharedMemory serializeSystemServerFontMap() {
+ try {
+ return Typeface.serializeFontMap(Typeface.getSystemFontMap());
+ } catch (IOException | ErrnoException e) {
+ Slog.e(TAG, "Failed to serialize SystemServer system font map", e);
+ return null;
+ }
+ }
+
+ private void setSerializedFontMap(SharedMemory serializedFontMap) {
+ SharedMemory oldFontMap = null;
+ synchronized (mSerializedFontMapLock) {
+ oldFontMap = mSerializedFontMap;
+ mSerializedFontMap = serializedFontMap;
+ }
+ if (oldFontMap != null) {
+ oldFontMap.close();
+ }
+ }
}
diff --git a/services/core/java/com/android/server/graphics/fonts/OtfFontFileParser.java b/services/core/java/com/android/server/graphics/fonts/OtfFontFileParser.java
index bc6ceec..1ed3972 100644
--- a/services/core/java/com/android/server/graphics/fonts/OtfFontFileParser.java
+++ b/services/core/java/com/android/server/graphics/fonts/OtfFontFileParser.java
@@ -31,6 +31,7 @@
import java.io.FileInputStream;
import java.io.IOException;
import java.nio.ByteBuffer;
+import java.nio.DirectByteBuffer;
import java.nio.NioUtils;
import java.nio.channels.FileChannel;
@@ -41,7 +42,7 @@
try {
return FontFileUtil.getPostScriptName(buffer, 0);
} finally {
- NioUtils.freeDirectBuffer(buffer);
+ unmap(buffer);
}
}
@@ -65,7 +66,7 @@
}
return psName + extension;
} finally {
- NioUtils.freeDirectBuffer(buffer);
+ unmap(buffer);
}
}
@@ -76,37 +77,42 @@
try {
return FontFileUtil.getRevision(buffer, 0);
} finally {
- NioUtils.freeDirectBuffer(buffer);
+ unmap(buffer);
}
}
@Override
public void tryToCreateTypeface(File file) throws Throwable {
- Font font = new Font.Builder(file).build();
- FontFamily family = new FontFamily.Builder(font).build();
- Typeface typeface = new Typeface.CustomFallbackBuilder(family).build();
+ ByteBuffer buffer = mmap(file);
+ try {
+ Font font = new Font.Builder(buffer).build();
+ FontFamily family = new FontFamily.Builder(font).build();
+ Typeface typeface = new Typeface.CustomFallbackBuilder(family).build();
- TextPaint p = new TextPaint();
- p.setTextSize(24f);
- p.setTypeface(typeface);
+ TextPaint p = new TextPaint();
+ p.setTextSize(24f);
+ p.setTypeface(typeface);
- // Test string to try with the passed font.
- // TODO: Good to extract from font file.
- String testTextToDraw = "abcXYZ@- "
- + "\uD83E\uDED6" // Emoji E13.0
- + "\uD83C\uDDFA\uD83C\uDDF8" // Emoji Flags
- + "\uD83D\uDC8F\uD83C\uDFFB" // Emoji Skin tone Sequence
- // ZWJ Sequence
- + "\uD83D\uDC68\uD83C\uDFFC\u200D\u2764\uFE0F\u200D\uD83D\uDC8B\u200D"
- + "\uD83D\uDC68\uD83C\uDFFF";
+ // Test string to try with the passed font.
+ // TODO: Good to extract from font file.
+ String testTextToDraw = "abcXYZ@- "
+ + "\uD83E\uDED6" // Emoji E13.0
+ + "\uD83C\uDDFA\uD83C\uDDF8" // Emoji Flags
+ + "\uD83D\uDC8F\uD83C\uDFFB" // Emoji Skin tone Sequence
+ // ZWJ Sequence
+ + "\uD83D\uDC68\uD83C\uDFFC\u200D\u2764\uFE0F\u200D\uD83D\uDC8B\u200D"
+ + "\uD83D\uDC68\uD83C\uDFFF";
- int width = (int) Math.ceil(Layout.getDesiredWidth(testTextToDraw, p));
- StaticLayout layout = StaticLayout.Builder.obtain(
- testTextToDraw, 0, testTextToDraw.length(), p, width).build();
- Bitmap bmp = Bitmap.createBitmap(
- layout.getWidth(), layout.getHeight(), Bitmap.Config.ALPHA_8);
- Canvas canvas = new Canvas(bmp);
- layout.draw(canvas);
+ int width = (int) Math.ceil(Layout.getDesiredWidth(testTextToDraw, p));
+ StaticLayout layout = StaticLayout.Builder.obtain(
+ testTextToDraw, 0, testTextToDraw.length(), p, width).build();
+ Bitmap bmp = Bitmap.createBitmap(
+ layout.getWidth(), layout.getHeight(), Bitmap.Config.ALPHA_8);
+ Canvas canvas = new Canvas(bmp);
+ layout.draw(canvas);
+ } finally {
+ unmap(buffer);
+ }
}
private static ByteBuffer mmap(File file) throws IOException {
@@ -115,4 +121,10 @@
return fileChannel.map(FileChannel.MapMode.READ_ONLY, 0, fileChannel.size());
}
}
+
+ private static void unmap(ByteBuffer buffer) {
+ if (buffer instanceof DirectByteBuffer) {
+ NioUtils.freeDirectBuffer(buffer);
+ }
+ }
}
diff --git a/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java b/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java
index 898b8d4..44f96c5 100644
--- a/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java
+++ b/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java
@@ -56,6 +56,11 @@
import java.io.OutputStream;
import java.nio.file.Files;
import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.regex.Pattern;
/**
* Tests if fonts can be updated by {@link FontManager} API.
@@ -95,6 +100,12 @@
EMOJI_RENDERING_TEST_APP_ID + "/.EmojiRenderingTestActivity";
private static final long ACTIVITY_TIMEOUT_MILLIS = SECONDS.toMillis(10);
+ private static final Pattern PATTERN_FONT_FILES = Pattern.compile("\\.(ttf|otf|ttc|otc)$");
+ private static final Pattern PATTERN_TMP_FILES = Pattern.compile("^/data/local/tmp/");
+ private static final Pattern PATTERN_DATA_FONT_FILES = Pattern.compile("^/data/fonts/files/");
+ private static final Pattern PATTERN_SYSTEM_FONT_FILES =
+ Pattern.compile("^/(system|product)/fonts/");
+
private String mKeyId;
private FontManager mFontManager;
@@ -236,6 +247,32 @@
assertThat(fontPathAfterReboot).isEqualTo(fontPath);
}
+
+ @Test
+ public void fdLeakTest() throws Exception {
+ long originalOpenFontCount =
+ countMatch(getOpenFiles("system_server"), PATTERN_FONT_FILES);
+ Pattern patternEmojiVPlus1 =
+ Pattern.compile(Pattern.quote(TEST_NOTO_COLOR_EMOJI_VPLUS1_TTF));
+ for (int i = 0; i < 10; i++) {
+ assertThat(updateFontFile(
+ TEST_NOTO_COLOR_EMOJI_VPLUS1_TTF, TEST_NOTO_COLOR_EMOJI_VPLUS1_TTF_FSV_SIG))
+ .isEqualTo(FontManager.RESULT_SUCCESS);
+ List<String> openFiles = getOpenFiles("system_server");
+ for (Pattern p : Arrays.asList(PATTERN_FONT_FILES, PATTERN_SYSTEM_FONT_FILES,
+ PATTERN_DATA_FONT_FILES, PATTERN_TMP_FILES)) {
+ Log.i(TAG, String.format("num of %s: %d", p, countMatch(openFiles, p)));
+ }
+ // system_server should not keep /data/fonts files open.
+ assertThat(countMatch(openFiles, PATTERN_DATA_FONT_FILES)).isEqualTo(0);
+ // system_server should not keep passed FD open.
+ assertThat(countMatch(openFiles, patternEmojiVPlus1)).isEqualTo(0);
+ // The number of open font FD should not increase.
+ assertThat(countMatch(openFiles, PATTERN_FONT_FILES))
+ .isAtMost(originalOpenFontCount);
+ }
+ }
+
private static String insertCert(String certPath) throws Exception {
Pair<String, String> result;
try (InputStream is = new FileInputStream(certPath)) {
@@ -338,7 +375,37 @@
return !expectCommandToSucceed(cmd).trim().isEmpty();
}
+ private static List<String> getOpenFiles(String appId) throws Exception {
+ String pid = pidOf(appId);
+ if (pid.isEmpty()) {
+ return Collections.emptyList();
+ }
+ String cmd = String.format("lsof -p %s", pid);
+ String out = expectCommandToSucceed(cmd);
+ List<String> paths = new ArrayList<>();
+ boolean first = true;
+ for (String line : out.split("\n")) {
+ // Skip the header.
+ if (first) {
+ first = false;
+ continue;
+ }
+ String[] records = line.split(" ");
+ if (records.length > 0) {
+ paths.add(records[records.length - 1]);
+ }
+ }
+ return paths;
+ }
+
private static String pidOf(String appId) throws Exception {
return expectCommandToSucceed("pidof " + appId).trim();
}
+
+ private static long countMatch(List<String> paths, Pattern pattern) {
+ // Note: asPredicate() returns true for partial matching.
+ return paths.stream()
+ .filter(pattern.asPredicate())
+ .count();
+ }
}