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

Commit e3a3105c authored by Hai Zhang's avatar Hai Zhang
Browse files

Further refactor UidPermissionState and GIDs. 

The GIDs returned by the original permission state implementation in R
actually was never unique, but simply all GIDs from granted permission
concatenated together, and PERMISSION_OPERATION_SUCCESS_GIDS_CHANGED
was returned when the length of the GIDs changed. This is equivalent
to simply checking whether the permission whose grant state changed
has GIDs or not, and can greatly simplify the
logic. PERMISSION_OPERATION_SUCCESS_GIDS_CHANGED was only used in two
places anyway.

The original permission actually would never return
PERMISSION_OPERATION_FAILURE as well because it checks hasPermission()
beforehand and returns PERMISSION_OPERATION_SUCCESS if there's nothing
to change in grant/revokePermission(). The name
PERMISSION_OPERATION_FAILURE isn't a great name for unchanged anyway,
so grant/revokePermission() is now changed to simply return a boolean
for whether the permission state is changed.

The cache for isPermissionReviewRequired() is removed because it's
broken in subtle cases and iterating over an ArrayMap isn't a terrible
trade-off anyway, in exchange for simpler code and correct behavior.

Made removePermissionState() public so that code that actually wants
to erase the state permission doesn't need to perform a revocation
followed by updating all flags to 0.

Non-null arrays are preferred in APIs, the same as non-null
collections, so the GIDs-related APIs are updated to return non-null
int arrays as well. EmptyArray.INT is used instead of null so there
shouldn't be any performance penalty. Also ensured that the APIs are
returning copies instead of the original array to guard against
accidental mutation.

Bug: 158736025
Test: presubmit
Change-Id: I606210e18e5f8f87b8f8408fe476a72c2b7ed1c1
parent f579dd52

core/java/com/android/server/SystemConfig.java

+2 −2
Original line number Diff line number Diff line
@@ -46,6 +46,7 @@ import com.android.internal.annotations.VisibleForTesting; 
import com.android.internal.util.XmlUtils;



import libcore.io.IoUtils;

import libcore.util.EmptyArray;



import org.xmlpull.v1.XmlPullParser;

import org.xmlpull.v1.XmlPullParserException;
@@ -55,7 +56,6 @@ import java.io.File; 
import java.io.FileNotFoundException;

import java.io.FileReader;

import java.io.IOException;

import java.util.Arrays;

import java.util.ArrayList;

import java.util.Collections;

import java.util.List;
@@ -95,7 +95,7 @@ public class SystemConfig { 
    private static final String VENDOR_SKU_PROPERTY = "ro.boot.product.vendor.sku";



    // Group-ids that are given to all packages as read from etc/permissions/*.xml.

    int[] mGlobalGids;

    int[] mGlobalGids = EmptyArray.INT;



    // These are the built-in uid -> permission mappings that were read from the

    // system configuration files.

services/core/java/com/android/server/pm/permission/BasePermission.java

+12 −7
Original line number Diff line number Diff line
@@ -36,13 +36,14 @@ import android.os.UserHandle; 
import android.util.Log;

import android.util.Slog;



import com.android.internal.util.ArrayUtils;

import com.android.server.pm.DumpState;

import com.android.server.pm.PackageManagerService;

import com.android.server.pm.PackageSettingBase;

import com.android.server.pm.parsing.PackageInfoUtils;

import com.android.server.pm.parsing.pkg.AndroidPackage;



import libcore.util.EmptyArray;



import org.xmlpull.v1.XmlPullParser;

import org.xmlpull.v1.XmlSerializer;
@@ -95,7 +96,8 @@ public final class BasePermission { 
    int uid;



    /** Additional GIDs given to apps granted this permission */

    private int[] gids;

    @NonNull

    private int[] gids = EmptyArray.INT;



    /**

     * Flag indicating that {@link #gids} should be adjusted based on the
@@ -132,7 +134,7 @@ public final class BasePermission { 
    public int getUid() {

        return uid;

    }

    public void setGids(int[] gids, boolean perUser) {

    public void setGids(@NonNull int[] gids, boolean perUser) {

        this.gids = gids;

        this.perUser = perUser;

    }
@@ -141,18 +143,20 @@ public final class BasePermission { 
    }



    public boolean hasGids() {

        return !ArrayUtils.isEmpty(gids);

        return gids.length != 0;

    }



    @NonNull

    public int[] computeGids(int userId) {

        if (perUser) {

            final int[] userGids = new int[gids.length];

            for (int i = 0; i < gids.length; i++) {

                userGids[i] = UserHandle.getUid(userId, gids[i]);

                final int gid = gids[i];

                userGids[i] = UserHandle.getUid(userId, gid);

            }

            return userGids;

        } else {

            return gids;

            return gids.length != 0 ? gids.clone() : gids;

        }

    }
@@ -286,7 +290,8 @@ public final class BasePermission { 
            pendingPermissionInfo.packageName = newPackageName;

        }

        uid = 0;

        setGids(null, false);

        gids = EmptyArray.INT;

        perUser = false;

    }



    public boolean addToTree(@ProtectionLevel int protectionLevel,

services/core/java/com/android/server/pm/permission/PermissionManagerService.java

+35 −73
Original line number Diff line number Diff line
@@ -53,7 +53,6 @@ import static com.android.server.pm.PackageManagerService.DEBUG_PACKAGE_SCANNING 
import static com.android.server.pm.PackageManagerService.DEBUG_PERMISSIONS;

import static com.android.server.pm.PackageManagerService.DEBUG_REMOVE;

import static com.android.server.pm.PackageManagerService.PLATFORM_PACKAGE_NAME;

import static com.android.server.pm.permission.UidPermissionState.PERMISSION_OPERATION_FAILURE;



import static java.util.concurrent.TimeUnit.SECONDS;
@@ -152,6 +151,8 @@ import com.android.server.pm.permission.PermissionManagerServiceInternal.Permiss 
import com.android.server.policy.PermissionPolicyInternal;

import com.android.server.policy.SoftRestrictedPermissionPolicy;



import libcore.util.EmptyArray;



import java.io.FileDescriptor;

import java.io.PrintWriter;

import java.lang.annotation.Retention;
@@ -245,6 +246,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
    private final SparseArray<ArraySet<String>> mSystemPermissions;



    /** Built-in group IDs given to all packages. Read from system configuration files. */

    @NonNull

    private final int[] mGlobalGids;



    private final HandlerThread mHandlerThread;
@@ -1501,7 +1503,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
            // normal runtime permissions.  For now they apply to all users.

            // TODO(zhanghai): We are breaking the behavior above by making all permission state

            //  per-user. It isn't documented behavior and relatively rarely used anyway.

            if (uidState.grantPermission(bp) != PERMISSION_OPERATION_FAILURE) {

            if (uidState.grantPermission(bp)) {

                if (callback != null) {

                    callback.onInstallPermissionGranted();

                }
@@ -1519,19 +1521,15 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
            return;

        }



        final int result = uidState.grantPermission(bp);

        switch (result) {

            case PERMISSION_OPERATION_FAILURE: {

        if (!uidState.grantPermission(bp)) {

            return;

        }



            case UidPermissionState.PERMISSION_OPERATION_SUCCESS_GIDS_CHANGED: {

        if (bp.hasGids()) {

            if (callback != null) {

                callback.onGidsChanged(UserHandle.getAppId(pkg.getUid()), userId);

            }

        }

            break;

        }



        if (bp.isRuntime()) {

            logPermission(MetricsEvent.ACTION_PERMISSION_GRANTED, permName, packageName);
@@ -1650,7 +1648,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
            // normal runtime permissions.  For now they apply to all users.

            // TODO(zhanghai): We are breaking the behavior above by making all permission state

            //  per-user. It isn't documented behavior and relatively rarely used anyway.

            if (uidState.revokePermission(bp) != PERMISSION_OPERATION_FAILURE) {

            if (uidState.revokePermission(bp)) {

                if (callback != null) {

                    mDefaultPermissionCallback.onInstallPermissionRevoked();

                }
@@ -1658,12 +1656,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
            return;

        }



        // Permission is already revoked, no need to do anything.

        if (!uidState.isPermissionGranted(permName)) {

            return;

        }



        if (uidState.revokePermission(bp) == PERMISSION_OPERATION_FAILURE) {

        if (!uidState.revokePermission(bp)) {

            return;

        }
@@ -2504,11 +2497,11 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
        }

    }



    @Nullable

    @NonNull

    private int[] getPermissionGids(@NonNull String permissionName, @UserIdInt int userId) {

        BasePermission permission = mSettings.getPermission(permissionName);

        if (permission == null) {

            return null;

            return EmptyArray.INT;

        }

        return permission.computeGids(userId);

    }
@@ -2629,8 +2622,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
                }

            }



            uidState.setGlobalGids(mGlobalGids);



            ArraySet<String> newImplicitPermissions = new ArraySet<>();

            final String friendlyName = pkg.getPackageName() + "(" + pkg.getUid() + ")";
@@ -2765,7 +2756,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
                        switch (grant) {

                            case GRANT_INSTALL: {

                                // Grant an install permission.

                                if (uidState.grantPermission(bp) != PERMISSION_OPERATION_FAILURE) {

                                if (uidState.grantPermission(bp)) {

                                    changedInstallPermission = true;

                                }

                            } break;
@@ -2797,8 +2788,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
                                    if (permissionPolicyInitialized && hardRestricted) {

                                        if (!restrictionExempt) {

                                            if (origPermState != null && origPermState.isGranted()

                                                    && uidState.revokePermission(

                                                    bp) != PERMISSION_OPERATION_FAILURE) {

                                                    && uidState.revokePermission(bp)) {

                                                wasChanged = true;

                                            }

                                            if (!restrictionApplied) {
@@ -2830,8 +2820,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
                                            || (!hardRestricted || restrictionExempt)) {

                                        if ((origPermState != null && origPermState.isGranted())

                                                || upgradedActivityRecognitionPermission != null) {

                                            if (uidState.grantPermission(bp)

                                                    == PERMISSION_OPERATION_FAILURE) {

                                            if (!uidState.grantPermission(bp)) {

                                                wasChanged = true;

                                            }

                                        }
@@ -2850,8 +2839,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
                                    }



                                    if (!uidState.isPermissionGranted(bp.name)

                                            && uidState.grantPermission(bp)

                                                    != PERMISSION_OPERATION_FAILURE) {

                                            && uidState.grantPermission(bp)) {

                                        wasChanged = true;

                                    }
@@ -2899,13 +2887,11 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
                            } break;

                        }

                    } else {

                        if (uidState.revokePermission(bp) != PERMISSION_OPERATION_FAILURE) {

                            // Also drop the permission flags.

                            uidState.updatePermissionFlags(bp,

                                    MASK_PERMISSION_FLAGS_ALL, 0);

                            changedInstallPermission = true;

                        if (DEBUG_PERMISSIONS) {

                                Slog.i(TAG, "Un-granting permission " + perm

                            boolean wasGranted = uidState.isPermissionGranted(bp.name);

                            if (wasGranted || bp.isAppOp()) {

                                Slog.i(TAG, (wasGranted ? "Un-granting" : "Not granting")

                                        + " permission " + perm

                                        + " from package " + friendlyName

                                        + " (protectionLevel=" + bp.getProtectionLevel()

                                        + " flags=0x"
@@ -2913,20 +2899,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
                                                ps))

                                        + ")");

                            }

                        } else if (bp.isAppOp()) {

                            // Don't print warning for app op permissions, since it is fine for them

                            // not to be granted, there is a UI for the user to decide.

                            if (DEBUG_PERMISSIONS

                                    && (packageOfInterest == null

                                            || packageOfInterest.equals(pkg.getPackageName()))) {

                                Slog.i(TAG, "Not granting permission " + perm

                                        + " to package " + friendlyName

                                        + " (protectionLevel=" + bp.getProtectionLevel()

                                        + " flags=0x"

                                        + Integer.toHexString(PackageInfoUtils.appInfoFlags(pkg,

                                                ps))

                                        + ")");

                        }

                        if (uidState.removePermissionState(bp.name)) {

                            changedInstallPermission = true;

                        }

                    }

                }
@@ -3005,8 +2980,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 


                        if ((flags & BLOCKING_PERMISSION_FLAGS) == 0

                                && supportsRuntimePermissions) {

                            int revokeResult = ps.revokePermission(bp);

                            if (revokeResult != PERMISSION_OPERATION_FAILURE) {

                            if (ps.revokePermission(bp)) {

                                if (DEBUG_PERMISSIONS) {

                                    Slog.i(TAG, "Revoking runtime permission "

                                            + permission + " for " + pkgName
@@ -3865,14 +3839,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
                }

            }



            // The package is gone - no need to keep flags for applying policy.

            uidState.updatePermissionFlags(bp, PackageManager.MASK_PERMISSION_FLAGS_ALL, 0);



            // Try to revoke as a runtime permission which is per user.

            // TODO(zhanghai): This doesn't make sense. revokePermission() doesn't fail, and why are

            //  we only killing the uid when gids changed, instead of any permission change?

            if (uidState.revokePermission(bp)

                    == UidPermissionState.PERMISSION_OPERATION_SUCCESS_GIDS_CHANGED) {

            // TODO(zhanghai): Why are we only killing the UID when GIDs changed, instead of any

            //  permission change?

            if (uidState.removePermissionState(bp.name) && bp.hasGids()) {

                affectedUserId = userId;

            }

        }
@@ -3905,17 +3874,14 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
        boolean runtimePermissionChanged = false;



        // Prune permissions

        final List<com.android.server.pm.permission.PermissionState> permissionStates =

                uidState.getPermissionStates();

        final List<PermissionState> permissionStates = uidState.getPermissionStates();

        final int permissionStatesSize = permissionStates.size();

        for (int i = permissionStatesSize - 1; i >= 0; i--) {

            PermissionState permissionState = permissionStates.get(i);

            if (!usedPermissions.contains(permissionState.getName())) {

                BasePermission bp = mSettings.getPermissionLocked(permissionState.getName());

                if (bp != null) {

                    uidState.revokePermission(bp);

                    uidState.updatePermissionFlags(bp, MASK_PERMISSION_FLAGS_ALL, 0);

                    if (permissionState.isRuntime()) {

                    if (uidState.removePermissionState(bp.name) && permissionState.isRuntime()) {

                        runtimePermissionChanged = true;

                    }

                }
@@ -4178,11 +4144,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
                                            + p.getPackageName() + " and user " + userId);

                                    return;

                                }

                                if (uidState.getPermissionState(bp.getName()) != null) {

                                    uidState.revokePermission(bp);

                                    uidState.updatePermissionFlags(bp, MASK_PERMISSION_FLAGS_ALL,

                                            0);

                                }

                                uidState.removePermissionState(bp.name);

                            }

                        });

                    }
@@ -4741,7 +4703,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
            Slog.e(TAG, "Missing permissions state for app ID " + appId + " and user ID " + userId);

            return EMPTY_INT_ARRAY;

        }

        return uidState.computeGids(userId);

        return uidState.computeGids(mGlobalGids, userId);

    }



    private class PermissionManagerServiceInternalImpl extends PermissionManagerServiceInternal {
@@ -4804,7 +4766,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { 
                @UserIdInt int userId) {

            return PermissionManagerService.this.getGrantedPermissions(packageName, userId);

        }

        @Nullable

        @NonNull

        @Override

        public int[] getPermissionGids(@NonNull String permissionName, @UserIdInt int userId) {

            return PermissionManagerService.this.getPermissionGids(permissionName, userId);

services/core/java/com/android/server/pm/permission/PermissionState.java

+1 −2
Original line number Diff line number Diff line
@@ -17,7 +17,6 @@ 
package com.android.server.pm.permission;



import android.annotation.NonNull;

import android.annotation.Nullable;

import android.annotation.UserIdInt;



import com.android.internal.annotations.GuardedBy;
@@ -62,7 +61,7 @@ public final class PermissionState { 
        return mPermission.getName();

    }



    @Nullable

    @NonNull

    public int[] computeGids(@UserIdInt int userId) {

        return mPermission.computeGids(userId);

    }

services/core/java/com/android/server/pm/permission/UidPermissionState.java

+98 −205

File changed.

Preview size limit exceeded, changes collapsed.