Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 048d4b17 authored by Kohsuke Yatoh's avatar Kohsuke Yatoh
Browse files

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
parent 77e80b86
Loading
Loading
Loading
Loading
+9 −3
Original line number Diff line number Diff line
@@ -262,8 +262,14 @@ public final class SystemFonts {
     */
    @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 @@ public final class SystemFonts {
            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 @@ public final class SystemFonts {
            // 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);
            }
        }

+56 −25
Original line number Diff line number Diff line
@@ -25,12 +25,14 @@ import android.graphics.fonts.FontFamily;
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.File;
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 @@ public final class FontManagerService extends IFontManager.Stub {
        } 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 @@ public final class FontManagerService extends IFontManager.Stub {
    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 @@ public final class FontManagerService extends IFontManager.Stub {
    /**
     * 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);
            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);
                }
            }
        }
    }

    @Nullable
    private static SharedMemory serializeSystemServerFontMap() {
        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 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();
        }
    }
}
+37 −25
Original line number Diff line number Diff line
@@ -31,6 +31,7 @@ import java.io.File;
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 @@ import java.nio.channels.FileChannel;
        try {
            return FontFileUtil.getPostScriptName(buffer, 0);
        } finally {
            NioUtils.freeDirectBuffer(buffer);
            unmap(buffer);
        }
    }

@@ -65,7 +66,7 @@ import java.nio.channels.FileChannel;
            }
            return psName + extension;
        } finally {
            NioUtils.freeDirectBuffer(buffer);
            unmap(buffer);
        }

    }
@@ -76,13 +77,15 @@ import java.nio.channels.FileChannel;
        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();
        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();

@@ -107,6 +110,9 @@ import java.nio.channels.FileChannel;
                    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 @@ import java.nio.channels.FileChannel;
            return fileChannel.map(FileChannel.MapMode.READ_ONLY, 0, fileChannel.size());
        }
    }

    private static void unmap(ByteBuffer buffer) {
        if (buffer instanceof DirectByteBuffer) {
            NioUtils.freeDirectBuffer(buffer);
        }
    }
}
+67 −0
Original line number Diff line number Diff line
@@ -56,6 +56,11 @@ import java.io.InputStream;
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 @@ public class UpdatableSystemFontTest {
            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 @@ public class UpdatableSystemFontTest {
        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 @@ public class UpdatableSystemFontTest {
        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();
    }
}