From 09c8b5ba5955162f90b105f8364528b2e18e79f9 Mon Sep 17 00:00:00 2001 From: Hunter Knepshield Date: Wed, 12 Feb 2020 18:55:28 -0800 Subject: [PATCH] IDumpstateDevice 1.1 tweak: "device" -> "verbose" Pixel has been dumping some non-sensitive information in bug reports using IDumpstateDevice for a long time, and requiring nothing to be dumped on user builds by default suddenly changes behavior. To account for this use case, we instead change the meaning of the toggle to control *verbose* logging, specifically anything with privacy, storage, or battery impact. VTS tests are updated appropriately. Bug: 143183758 Bug: 143184495 Test: atest VtsHalDumpstateV1_1TargetTest Change-Id: Ib71ce43e9168d82fd9ee0564db813c5a3538c459 --- current.txt | 2 +- dumpstate/1.1/IDumpstateDevice.hal | 26 +++++---- .../VtsHalDumpstateV1_1TargetTest.cpp | 58 +++++++++---------- 3 files changed, 44 insertions(+), 42 deletions(-) diff --git a/current.txt b/current.txt index 7d67361b66..459044f4e2 100644 --- a/current.txt +++ b/current.txt @@ -646,7 +646,7 @@ f18695dd36ee205640b8326a17453858a7b4596653aaa6ef0016b0aef1bd4dac android.hardwar 66931c2506fbb5af61f20138cb05e0a09e7bf67d6964c231d27c648933bb33ec android.hardware.drm@1.3::ICryptoFactory 994d08ab27d613022c258a9ec48cece7adf2a305e92df5d76ef923e2c6665f64 android.hardware.drm@1.3::IDrmFactory 446287268831f4ddfac4a51bb1c32ae1e48e47bccd535fccc2c4546d0e7c4013 android.hardware.dumpstate@1.1::types -f284ffde7cadf5a1364b75ab313baf22401eeca289bdde2a2dc7a27ea4ab98d7 android.hardware.dumpstate@1.1::IDumpstateDevice +186bc152ae189ab48f3a761a44ddf5edd0d483073c5b6ca1f802f8b50488b754 android.hardware.dumpstate@1.1::IDumpstateDevice 769d346927a94fd40ee80a5a976d8d15cf022ef99c5900738f4a82f26c0ed229 android.hardware.gnss@2.1::types 626db710bf917ecf551a0b0b1f25be10bf52758f43e0fc808b148b6aae2ef73e android.hardware.gnss@2.1::IGnss ba5ac712b2a656dc07c83ab4a7a2c2f3bee1bbcb752e8b8ffa9b672f3b5b0728 android.hardware.gnss@2.1::IGnssAntennaInfo diff --git a/dumpstate/1.1/IDumpstateDevice.hal b/dumpstate/1.1/IDumpstateDevice.hal index 502c460998..183404dd68 100644 --- a/dumpstate/1.1/IDumpstateDevice.hal +++ b/dumpstate/1.1/IDumpstateDevice.hal @@ -29,8 +29,9 @@ interface IDumpstateDevice extends @1.0::IDumpstateDevice { * The 1.0 version of #dumpstateBoard(handle) should just delegate to this new method and pass * DumpstateMode::DEFAULT and a timeout of 30,000ms (30 seconds). * - * This method may still be called by the dumpstate routine even if getDeviceLoggingEnabled - * returns false. In this case, it must return DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED. + * This method may still be called by the dumpstate routine even if getVerboseLoggingEnabled + * returns false. In this case, it may include essential information but must not include + * information that identifies the user. * * @param h A native handle with one or two valid file descriptors. The first FD is for text * output, the second (if present) is for binary output. @@ -44,7 +45,7 @@ interface IDumpstateDevice extends @1.0::IDumpstateDevice { generates (DumpstateStatus status); /** - * Turns device vendor logging on or off. + * Turns verbose device vendor logging on or off. * * The setting should be persistent across reboots. Underlying implementations may need to start * vendor logging daemons, set system properties, or change logging masks, for example. Given @@ -52,20 +53,21 @@ interface IDumpstateDevice extends @1.0::IDumpstateDevice { * memory/storage/battery impacts, calling this method on a user build should only be done after * user consent has been obtained, e.g. from a toggle in developer settings. * - * Even if device logging has been disabled, dumpstateBoard may still be called by the dumpstate - * routine. In this case, it must return DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED. + * Even if verbose logging has been disabled, dumpstateBoard may still be called by the + * dumpstate routine, and essential information that does not identify the user may be included. * - * @param enable Whether to enable or disable device vendor logging. + * @param enable Whether to enable or disable verbose vendor logging. */ - setDeviceLoggingEnabled(bool enable); + setVerboseLoggingEnabled(bool enable); /** - * Queries the current state of device logging. Primarily for UI and informative purposes. + * Queries the current state of verbose device logging. Primarily for UI and informative + * purposes. * - * Even if device logging has been disabled, dumpstateBoard may still be called by the dumpstate - * routine. In this case, it must return DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED. + * Even if verbose logging has been disabled, dumpstateBoard may still be called by the + * dumpstate routine, and essential information that does not identify the user may be included. * - * @return enabled Whether or not vendor logging is currently enabled. + * @return enabled Whether or not verbose vendor logging is currently enabled. */ - getDeviceLoggingEnabled() generates (bool enabled); + getVerboseLoggingEnabled() generates (bool enabled); }; diff --git a/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp b/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp index 089b039c2c..51dce5e345 100644 --- a/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp +++ b/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp @@ -48,8 +48,8 @@ class DumpstateHidl1_1Test : public ::testing::TestWithParam { ASSERT_NE(dumpstate, nullptr) << "Could not get HIDL instance"; } - void ToggleDeviceLogging(bool enable) { - Return status = dumpstate->setDeviceLoggingEnabled(enable); + void ToggleVerboseLogging(bool enable) { + Return status = dumpstate->setVerboseLoggingEnabled(enable); ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description(); if (!dumpstate->ping().isOk()) { @@ -58,11 +58,11 @@ class DumpstateHidl1_1Test : public ::testing::TestWithParam { GetService(); } - Return logging_enabled = dumpstate->getDeviceLoggingEnabled(); + Return logging_enabled = dumpstate->getVerboseLoggingEnabled(); ASSERT_TRUE(logging_enabled.isOk()) << "Status should be ok: " << logging_enabled.description(); ASSERT_EQ(logging_enabled, enable) - << "Device logging should now be " << (enable ? "enabled" : "disabled"); + << "Verbose logging should now be " << (enable ? "enabled" : "disabled"); if (!dumpstate->ping().isOk()) { ALOGW("IDumpstateDevice service appears to have exited lazily, attempting to get " @@ -71,9 +71,9 @@ class DumpstateHidl1_1Test : public ::testing::TestWithParam { } } - void EnableDeviceLogging() { ToggleDeviceLogging(true); } + void EnableVerboseLogging() { ToggleVerboseLogging(true); } - void DisableDeviceLogging() { ToggleDeviceLogging(false); } + void DisableVerboseLogging() { ToggleVerboseLogging(false); } sp dumpstate; }; @@ -122,7 +122,7 @@ void AssertStatusForMode(const DumpstateMode mode, const Return // Negative test: make sure dumpstateBoard() doesn't crash when passed a null pointer. TEST_FOR_ALL_DUMPSTATE_MODES(TestNullHandle, [this](DumpstateMode mode) { - EnableDeviceLogging(); + EnableVerboseLogging(); Return status = dumpstate->dumpstateBoard_1_1(nullptr, mode, kDefaultTimeoutMillis); @@ -132,7 +132,7 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestNullHandle, [this](DumpstateMode mode) { // Negative test: make sure dumpstateBoard() ignores a handle with no FD. TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithNoFd, [this](DumpstateMode mode) { - EnableDeviceLogging(); + EnableVerboseLogging(); native_handle_t* handle = native_handle_create(0, 0); ASSERT_NE(handle, nullptr) << "Could not create native_handle"; @@ -148,7 +148,7 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithNoFd, [this](DumpstateMode mode) { // Positive test: make sure dumpstateBoard() writes something to the FD. TEST_FOR_ALL_DUMPSTATE_MODES(TestOk, [this](DumpstateMode mode) { - EnableDeviceLogging(); + EnableVerboseLogging(); // Index 0 corresponds to the read end of the pipe; 1 to the write end. int fds[2]; @@ -173,7 +173,7 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestOk, [this](DumpstateMode mode) { // Positive test: make sure dumpstateBoard() doesn't crash with two FDs. TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithTwoFds, [this](DumpstateMode mode) { - EnableDeviceLogging(); + EnableVerboseLogging(); int fds1[2]; int fds2[2]; @@ -203,7 +203,7 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithTwoFds, [this](DumpstateMode mode) { // Make sure dumpstateBoard_1_1 actually validates its arguments. TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Negative) { - EnableDeviceLogging(); + EnableVerboseLogging(); int fds[2]; ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno; @@ -225,7 +225,7 @@ TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Negative) { } TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Undefined) { - EnableDeviceLogging(); + EnableVerboseLogging(); int fds[2]; ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno; @@ -248,7 +248,7 @@ TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Undefined) { // Positive test: make sure dumpstateBoard() from 1.0 doesn't fail. TEST_P(DumpstateHidl1_1Test, Test1_0MethodOk) { - EnableDeviceLogging(); + EnableVerboseLogging(); int fds[2]; ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno; @@ -269,9 +269,10 @@ TEST_P(DumpstateHidl1_1Test, Test1_0MethodOk) { native_handle_delete(handle); } -// Make sure disabling device logging behaves correctly. -TEST_FOR_ALL_DUMPSTATE_MODES(TestDeviceLoggingDisabled, [this](DumpstateMode mode) { - DisableDeviceLogging(); +// Make sure disabling verbose logging behaves correctly. Some info is still allowed to be emitted, +// but it can't have privacy/storage/battery impacts. +TEST_FOR_ALL_DUMPSTATE_MODES(TestVerboseLoggingDisabled, [this](DumpstateMode mode) { + DisableVerboseLogging(); // Index 0 corresponds to the read end of the pipe; 1 to the write end. int fds[2]; @@ -284,11 +285,10 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestDeviceLoggingDisabled, [this](DumpstateMode mod Return status = dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); - AssertStatusForMode(mode, status, DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED, [&fds]() { - // Check that nothing was written. Could return 0 or -1. - char buff; - ASSERT_NE(1, read(fds[0], &buff, 1)) << "Dumped something when device logging is disabled"; - }); + // We don't include additional assertions here about the file passed in. If verbose logging is + // disabled, the OEM may choose to include nothing at all, but it is allowed to include some + // essential information based on the mode as long as it isn't private user information. + AssertStatusForMode(mode, status, DumpstateStatus::OK); native_handle_close(handle); native_handle_delete(handle); @@ -296,22 +296,22 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestDeviceLoggingDisabled, [this](DumpstateMode mod // Double-enable is perfectly valid, but the second call shouldn't do anything. TEST_P(DumpstateHidl1_1Test, TestRepeatedEnable) { - EnableDeviceLogging(); - EnableDeviceLogging(); + EnableVerboseLogging(); + EnableVerboseLogging(); } // Double-disable is perfectly valid, but the second call shouldn't do anything. TEST_P(DumpstateHidl1_1Test, TestRepeatedDisable) { - DisableDeviceLogging(); - DisableDeviceLogging(); + DisableVerboseLogging(); + DisableVerboseLogging(); } // Toggling in short order is perfectly valid. TEST_P(DumpstateHidl1_1Test, TestRepeatedToggle) { - EnableDeviceLogging(); - DisableDeviceLogging(); - EnableDeviceLogging(); - DisableDeviceLogging(); + EnableVerboseLogging(); + DisableVerboseLogging(); + EnableVerboseLogging(); + DisableVerboseLogging(); } INSTANTIATE_TEST_SUITE_P(