From 820f9bc656896fe7c5dbe5a7bb8b5967063468d2 Mon Sep 17 00:00:00 2001 From: Hunter Knepshield Date: Wed, 5 Feb 2020 20:10:53 -0800 Subject: Update contents of a telephony bug report. Much of the current info is not suitable to share with carriers from devices running user builds, so we strip much of it out. We also show a progress notification for connectivity reports now to satisfy system health requirements for a user-visible indicator. Bug: 146521742 Test: take bug report on user build Test: atest dumpstate_test Change-Id: I21182e75dd85936c979e8b983aa03cb3b02420b7 --- cmds/dumpstate/dumpstate.cpp | 107 +++++++++++++++++++++++--------- cmds/dumpstate/tests/dumpstate_test.cpp | 2 +- 2 files changed, 80 insertions(+), 29 deletions(-) diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 4d98520e01..7cfd4d14b5 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -890,6 +890,14 @@ static void DoSystemLogcat(time_t since) { CommandOptions::WithTimeoutInMs(timeout_ms).Build()); } +static void DoRadioLogcat() { + unsigned long timeout_ms = logcat_timeout({"radio"}); + RunCommand( + "RADIO LOG", + {"logcat", "-b", "radio", "-v", "threadtime", "-v", "printable", "-v", "uid", "-d", "*:v"}, + CommandOptions::WithTimeoutInMs(timeout_ms).Build(), true /* verbose_duration */); +} + static void DoLogcat() { unsigned long timeout_ms; // DumpFile("EVENT LOG TAGS", "/etc/event-log-tags"); @@ -908,11 +916,7 @@ static void DoLogcat() { "STATS LOG", {"logcat", "-b", "stats", "-v", "threadtime", "-v", "printable", "-v", "uid", "-d", "*:v"}, CommandOptions::WithTimeoutInMs(timeout_ms).Build(), true /* verbose_duration */); - timeout_ms = logcat_timeout({"radio"}); - RunCommand( - "RADIO LOG", - {"logcat", "-b", "radio", "-v", "threadtime", "-v", "printable", "-v", "uid", "-d", "*:v"}, - CommandOptions::WithTimeoutInMs(timeout_ms).Build(), true /* verbose_duration */); + DoRadioLogcat(); RunCommand("LOG STATISTICS", {"logcat", "-b", "all", "-S"}); @@ -1599,8 +1603,10 @@ static Dumpstate::RunStatus DumpstateDefault() { return status; } -// This method collects common dumpsys for telephony and wifi -static void DumpstateRadioCommon() { +// This method collects common dumpsys for telephony and wifi. Typically, wifi +// reports are fine to include all information, but telephony reports on user +// builds need to strip some content (see DumpstateTelephonyOnly). +static void DumpstateRadioCommon(bool include_sensitive_info = true) { DumpIpTablesAsRoot(); ds.AddDir(LOGPERSIST_DATA_DIR, false); @@ -1609,26 +1615,51 @@ static void DumpstateRadioCommon() { return; } - do_dmesg(); - DoLogcat(); + // We need to be picky about some stuff for telephony reports on user builds. + if (!include_sensitive_info) { + // Only dump the radio log buffer (other buffers and dumps contain too much unrelated info). + DoRadioLogcat(); + } else { + // Contains various system properties and process startup info. + do_dmesg(); + // Logs other than the radio buffer may contain package/component names and potential PII. + DoLogcat(); + // Too broad for connectivity problems. + DoKmsg(); + // Contains unrelated hardware info (camera, NFC, biometrics, ...). + DumpHals(); + } + DumpPacketStats(); - DoKmsg(); DumpIpAddrAndRules(); dump_route_tables(); - DumpHals(); - RunDumpsys("NETWORK DIAGNOSTICS", {"connectivity", "--diag"}, CommandOptions::WithTimeout(10).Build()); } -// This method collects dumpsys for telephony debugging only +// We use "telephony" here for legacy reasons, though this now really means "connectivity" (cellular +// + wifi + networking). This method collects dumpsys for connectivity debugging only. General rules +// for what can be included on user builds: all reported information MUST directly relate to +// connectivity debugging or customer support and MUST NOT contain unrelated personally identifiable +// information. This information MUST NOT identify user-installed packages (UIDs are OK, package +// names are not), and MUST NOT contain logs of user application traffic. +// TODO(b/148168577) rename this and other related fields/methods to "connectivity" instead. static void DumpstateTelephonyOnly() { DurationReporter duration_reporter("DUMPSTATE"); + const CommandOptions DUMPSYS_COMPONENTS_OPTIONS = CommandOptions::WithTimeout(60).Build(); - DumpstateRadioCommon(); + const bool include_sensitive_info = !PropertiesHelper::IsUserBuild(); - RunCommand("SYSTEM PROPERTIES", {"getprop"}); + DumpstateRadioCommon(include_sensitive_info); + + if (include_sensitive_info) { + // Contains too much unrelated PII, and given the unstructured nature of sysprops, we can't + // really cherrypick all of the connectivity-related ones. Apps generally have no business + // reading these anyway, and there should be APIs to supply the info in a more app-friendly + // way. + RunCommand("SYSTEM PROPERTIES", {"getprop"}); + } printf("========================================================\n"); printf("== Android Framework Services\n"); @@ -1636,15 +1667,28 @@ static void DumpstateTelephonyOnly() { RunDumpsys("DUMPSYS", {"connectivity"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); - RunDumpsys("DUMPSYS", {"connmetrics"}, CommandOptions::WithTimeout(90).Build(), - SEC_TO_MSEC(10)); - RunDumpsys("DUMPSYS", {"netd"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); + // TODO(b/146521742) build out an argument to include bound services here for user builds RunDumpsys("DUMPSYS", {"carrier_config"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); RunDumpsys("DUMPSYS", {"wifi"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); - RunDumpsys("BATTERYSTATS", {"batterystats"}, CommandOptions::WithTimeout(90).Build(), + RunDumpsys("DUMPSYS", {"netpolicy"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); + RunDumpsys("DUMPSYS", {"network_management"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); + if (include_sensitive_info) { + // Contains raw IP addresses, omit from reports on user builds. + RunDumpsys("DUMPSYS", {"netd"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); + // Contains raw destination IP/MAC addresses, omit from reports on user builds. + RunDumpsys("DUMPSYS", {"connmetrics"}, CommandOptions::WithTimeout(90).Build(), + SEC_TO_MSEC(10)); + // Contains package/component names, omit from reports on user builds. + RunDumpsys("BATTERYSTATS", {"batterystats"}, CommandOptions::WithTimeout(90).Build(), + SEC_TO_MSEC(10)); + // Contains package names, but should be relatively simple to remove them (also contains + // UIDs already), omit from reports on user builds. + RunDumpsys("BATTERYSTATS", {"deviceidle"}, CommandOptions::WithTimeout(90).Build(), + SEC_TO_MSEC(10)); + } printf("========================================================\n"); printf("== Running Application Services\n"); @@ -1652,18 +1696,24 @@ static void DumpstateTelephonyOnly() { RunDumpsys("TELEPHONY SERVICES", {"activity", "service", "TelephonyDebugService"}); - printf("========================================================\n"); - printf("== Running Application Services (non-platform)\n"); - printf("========================================================\n"); + if (include_sensitive_info) { + printf("========================================================\n"); + printf("== Running Application Services (non-platform)\n"); + printf("========================================================\n"); - RunDumpsys("APP SERVICES NON-PLATFORM", {"activity", "service", "all-non-platform"}, - DUMPSYS_COMPONENTS_OPTIONS); + // Contains package/component names and potential PII, omit from reports on user builds. + // To get dumps of the active CarrierService(s) on user builds, we supply an argument to the + // carrier_config dumpsys instead. + RunDumpsys("APP SERVICES NON-PLATFORM", {"activity", "service", "all-non-platform"}, + DUMPSYS_COMPONENTS_OPTIONS); - printf("========================================================\n"); - printf("== Checkins\n"); - printf("========================================================\n"); + printf("========================================================\n"); + printf("== Checkins\n"); + printf("========================================================\n"); - RunDumpsys("CHECKIN BATTERYSTATS", {"batterystats", "-c"}); + // Contains package/component names, omit from reports on user builds. + RunDumpsys("CHECKIN BATTERYSTATS", {"batterystats", "-c"}); + } printf("========================================================\n"); printf("== dumpstate: done (id %d)\n", ds.id_); @@ -2275,6 +2325,7 @@ static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOpt break; case Dumpstate::BugreportMode::BUGREPORT_TELEPHONY: options->telephony_only = true; + options->do_progress_updates = true; options->do_fb = false; options->do_broadcast = true; break; diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index cff1d439d9..99d482f034 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -372,12 +372,12 @@ TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { EXPECT_TRUE(options_.do_broadcast); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.telephony_only); + EXPECT_TRUE(options_.do_progress_updates); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); EXPECT_FALSE(options_.use_control_socket); EXPECT_FALSE(options_.show_header_only); - EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); } -- cgit v1.2.3-59-g8ed1b From 70610fa2153ad198f0d2fe433b54d8083e805939 Mon Sep 17 00:00:00 2001 From: Hunter Knepshield Date: Fri, 3 Jan 2020 15:27:33 -0800 Subject: Update timeout logic for connectivity reports. Typically, BUGREPORT_MODE_TELEPHONY takes single-digit seconds, which leaves the routine waiting for just USER_CONSENT_TIMEOUT_MS (30 seconds) for the user to respond. Given the sizeable dialog with lots to read, we don't believe this is a reasonable default. We increase the timeout specifically for MODE_TELEPHONY to be 2 minutes, which should then roughly match the time it takes a full report to be generated, and plenty of time for the user to fully read the consent dialog. Bug: 146521742 Test: manual, ensure timeout happens after 2 minutes instead of 30 seconds Change-Id: I3a5d9c696470a0dc3cbeb0d84b78ea36a3694fea --- cmds/dumpstate/dumpstate.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 7cfd4d14b5..dccaf99c8d 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -135,6 +135,11 @@ typedef Dumpstate::ConsentCallback::ConsentResult UserConsentResult; static char cmdline_buf[16384] = "(unknown)"; static const char *dump_traces_path = nullptr; static const uint64_t USER_CONSENT_TIMEOUT_MS = 30 * 1000; +// Because telephony reports are significantly faster to collect (< 10 seconds vs. > 2 minutes), +// it's often the case that they time out far too quickly for consent with such a hefty dialog for +// the user to read. For telephony reports only, we increase the default timeout to 2 minutes to +// roughly match full reports' durations. +static const uint64_t TELEPHONY_REPORT_USER_CONSENT_TIMEOUT_MS = 2 * 60 * 1000; // TODO: variables and functions below should be part of dumpstate object @@ -2876,8 +2881,13 @@ Dumpstate::RunStatus Dumpstate::CopyBugreportIfUserConsented() { if (consent_result == UserConsentResult::UNAVAILABLE) { // User has not responded yet. uint64_t elapsed_ms = consent_callback_->getElapsedTimeMs(); - if (elapsed_ms < USER_CONSENT_TIMEOUT_MS) { - uint delay_seconds = (USER_CONSENT_TIMEOUT_MS - elapsed_ms) / 1000; + // Telephony is a fast report type, particularly on user builds where information may be + // more aggressively limited. To give the user time to read the consent dialog, increase the + // timeout. + uint64_t timeout_ms = options_->telephony_only ? TELEPHONY_REPORT_USER_CONSENT_TIMEOUT_MS + : USER_CONSENT_TIMEOUT_MS; + if (elapsed_ms < timeout_ms) { + uint delay_seconds = (timeout_ms - elapsed_ms) / 1000; MYLOGD("Did not receive user consent yet; going to wait for %d seconds", delay_seconds); sleep(delay_seconds); } -- cgit v1.2.3-59-g8ed1b