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

Commit 0c39dd02 authored by Alex Buynytskyy's avatar Alex Buynytskyy
Browse files

Create XML parser only once.

This greatly cuts memory usage: 2.5M -> 0.5M (see bug for traces).

Bug: 200995209
Test: atest SystemConfigTest SystemConfigNamedActorTest
Change-Id: I0b76dc2610afaad3e418ef3115c5e54a05ab334e
parent 66879294
Loading
Loading
Loading
Loading
+26 −23
Original line number Diff line number Diff line
@@ -488,12 +488,14 @@ public class SystemConfig {
    }

    private void readAllPermissions() {
        final XmlPullParser parser = Xml.newPullParser();

        // Read configuration from system
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getRootDirectory(), "etc", "sysconfig"), ALLOW_ALL);

        // Read configuration from the old permissions dir
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getRootDirectory(), "etc", "permissions"), ALLOW_ALL);

        // Vendors are only allowed to customize these
@@ -503,18 +505,18 @@ public class SystemConfig {
            // For backward compatibility
            vendorPermissionFlag |= (ALLOW_PERMISSIONS | ALLOW_APP_CONFIGS);
        }
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getVendorDirectory(), "etc", "sysconfig"), vendorPermissionFlag);
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getVendorDirectory(), "etc", "permissions"), vendorPermissionFlag);

        String vendorSkuProperty = SystemProperties.get(VENDOR_SKU_PROPERTY, "");
        if (!vendorSkuProperty.isEmpty()) {
            String vendorSkuDir = "sku_" + vendorSkuProperty;
            readPermissions(Environment.buildPath(
            readPermissions(parser, Environment.buildPath(
                    Environment.getVendorDirectory(), "etc", "sysconfig", vendorSkuDir),
                    vendorPermissionFlag);
            readPermissions(Environment.buildPath(
            readPermissions(parser, Environment.buildPath(
                    Environment.getVendorDirectory(), "etc", "permissions", vendorSkuDir),
                    vendorPermissionFlag);
        }
@@ -522,18 +524,18 @@ public class SystemConfig {
        // Allow ODM to customize system configs as much as Vendor, because /odm is another
        // vendor partition other than /vendor.
        int odmPermissionFlag = vendorPermissionFlag;
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getOdmDirectory(), "etc", "sysconfig"), odmPermissionFlag);
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getOdmDirectory(), "etc", "permissions"), odmPermissionFlag);

        String skuProperty = SystemProperties.get(SKU_PROPERTY, "");
        if (!skuProperty.isEmpty()) {
            String skuDir = "sku_" + skuProperty;

            readPermissions(Environment.buildPath(
            readPermissions(parser, Environment.buildPath(
                    Environment.getOdmDirectory(), "etc", "sysconfig", skuDir), odmPermissionFlag);
            readPermissions(Environment.buildPath(
            readPermissions(parser, Environment.buildPath(
                    Environment.getOdmDirectory(), "etc", "permissions", skuDir),
                    odmPermissionFlag);
        }
@@ -541,9 +543,9 @@ public class SystemConfig {
        // Allow OEM to customize these
        int oemPermissionFlag = ALLOW_FEATURES | ALLOW_OEM_PERMISSIONS | ALLOW_ASSOCIATIONS
                | ALLOW_VENDOR_APEX;
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getOemDirectory(), "etc", "sysconfig"), oemPermissionFlag);
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getOemDirectory(), "etc", "permissions"), oemPermissionFlag);

        // Allow Product to customize these configs
@@ -558,15 +560,15 @@ public class SystemConfig {
            // DEVICE_INITIAL_SDK_INT for the devices without product interface enforcement.
            productPermissionFlag = ALLOW_ALL;
        }
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getProductDirectory(), "etc", "sysconfig"), productPermissionFlag);
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getProductDirectory(), "etc", "permissions"), productPermissionFlag);

        // Allow /system_ext to customize all system configs
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getSystemExtDirectory(), "etc", "sysconfig"), ALLOW_ALL);
        readPermissions(Environment.buildPath(
        readPermissions(parser, Environment.buildPath(
                Environment.getSystemExtDirectory(), "etc", "permissions"), ALLOW_ALL);

        // Skip loading configuration from apex if it is not a system process.
@@ -580,12 +582,13 @@ public class SystemConfig {
            if (f.isFile() || f.getPath().contains("@")) {
                continue;
            }
            readPermissions(Environment.buildPath(f, "etc", "permissions"), apexPermissionFlag);
            readPermissions(parser, Environment.buildPath(f, "etc", "permissions"),
                    apexPermissionFlag);
        }
    }

    @VisibleForTesting
    public void readPermissions(File libraryDir, int permissionFlag) {
    public void readPermissions(final XmlPullParser parser, File libraryDir, int permissionFlag) {
        // Read permissions from given directory.
        if (!libraryDir.exists() || !libraryDir.isDirectory()) {
            if (permissionFlag == ALLOW_ALL) {
@@ -620,12 +623,12 @@ public class SystemConfig {
                continue;
            }

            readPermissionsFromXml(f, permissionFlag);
            readPermissionsFromXml(parser, f, permissionFlag);
        }

        // Read platform permissions last so it will take precedence
        if (platformFile != null) {
            readPermissionsFromXml(platformFile, permissionFlag);
            readPermissionsFromXml(parser, platformFile, permissionFlag);
        }
    }

@@ -634,8 +637,9 @@ public class SystemConfig {
                + permFile + " at " + parser.getPositionDescription());
    }

    private void readPermissionsFromXml(File permFile, int permissionFlag) {
        FileReader permReader = null;
    private void readPermissionsFromXml(final XmlPullParser parser, File permFile,
            int permissionFlag) {
        final FileReader permReader;
        try {
            permReader = new FileReader(permFile);
        } catch (FileNotFoundException e) {
@@ -647,7 +651,6 @@ public class SystemConfig {
        final boolean lowRam = ActivityManager.isLowRamDeviceStatic();

        try {
            XmlPullParser parser = Xml.newPullParser();
            parser.setInput(permReader);

            int type;
+3 −1
Original line number Diff line number Diff line
@@ -17,6 +17,7 @@
package com.android.server.systemconfig

import android.content.Context
import android.util.Xml
import androidx.test.InstrumentationRegistry
import com.android.server.SystemConfig
import com.google.common.truth.Truth.assertThat
@@ -227,6 +228,7 @@ class SystemConfigNamedActorTest {
            .writeText(this.trimIndent())

    private fun assertPermissions() = SystemConfig(false).apply {
        readPermissions(tempFolder.root, 0)
        val parser = Xml.newPullParser()
        readPermissions(parser, tempFolder.root, 0)
    }. let { assertThat(it.namedActors) }
}
+17 −10
Original line number Diff line number Diff line
@@ -25,6 +25,7 @@ import android.platform.test.annotations.Presubmit;
import android.util.ArrayMap;
import android.util.ArraySet;
import android.util.Log;
import android.util.Xml;

import androidx.test.filters.SmallTest;
import androidx.test.runner.AndroidJUnit4;
@@ -36,6 +37,7 @@ import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;
import org.xmlpull.v1.XmlPullParser;

import java.io.BufferedWriter;
import java.io.File;
@@ -76,6 +78,11 @@ public class SystemConfigTest {
        }
    }

    private void readPermissions(File libraryDir, int permissionFlag) {
        final XmlPullParser parser = Xml.newPullParser();
        mSysConfig.readPermissions(parser, libraryDir, permissionFlag);
    }

    /**
     * Tests that readPermissions works correctly for the tag: install-in-user-type
     */
@@ -134,8 +141,8 @@ public class SystemConfigTest {
        // Also, make a third file, but with the name folder1/permFile2.xml, to prove no conflicts.
        createTempFile(folder1, "permFile2.xml", contents3);

        mSysConfig.readPermissions(folder1, /* No permission needed anyway */ 0);
        mSysConfig.readPermissions(folder2, /* No permission needed anyway */ 0);
        readPermissions(folder1, /* No permission needed anyway */ 0);
        readPermissions(folder2, /* No permission needed anyway */ 0);

        Map<String, Set<String>> actualWhite = mSysConfig.getAndClearPackageToUserTypeWhitelist();
        Map<String, Set<String>> actualBlack = mSysConfig.getAndClearPackageToUserTypeBlacklist();
@@ -165,7 +172,7 @@ public class SystemConfigTest {
        final File folder = createTempSubfolder("folder");
        createTempFile(folder, "component-override.xml", contents);

        mSysConfig.readPermissions(folder, /* No permission needed anyway */ 0);
        readPermissions(folder, /* No permission needed anyway */ 0);

        final ArrayMap<String, Boolean> packageOneExpected = new ArrayMap<>();
        packageOneExpected.put("com.android.package1.Full", true);
@@ -197,7 +204,7 @@ public class SystemConfigTest {
        final File folder = createTempSubfolder("folder");
        createTempFile(folder, "staged-installer-whitelist.xml", contents);

        mSysConfig.readPermissions(folder, /* Grant all permission flags */ ~0);
        readPermissions(folder, /* Grant all permission flags */ ~0);

        assertThat(mSysConfig.getWhitelistedStagedInstallers())
                .containsExactly("com.android.package1");
@@ -215,7 +222,7 @@ public class SystemConfigTest {
        final File folder = createTempSubfolder("folder");
        createTempFile(folder, "staged-installer-whitelist.xml", contents);

        mSysConfig.readPermissions(folder, /* Grant all permission flags */ ~0);
        readPermissions(folder, /* Grant all permission flags */ ~0);

        assertThat(mSysConfig.getWhitelistedStagedInstallers())
                .containsExactly("com.android.package1");
@@ -238,7 +245,7 @@ public class SystemConfigTest {

        IllegalStateException e = expectThrows(
                IllegalStateException.class,
                () -> mSysConfig.readPermissions(folder, /* Grant all permission flags */ ~0));
                () -> readPermissions(folder, /* Grant all permission flags */ ~0));

        assertThat(e).hasMessageThat().contains("Multiple modules installers");
    }
@@ -257,7 +264,7 @@ public class SystemConfigTest {
        final File folder = createTempSubfolder("folder");
        createTempFile(folder, "staged-installer-whitelist.xml", contents);

        mSysConfig.readPermissions(folder, /* Grant all but ALLOW_APP_CONFIGS flag */ ~0x08);
        readPermissions(folder, /* Grant all but ALLOW_APP_CONFIGS flag */ ~0x08);

        assertThat(mSysConfig.getWhitelistedStagedInstallers()).isEmpty();
    }
@@ -277,7 +284,7 @@ public class SystemConfigTest {
        final File folder = createTempSubfolder("folder");
        createTempFile(folder, "vendor-apex-allowlist.xml", contents);

        mSysConfig.readPermissions(folder, /* Grant all permission flags */ ~0);
        readPermissions(folder, /* Grant all permission flags */ ~0);

        assertThat(mSysConfig.getAllowedVendorApexes())
                .containsExactly("com.android.apex1", "com.installer");
@@ -297,7 +304,7 @@ public class SystemConfigTest {
        final File folder = createTempSubfolder("folder");
        createTempFile(folder, "vendor-apex-allowlist.xml", contents);

        mSysConfig.readPermissions(folder, /* Grant all permission flags */ ~0);
        readPermissions(folder, /* Grant all permission flags */ ~0);

        assertThat(mSysConfig.getAllowedVendorApexes()).isEmpty();
    }
@@ -317,7 +324,7 @@ public class SystemConfigTest {
        final File folder = createTempSubfolder("folder");
        createTempFile(folder, "vendor-apex-allowlist.xml", contents);

        mSysConfig.readPermissions(folder, /* Grant all but ALLOW_VENDOR_APEX flag */ ~0x400);
        readPermissions(folder, /* Grant all but ALLOW_VENDOR_APEX flag */ ~0x400);

        assertThat(mSysConfig.getAllowedVendorApexes()).isEmpty();
    }