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

Commit 84dbf58f authored by Hunter Knepshield's avatar Hunter Knepshield
Browse files

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
Merged-In: Ib71ce43e9168d82fd9ee0564db813c5a3538c459
(cherry picked from commit 09c8b5ba)
parent 6a83338d

current.txt

+1 −1
Original line number Original line Diff line number Diff line
@@ -613,7 +613,7 @@ dd377f404a8e71f6191d295e10067db629b0f0c28e594af906f2bea5d87fe2cc android.hardwar 
07d0a252b2d8fa35887908a996ba395cf392968395fc30afab791f46e0c22a52 android.hardware.boot@1.1::IBootControl

07d0a252b2d8fa35887908a996ba395cf392968395fc30afab791f46e0c22a52 android.hardware.boot@1.1::IBootControl

74049a402be913963edfdd80828a53736570e9d8124a1bf18166b6ed46a6b0ab android.hardware.boot@1.1::types

74049a402be913963edfdd80828a53736570e9d8124a1bf18166b6ed46a6b0ab android.hardware.boot@1.1::types

d9df99be0f59d8f33a9699fe316c67bfd11818aa69440bb1123ba43e717cea85 android.hardware.dumpstate@1.1::types

d9df99be0f59d8f33a9699fe316c67bfd11818aa69440bb1123ba43e717cea85 android.hardware.dumpstate@1.1::types

f284ffde7cadf5a1364b75ab313baf22401eeca289bdde2a2dc7a27ea4ab98d7 android.hardware.dumpstate@1.1::IDumpstateDevice

186bc152ae189ab48f3a761a44ddf5edd0d483073c5b6ca1f802f8b50488b754 android.hardware.dumpstate@1.1::IDumpstateDevice

ce8dbe76eb9ee94b46ef98f725be992e760a5751073d4f4912484026541371f3 android.hardware.health@2.1::IHealth

ce8dbe76eb9ee94b46ef98f725be992e760a5751073d4f4912484026541371f3 android.hardware.health@2.1::IHealth

26f04510a0b57aba5167c5c0a7c2f077c2acbb98b81902a072517829fd9fd67f android.hardware.health@2.1::IHealthInfoCallback

26f04510a0b57aba5167c5c0a7c2f077c2acbb98b81902a072517829fd9fd67f android.hardware.health@2.1::IHealthInfoCallback

e2f8bc1868fd4a3fd587c172773ea5a8c2f5a3deaf7958394102ca455252b255 android.hardware.health@2.1::types

e2f8bc1868fd4a3fd587c172773ea5a8c2f5a3deaf7958394102ca455252b255 android.hardware.health@2.1::types

dumpstate/1.1/IDumpstateDevice.hal

+14 −12
Original line number Original line Diff line number Diff line
@@ -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

     * 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).

     * DumpstateMode::DEFAULT and a timeout of 30,000ms (30 seconds).

     *

     *

     * This method may still be called by the dumpstate routine even if getDeviceLoggingEnabled

     * This method may still be called by the dumpstate routine even if getVerboseLoggingEnabled

     * returns false. In this case, it must return DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED.

     * 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

     * @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.

     *     output, the second (if present) is for binary output.
@@ -44,7 +45,7 @@ interface IDumpstateDevice extends @1.0::IDumpstateDevice { 
        generates (DumpstateStatus status);

        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

     * 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

     * 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

     * 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.

     * 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

     * Even if verbose logging has been disabled, dumpstateBoard may still be called by the

     * routine. In this case, it must return DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED.

     * 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

     * Even if verbose logging has been disabled, dumpstateBoard may still be called by the

     * routine. In this case, it must return DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED.

     * 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);

};
 
};

dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp

+29 −29
Original line number Original line Diff line number Diff line
@@ -48,8 +48,8 @@ class DumpstateHidl1_1Test : public ::testing::TestWithParam<std::string> { 
        ASSERT_NE(dumpstate, nullptr) << "Could not get HIDL instance";

        ASSERT_NE(dumpstate, nullptr) << "Could not get HIDL instance";

    }

    }





    void ToggleDeviceLogging(bool enable) {

    void ToggleVerboseLogging(bool enable) {

        Return<void> status = dumpstate->setDeviceLoggingEnabled(enable);

        Return<void> status = dumpstate->setVerboseLoggingEnabled(enable);

        ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description();

        ASSERT_TRUE(status.isOk()) << "Status should be ok: " << status.description();





        if (!dumpstate->ping().isOk()) {

        if (!dumpstate->ping().isOk()) {
@@ -58,11 +58,11 @@ class DumpstateHidl1_1Test : public ::testing::TestWithParam<std::string> { 
            GetService();

            GetService();

        }

        }





        Return<bool> logging_enabled = dumpstate->getDeviceLoggingEnabled();

        Return<bool> logging_enabled = dumpstate->getVerboseLoggingEnabled();

        ASSERT_TRUE(logging_enabled.isOk())

        ASSERT_TRUE(logging_enabled.isOk())

                << "Status should be ok: " << logging_enabled.description();

                << "Status should be ok: " << logging_enabled.description();

        ASSERT_EQ(logging_enabled, enable)

        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()) {

        if (!dumpstate->ping().isOk()) {

            ALOGW("IDumpstateDevice service appears to have exited lazily, attempting to get "

            ALOGW("IDumpstateDevice service appears to have exited lazily, attempting to get "
@@ -71,9 +71,9 @@ class DumpstateHidl1_1Test : public ::testing::TestWithParam<std::string> { 
        }

        }

    }

    }





    void EnableDeviceLogging() { ToggleDeviceLogging(true); }

    void EnableVerboseLogging() { ToggleVerboseLogging(true); }





    void DisableDeviceLogging() { ToggleDeviceLogging(false); }

    void DisableVerboseLogging() { ToggleVerboseLogging(false); }





    sp<IDumpstateDevice> dumpstate;

    sp<IDumpstateDevice> dumpstate;

};

};
@@ -123,7 +123,7 @@ void AssertStatusForMode(const DumpstateMode mode, const Return<DumpstateStatus> 




// Negative test: make sure dumpstateBoard() doesn't crash when passed a null pointer.

// Negative test: make sure dumpstateBoard() doesn't crash when passed a null pointer.

TEST_FOR_ALL_DUMPSTATE_MODES(TestNullHandle, [this](DumpstateMode mode) {

TEST_FOR_ALL_DUMPSTATE_MODES(TestNullHandle, [this](DumpstateMode mode) {

    EnableDeviceLogging();

    EnableVerboseLogging();





    Return<DumpstateStatus> status =

    Return<DumpstateStatus> status =

            dumpstate->dumpstateBoard_1_1(nullptr, mode, kDefaultTimeoutMillis);

            dumpstate->dumpstateBoard_1_1(nullptr, mode, kDefaultTimeoutMillis);
@@ -133,7 +133,7 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestNullHandle, [this](DumpstateMode mode) { 




// Negative test: make sure dumpstateBoard() ignores a handle with no FD.

// Negative test: make sure dumpstateBoard() ignores a handle with no FD.

TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithNoFd, [this](DumpstateMode mode) {

TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithNoFd, [this](DumpstateMode mode) {

    EnableDeviceLogging();

    EnableVerboseLogging();





    native_handle_t* handle = native_handle_create(0, 0);

    native_handle_t* handle = native_handle_create(0, 0);

    ASSERT_NE(handle, nullptr) << "Could not create native_handle";

    ASSERT_NE(handle, nullptr) << "Could not create native_handle";
@@ -149,7 +149,7 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithNoFd, [this](DumpstateMode mode) { 




// Positive test: make sure dumpstateBoard() writes something to the FD.

// Positive test: make sure dumpstateBoard() writes something to the FD.

TEST_FOR_ALL_DUMPSTATE_MODES(TestOk, [this](DumpstateMode mode) {

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.

    // Index 0 corresponds to the read end of the pipe; 1 to the write end.

    int fds[2];

    int fds[2];
@@ -174,7 +174,7 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestOk, [this](DumpstateMode mode) { 




// Positive test: make sure dumpstateBoard() doesn't crash with two FDs.

// Positive test: make sure dumpstateBoard() doesn't crash with two FDs.

TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithTwoFds, [this](DumpstateMode mode) {

TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithTwoFds, [this](DumpstateMode mode) {

    EnableDeviceLogging();

    EnableVerboseLogging();





    int fds1[2];

    int fds1[2];

    int fds2[2];

    int fds2[2];
@@ -204,7 +204,7 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithTwoFds, [this](DumpstateMode mode) { 




// Make sure dumpstateBoard_1_1 actually validates its arguments.

// Make sure dumpstateBoard_1_1 actually validates its arguments.

TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Negative) {

TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Negative) {

    EnableDeviceLogging();

    EnableVerboseLogging();





    int fds[2];

    int fds[2];

    ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno;

    ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno;
@@ -226,7 +226,7 @@ TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Negative) { 
}

}





TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Undefined) {

TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Undefined) {

    EnableDeviceLogging();

    EnableVerboseLogging();





    int fds[2];

    int fds[2];

    ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno;

    ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno;
@@ -249,7 +249,7 @@ TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Undefined) { 




// Positive test: make sure dumpstateBoard() from 1.0 doesn't fail.

// Positive test: make sure dumpstateBoard() from 1.0 doesn't fail.

TEST_P(DumpstateHidl1_1Test, Test1_0MethodOk) {

TEST_P(DumpstateHidl1_1Test, Test1_0MethodOk) {

    EnableDeviceLogging();

    EnableVerboseLogging();





    int fds[2];

    int fds[2];

    ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno;

    ASSERT_EQ(0, pipe2(fds, O_NONBLOCK)) << errno;
@@ -270,9 +270,10 @@ TEST_P(DumpstateHidl1_1Test, Test1_0MethodOk) { 
    native_handle_delete(handle);

    native_handle_delete(handle);

}

}





// Make sure disabling device logging behaves correctly.

// Make sure disabling verbose logging behaves correctly. Some info is still allowed to be emitted,

TEST_FOR_ALL_DUMPSTATE_MODES(TestDeviceLoggingDisabled, [this](DumpstateMode mode) {

// but it can't have privacy/storage/battery impacts.

    DisableDeviceLogging();

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.

    // Index 0 corresponds to the read end of the pipe; 1 to the write end.

    int fds[2];

    int fds[2];
@@ -285,11 +286,10 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestDeviceLoggingDisabled, [this](DumpstateMode mod 
    Return<DumpstateStatus> status =

    Return<DumpstateStatus> status =

            dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis);

            dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis);





    AssertStatusForMode(mode, status, DumpstateStatus::DEVICE_LOGGING_NOT_ENABLED, [&fds]() {

    // We don't include additional assertions here about the file passed in. If verbose logging is

        // Check that nothing was written. Could return 0 or -1.

    // disabled, the OEM may choose to include nothing at all, but it is allowed to include some

        char buff;

    // essential information based on the mode as long as it isn't private user information.

        ASSERT_NE(1, read(fds[0], &buff, 1)) << "Dumped something when device logging is disabled";

    AssertStatusForMode(mode, status, DumpstateStatus::OK);

    });





    native_handle_close(handle);

    native_handle_close(handle);

    native_handle_delete(handle);

    native_handle_delete(handle);
@@ -297,22 +297,22 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestDeviceLoggingDisabled, [this](DumpstateMode mod 




// Double-enable is perfectly valid, but the second call shouldn't do anything.

// Double-enable is perfectly valid, but the second call shouldn't do anything.

TEST_P(DumpstateHidl1_1Test, TestRepeatedEnable) {

TEST_P(DumpstateHidl1_1Test, TestRepeatedEnable) {

    EnableDeviceLogging();

    EnableVerboseLogging();

    EnableDeviceLogging();

    EnableVerboseLogging();

}

}





// Double-disable is perfectly valid, but the second call shouldn't do anything.

// Double-disable is perfectly valid, but the second call shouldn't do anything.

TEST_P(DumpstateHidl1_1Test, TestRepeatedDisable) {

TEST_P(DumpstateHidl1_1Test, TestRepeatedDisable) {

    DisableDeviceLogging();

    DisableVerboseLogging();

    DisableDeviceLogging();

    DisableVerboseLogging();

}

}





// Toggling in short order is perfectly valid.

// Toggling in short order is perfectly valid.

TEST_P(DumpstateHidl1_1Test, TestRepeatedToggle) {

TEST_P(DumpstateHidl1_1Test, TestRepeatedToggle) {

    EnableDeviceLogging();

    EnableVerboseLogging();

    DisableDeviceLogging();

    DisableVerboseLogging();

    EnableDeviceLogging();

    EnableVerboseLogging();

    DisableDeviceLogging();

    DisableVerboseLogging();

}

}





INSTANTIATE_TEST_SUITE_P(

INSTANTIATE_TEST_SUITE_P(