From 7c47347a17a153ba1593fbd2157b9f93ae4625a5 Mon Sep 17 00:00:00 2001 From: Siim Sammul Date: Fri, 8 Apr 2022 15:58:37 +0100 Subject: [PATCH] Add rate limiting to addTombstoneToDropbox. Support for attaching a header for the number of dropped entries for tombstones in proto format will come in a later change. Use the .pb tombstone file for both plaintext and proto tombstones as they both should always exist and the proto one is easier to parse. The plaintext file needs to be kept around for gmscore needs. Bug: 225173288 Test: atest ErrorsTest Change-Id: If6434e4c309d010624a15d7b478bc645c326e8ef --- .../java/com/android/server/BootReceiver.java | 19 +++++++- .../server/am/ActivityManagerService.java | 3 +- .../android/server/am/DropboxRateLimiter.java | 9 +++- .../server/os/NativeTombstoneManager.java | 45 ++++++++++++------- 4 files changed, 53 insertions(+), 23 deletions(-) diff --git a/services/core/java/com/android/server/BootReceiver.java b/services/core/java/com/android/server/BootReceiver.java index aac1035059eca..7f2d542393666 100644 --- a/services/core/java/com/android/server/BootReceiver.java +++ b/services/core/java/com/android/server/BootReceiver.java @@ -46,6 +46,7 @@ import com.android.internal.annotations.VisibleForTesting; import com.android.internal.logging.MetricsLogger; import com.android.internal.util.FrameworkStatsLog; import com.android.internal.util.XmlUtils; +import com.android.server.am.DropboxRateLimiter; import org.xmlpull.v1.XmlPullParser; import org.xmlpull.v1.XmlPullParserException; @@ -308,19 +309,31 @@ public class BootReceiver extends BroadcastReceiver { writeTimestamps(timestamps); } + private static final DropboxRateLimiter sDropboxRateLimiter = new DropboxRateLimiter(); + /** * Add a tombstone to the DropBox. * * @param ctx Context * @param tombstone path to the tombstone + * @param proto whether the tombstone is stored as proto + * @param processName the name of the process corresponding to the tombstone */ - public static void addTombstoneToDropBox(Context ctx, File tombstone, boolean proto) { + public static void addTombstoneToDropBox( + Context ctx, File tombstone, boolean proto, String processName) { final DropBoxManager db = ctx.getSystemService(DropBoxManager.class); if (db == null) { Slog.e(TAG, "Can't log tombstone: DropBoxManager not available"); return; } + // Check if we should rate limit and abort early if needed. Do this for both proto and + // non-proto tombstones, even though proto tombstones do not support including the counter + // of events dropped since rate limiting activated yet. + DropboxRateLimiter.RateLimitResult rateLimitResult = + sDropboxRateLimiter.shouldRateLimit(TAG_TOMBSTONE, processName); + if (rateLimitResult.shouldRateLimit()) return; + HashMap timestamps = readTimestamps(); try { if (proto) { @@ -328,7 +341,9 @@ public class BootReceiver extends BroadcastReceiver { db.addFile(TAG_TOMBSTONE_PROTO, tombstone, 0); } } else { - final String headers = getBootHeadersToLogAndUpdate(); + // Add the header indicating how many events have been dropped due to rate limiting. + final String headers = getBootHeadersToLogAndUpdate() + + rateLimitResult.createHeader(); addFileToDropBox(db, timestamps, headers, tombstone.getPath(), LOG_SIZE, TAG_TOMBSTONE); } diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 163df101620dc..0a034a02a6fce 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -8817,8 +8817,7 @@ public class ActivityManagerService extends IActivityManager.Stub millisSinceOldestPendingRead).append("\n"); } } - sb.append("Dropped-Count: ").append( - rateLimitResult.droppedCountSinceRateLimitActivated()).append("\n"); + sb.append(rateLimitResult.createHeader()); sb.append("\n"); // Do the rest in a worker thread to avoid blocking the caller on I/O diff --git a/services/core/java/com/android/server/am/DropboxRateLimiter.java b/services/core/java/com/android/server/am/DropboxRateLimiter.java index 08a6719a016a1..baf062d284577 100644 --- a/services/core/java/com/android/server/am/DropboxRateLimiter.java +++ b/services/core/java/com/android/server/am/DropboxRateLimiter.java @@ -111,8 +111,8 @@ public class DropboxRateLimiter { /** Holds information on whether we should rate limit and how many events have been dropped. */ public class RateLimitResult { - boolean mShouldRateLimit; - int mDroppedCountSinceRateLimitActivated; + final boolean mShouldRateLimit; + final int mDroppedCountSinceRateLimitActivated; public RateLimitResult(boolean shouldRateLimit, int droppedCountSinceRateLimitActivated) { mShouldRateLimit = shouldRateLimit; @@ -128,6 +128,11 @@ public class DropboxRateLimiter { public int droppedCountSinceRateLimitActivated() { return mDroppedCountSinceRateLimitActivated; } + + /** Returns a header indicating the number of dropped events. */ + public String createHeader() { + return "Dropped-Count: " + mDroppedCountSinceRateLimitActivated + "\n"; + } } private class ErrorRecord { diff --git a/services/core/java/com/android/server/os/NativeTombstoneManager.java b/services/core/java/com/android/server/os/NativeTombstoneManager.java index 3fc416931a06f..45192b7bcdbe6 100644 --- a/services/core/java/com/android/server/os/NativeTombstoneManager.java +++ b/services/core/java/com/android/server/os/NativeTombstoneManager.java @@ -113,19 +113,22 @@ public final class NativeTombstoneManager { return; } - if (filename.endsWith(".pb")) { - handleProtoTombstone(path); - BootReceiver.addTombstoneToDropBox(mContext, path, true); - } else { - BootReceiver.addTombstoneToDropBox(mContext, path, false); + String processName = "UNKNOWN"; + final boolean isProtoFile = filename.endsWith(".pb"); + File protoPath = isProtoFile ? path : new File(path.getAbsolutePath() + ".pb"); + + Optional parsedTombstone = handleProtoTombstone(protoPath, isProtoFile); + if (parsedTombstone.isPresent()) { + processName = parsedTombstone.get().getProcessName(); } + BootReceiver.addTombstoneToDropBox(mContext, path, isProtoFile, processName); } - private void handleProtoTombstone(File path) { + private Optional handleProtoTombstone(File path, boolean addToList) { final String filename = path.getName(); if (!filename.endsWith(".pb")) { Slog.w(TAG, "unexpected tombstone name: " + path); - return; + return Optional.empty(); } final String suffix = filename.substring("tombstone_".length()); @@ -136,11 +139,11 @@ public final class NativeTombstoneManager { number = Integer.parseInt(numberStr); if (number < 0 || number > 99) { Slog.w(TAG, "unexpected tombstone name: " + path); - return; + return Optional.empty(); } } catch (NumberFormatException ex) { Slog.w(TAG, "unexpected tombstone name: " + path); - return; + return Optional.empty(); } ParcelFileDescriptor pfd; @@ -148,23 +151,27 @@ public final class NativeTombstoneManager { pfd = ParcelFileDescriptor.open(path, MODE_READ_WRITE); } catch (FileNotFoundException ex) { Slog.w(TAG, "failed to open " + path, ex); - return; + return Optional.empty(); } final Optional parsedTombstone = TombstoneFile.parse(pfd); if (!parsedTombstone.isPresent()) { IoUtils.closeQuietly(pfd); - return; + return Optional.empty(); } - synchronized (mLock) { - TombstoneFile previous = mTombstones.get(number); - if (previous != null) { - previous.dispose(); + if (addToList) { + synchronized (mLock) { + TombstoneFile previous = mTombstones.get(number); + if (previous != null) { + previous.dispose(); + } + + mTombstones.put(number, parsedTombstone.get()); } - - mTombstones.put(number, parsedTombstone.get()); } + + return parsedTombstone; } /** @@ -363,6 +370,10 @@ public final class NativeTombstoneManager { return true; } + public String getProcessName() { + return mProcessName; + } + public void dispose() { IoUtils.closeQuietly(mPfd); }