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

Commit acd97fa3 authored by Robin Lee's avatar Robin Lee
Browse files

Remove infinite money glitch from ProtoLog service 

Clients had a register() method but no unregister(), only a
binderDied(). It also registers many duplicates if init() is called
repeatedly. This has some weaknesses:

- The implementation of linkToDeath creates a strong self-reference that
  is never deleted, preventing the client object from geting cleaned up
  until the binder fully dies.

- Lack of unregister means that every call to init() leaks a new
  BinderProxy, eventually overwhelming the limits in long-running test
  suites that setup proto logging thousands of times in one process.

- The existing cleanup method fails to delete the entry from the client
  hashmap, which once again leaks the client object in BinderProxy form.

- Use of IBinder.Stub as primary keys is not safe because multiple
  objects can represent the same Binder. Switched to using the IBinder
  as primary key since this is a unique reference in the same process.

Removing the infinite money glitch (also known in gaming circles as
a binder leak, which is one of the common ways this is exploited by our
players) makes us more robust against cheating by WmShellUnitTests and
other bad actors.

Test: atest --iterations 5 WMShellUnitTests
Bug: 419430883
Flag: android.tracing.client_side_proto_logging
Change-Id: I151ac2714b180cc4411ec6be260503c8e5e45ea9
parent 5a136f7b

core/java/com/android/internal/protolog/IProtoLogConfigurationService.aidl

+3 −1
Original line number Diff line number Diff line
@@ -48,4 +48,6 @@ interface IProtoLogConfigurationService { 
    }



    oneway void registerClient(IProtoLogClient client, in RegisterClientArgs args);



    oneway void unregisterClient(IProtoLogClient  client);

}

core/java/com/android/internal/protolog/PerfettoProtoLogImpl.java

+8 −0
Original line number Diff line number Diff line
@@ -210,6 +210,14 @@ public abstract class PerfettoProtoLogImpl extends IProtoLogClient.Stub implemen 
        mDataSource.unregisterOnStartCallback(this::onTracingInstanceStart);

        mDataSource.unregisterOnFlushCallback(this::onTracingFlush);

        mDataSource.unregisterOnStopCallback(this::onTracingInstanceStop);



        if (android.tracing.Flags.clientSideProtoLogging()) {

            try {

                mConfigurationService.unregisterClient(this);

            } catch (RemoteException e) {

                throw new RuntimeException("Failed to unregister ProtoLog client", e);

            }

        }

    }



    @NonNull

core/java/com/android/internal/protolog/ProtoLogConfigurationServiceImpl.java

+120 −60
Original line number Diff line number Diff line
@@ -31,9 +31,11 @@ import android.annotation.NonNull; 
import android.annotation.Nullable;

import android.annotation.SystemService;

import android.content.Context;

import android.os.IBinder;

import android.os.RemoteException;

import android.os.ResultReceiver;

import android.os.ShellCallback;

import android.util.ArraySet;

import android.util.Log;

import android.util.proto.ProtoInputStream;

import android.util.proto.ProtoOutputStream;
@@ -45,6 +47,7 @@ import java.io.FileInputStream; 
import java.io.FileNotFoundException;

import java.io.IOException;

import java.io.PrintWriter;

import java.util.Arrays;

import java.util.HashMap;

import java.util.HashSet;

import java.util.Map;
@@ -68,7 +71,7 @@ import java.util.TreeMap; 
 */

@SystemService(Context.PROTOLOG_CONFIGURATION_SERVICE)

public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationService.Stub

        implements ProtoLogConfigurationService {

        implements ProtoLogConfigurationService, IBinder.DeathRecipient {

    private static final String LOG_TAG = "ProtoLogConfigurationService";



    private final ProtoLogDataSource mDataSource;
@@ -79,20 +82,40 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ 
     * need to be dumped on flush.

     */

    private final Map<String, Integer> mConfigFileCounts = new HashMap<>();



    /**

     * Keeps track of the viewer config file of each client if available.

     * Container for data about a {@link IProtoLogClient} that needs to get cleaned up when the

     * client goes away.

     */

    private final Map<IProtoLogClient, String> mClientConfigFiles = new HashMap<>();

    private static final class ClientRecord {

        /** Immutable Binder.Stub for communication with the client. */

        @NonNull

        public final IProtoLogClient client;



        /** Immutable name of the viewer config file of each client if available. */

        @Nullable

        public final String configFile;



        /** Mutable set of ProtoLog groups registered for this client to actively trace. */

        @NonNull

        public final Set<String> groups = new ArraySet<>();



        public ClientRecord(@NonNull IProtoLogClient client, @Nullable String configFile) {

            this.client = client;

            this.configFile = configFile;

        }

    }



    /**

     * Keeps track of all the clients that are actively tracing.

     */

    private final Map<IBinder, ClientRecord> mClientRecords = new HashMap<>();



    /**

     * Keeps track of all the protolog groups that have been registered by clients and are still

     * being actively traced.

     */

    private final Set<String> mRegisteredGroups = new HashSet<>();

    /**

     * Keeps track of all the clients that are actively tracing a given protolog group.

     */

    private final Map<String, Set<IProtoLogClient>> mGroupToClients = new HashMap<>();



    /**

     * Keeps track of whether or not a given group should be logged to logcat.
@@ -151,14 +174,52 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ 
    @Override

    public void registerClient(@NonNull IProtoLogClient client, @NonNull RegisterClientArgs args)

            throws RemoteException {

        client.asBinder().linkToDeath(() -> onClientBinderDeath(client), /* flags */ 0);

        final IBinder clientBinder = client.asBinder();



        final String viewerConfigFile = args.viewerConfigFile;

        mClientRecords.put(clientBinder, new ClientRecord(client, viewerConfigFile));



        if (viewerConfigFile != null) {

            registerViewerConfigFile(client, viewerConfigFile);

            mConfigFileCounts.put(viewerConfigFile,

                    mConfigFileCounts.getOrDefault(viewerConfigFile, 0) + 1);

        }



        registerGroups(client, args.groups, args.groupsDefaultLogcatStatus);



        clientBinder.linkToDeath(this, /* flags= */ 0);

    }



    /**

     * Unregister the {@param client}.

     *

     * TODO: this lacks synchronization.

     */

    @Override

    public void unregisterClient(@Nullable IProtoLogClient client) {

        if (client == null) {

            return;

        }



        final IBinder clientBinder = client.asBinder();

        if (clientBinder != null) {

            clientBinder.unlinkToDeath(this, /* flags= */ 0);

        }



        // Retrieve the client record for cleanup.

        final ClientRecord clientRecord = mClientRecords.remove(clientBinder);

        if (clientRecord == null) {

            return;

        }



        if (clientRecord.configFile != null) {

            final var newCount = mConfigFileCounts.get(clientRecord.configFile) - 1;

            mConfigFileCounts.put(clientRecord.configFile, newCount);



            // Dump the tracing config now if no other client is going to dump the same config file.

            if (newCount == 0) {

                mViewerConfigFileTracer.trace(mDataSource, clientRecord.configFile);

            }

        }

    }



    @Override
@@ -214,11 +275,21 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ 
        return isLoggingToLogcat;

    }



    private void registerViewerConfigFile(

            @NonNull IProtoLogClient client, @NonNull String viewerConfigFile) {

        final var count = mConfigFileCounts.getOrDefault(viewerConfigFile, 0);

        mConfigFileCounts.put(viewerConfigFile, count + 1);

        mClientConfigFiles.put(client, viewerConfigFile);

    /**

     * Legacy method (no longer called) inherited from {@link IBinder.DeathRecipient}.

     *

     * Because the method is non-default, it has to be implemented, but the newer version taking an

     * IBinder will always be called instead.

     */

    public void binderDied() {

    }



    /**

     * Unregister client when its owner dies - inherited from {@link IBinder.DeathRecipient}

     */

    @Override

    public void binderDied(@NonNull IBinder clientBinder) {

        unregisterClient(IProtoLogClient.Stub.asInterface(clientBinder));

    }



    private void registerGroups(@NonNull IProtoLogClient client, @NonNull String[] groups,
@@ -230,14 +301,17 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ 
                        + " and logcatStatuses has length " + logcatStatuses.length);

        }



        final var clientRecord = mClientRecords.get(client.asBinder());

        if (clientRecord == null) {

            throw new RuntimeException("Client " + client + " is not registered");

        }



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

            String group = groups[i];

            boolean logcatStatus = logcatStatuses[i];



            mRegisteredGroups.add(group);



            mGroupToClients.putIfAbsent(group, new HashSet<>());

            mGroupToClients.get(group).add(client);

            clientRecord.groups.add(group);



            if (!mLogGroupToLogcatStatus.containsKey(group)) {

                mLogGroupToLogcatStatus.put(group, logcatStatus);
@@ -253,33 +327,18 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ 
    private void toggleProtoLogToLogcat(

            @NonNull PrintWriter pw, boolean enabled, @NonNull String[] groups

    ) {

        final var clientToGroups = new HashMap<IProtoLogClient, Set<String>>();



        for (String group : groups) {

            final var clients = mGroupToClients.get(group);



            if (clients == null) {

                // No clients associated to this group

                var warning = "Attempting to toggle log to logcat for group " + group

                        + " with no registered clients. This is a no-op.";

                Log.w(LOG_TAG, warning);

                pw.println("WARNING: " + warning);

                continue;

            }

        // For each client, if its groups intersect the given list, send the command to toggle.

        for (var clientRecord : mClientRecords.values()) {

            final var affectedGroups = new ArraySet<>(clientRecord.groups);

            affectedGroups.retainAll(Arrays.asList(groups));



            for (IProtoLogClient client : clients) {

                clientToGroups.putIfAbsent(client, new HashSet<>());

                clientToGroups.get(client).add(group);

            }

        }



        for (IProtoLogClient client : clientToGroups.keySet()) {

            if (!affectedGroups.isEmpty()) {

                final var clientGroups = affectedGroups.toArray(new String[0]);

                try {

                final var clientGroups = clientToGroups.get(client).toArray(new String[0]);

                pw.println("Toggling logcat logging for client " + client.toString()

                    pw.println("Toggling logcat logging for client " + clientRecord.client

                            + " to " + enabled + " for groups: ["

                            + String.join(", ", clientGroups) + "]");

                client.toggleLogcat(enabled, clientGroups);

                    clientRecord.client.toggleLogcat(enabled, clientGroups);

                    pw.println("- Done");

                } catch (RemoteException e) {

                    pw.println("- Failed");
@@ -287,7 +346,21 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ 
                            "Failed to toggle logcat status for groups on client", e);

                }

            }

        }



        // Groups that actually have no clients associated indicate some kind of a bug.

        Set<String> noOpGroups = new ArraySet<>(groups);

        mClientRecords.forEach((k, v) -> noOpGroups.removeAll(v.groups));



        // Send out a warning in logcat and the PrintWriter for unrecognized groups.

        for (String group : noOpGroups) {

            var warning = "Attempting to toggle log to logcat for group " + group

                    + " with no registered clients. This is a no-op.";

            Log.w(LOG_TAG, warning);

            pw.println("WARNING: " + warning);

        }



        // Flip the status of the groups in our record-keeping.

        for (String group : groups) {

            mLogGroupToLogcatStatus.put(group, enabled);

        }
@@ -319,19 +392,6 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ 
        });

    }



    private void onClientBinderDeath(@NonNull IProtoLogClient client) {

        // Dump the tracing config now if no other client is going to dump the same config file.

        String configFile = mClientConfigFiles.get(client);

        if (configFile != null) {

            final var newCount = mConfigFileCounts.get(configFile) - 1;

            mConfigFileCounts.put(configFile, newCount);

            boolean lastProcessWithViewerConfig = newCount == 0;

            if (lastProcessWithViewerConfig) {

                mViewerConfigFileTracer.trace(mDataSource, configFile);

            }

        }

    }



    private static void writeViewerConfigGroup(

            @NonNull ProtoInputStream pis, @NonNull ProtoOutputStream os) throws IOException {

        final long inGroupToken = pis.start(GROUPS);