Look up callers' UIDs when wake locks are acquired.
Update PowerManager::acquireWakeLock() to get the calling
process's actual UID instead of just storing -1. (The UID
isn't used for anything, though.)
Also do some related refactoring that came up while adding
tests: to avoid duplicated code, make PowerManagerStub use
WakeLockManagerStub instead of recording wake lock requests
itself.
Bug: 24988639
Change-Id: If2d2d2935052ab268f6b21dd72a7b1f02a66d799
diff --git a/client/wake_lock_unittest.cc b/client/wake_lock_unittest.cc
index 09f3dbf..dbd1505 100644
--- a/client/wake_lock_unittest.cc
+++ b/client/wake_lock_unittest.cc
@@ -50,15 +50,17 @@
};
TEST_F(WakeLockTest, CreateAndDestroy) {
+ const uid_t kUid = 123;
+ binder_wrapper()->set_calling_uid(kUid);
std::unique_ptr<WakeLock> lock(client_.CreateWakeLock("foo", "bar"));
- ASSERT_EQ(1u, power_manager_->num_locks());
+ ASSERT_EQ(1, power_manager_->GetNumWakeLocks());
ASSERT_EQ(1u, binder_wrapper()->local_binders().size());
EXPECT_EQ(
- PowerManagerStub::ConstructLockString("foo", "bar", -1),
- power_manager_->GetLockString(binder_wrapper()->local_binders()[0]));
+ PowerManagerStub::ConstructWakeLockString("foo", "bar", kUid),
+ power_manager_->GetWakeLockString(binder_wrapper()->local_binders()[0]));
lock.reset();
- EXPECT_EQ(0u, power_manager_->num_locks());
+ EXPECT_EQ(0, power_manager_->GetNumWakeLocks());
}
TEST_F(WakeLockTest, PowerManagerDeath) {
@@ -68,7 +70,7 @@
// Since PowerManagerClient was informed that the power manager died, WakeLock
// shouldn't try to release its lock on destruction.
lock.reset();
- EXPECT_EQ(1u, power_manager_->num_locks());
+ EXPECT_EQ(1, power_manager_->GetNumWakeLocks());
}
} // namespace android
diff --git a/daemon/Android.mk b/daemon/Android.mk
index 0eb0f5d..53d9b08 100644
--- a/daemon/Android.mk
+++ b/daemon/Android.mk
@@ -18,9 +18,13 @@
nativepowerman_CommonCFlags := -Wall -Werror -Wno-unused-parameter
nativepowerman_CommonCFlags += -Wno-sign-promo # for libchrome
-nativepowerman_CommonCIncludes := $(LOCAL_PATH)/../include
+nativepowerman_CommonCIncludes := \
+ $(LOCAL_PATH)/../include \
+ external/gtest/include \
+
nativepowerman_CommonSharedLibraries := \
libbinder \
+ libbinderwrapper \
libchrome \
libcutils \
libpowermanager \
@@ -39,7 +43,6 @@
LOCAL_STATIC_LIBRARIES := libnativepowerman
LOCAL_SHARED_LIBRARIES := \
$(nativepowerman_CommonSharedLibraries) \
- libbinderwrapper \
libchromeos \
libchromeos-binder \
@@ -69,11 +72,10 @@
LOCAL_MODULE := libnativepowerman
LOCAL_CPP_EXTENSION := .cc
LOCAL_CFLAGS := $(nativepowerman_CommonCFlags)
-LOCAL_C_INCLUDES := $(nativepowerman_CommonCIncludes) external/gtest/include
+LOCAL_C_INCLUDES := $(nativepowerman_CommonCIncludes)
LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/../include
LOCAL_SHARED_LIBRARIES := \
$(nativepowerman_CommonSharedLibraries) \
- libbinderwrapper \
libchromeos \
LOCAL_SRC_FILES := \
@@ -97,13 +99,12 @@
LOCAL_STATIC_LIBRARIES := libnativepowerman libgtest libBionicGtestMain
LOCAL_SHARED_LIBRARIES := \
$(nativepowerman_CommonSharedLibraries) \
- libbinderwrapper \
libbinderwrapper_test_support \
+ libnativepower_test_support \
LOCAL_SRC_FILES := \
power_manager_unittest.cc \
system_property_setter_stub.cc \
- wake_lock_manager_stub.cc \
wake_lock_manager_unittest.cc \
include $(BUILD_NATIVE_TEST)
@@ -121,5 +122,7 @@
LOCAL_SRC_FILES := \
BnPowerManager.cc \
power_manager_stub.cc \
+ wake_lock_manager.cc \
+ wake_lock_manager_stub.cc \
include $(BUILD_SHARED_LIBRARY)
diff --git a/daemon/power_manager.cc b/daemon/power_manager.cc
index bb642b1..0498965 100644
--- a/daemon/power_manager.cc
+++ b/daemon/power_manager.cc
@@ -62,7 +62,10 @@
const String16& tag,
const String16& packageName,
bool isOneWay) {
- return AddWakeLockRequest(lock, tag, packageName, -1) ? OK : UNKNOWN_ERROR;
+ return AddWakeLockRequest(lock, tag, packageName,
+ BinderWrapper::Get()->GetCallingUid())
+ ? OK
+ : UNKNOWN_ERROR;
}
status_t PowerManager::acquireWakeLockWithUid(int flags,
@@ -71,7 +74,9 @@
const String16& packageName,
int uid,
bool isOneWay) {
- return AddWakeLockRequest(lock, tag, packageName, uid) ? OK : UNKNOWN_ERROR;
+ return AddWakeLockRequest(lock, tag, packageName, static_cast<uid_t>(uid))
+ ? OK
+ : UNKNOWN_ERROR;
}
status_t PowerManager::releaseWakeLock(const sp<IBinder>& lock,
diff --git a/daemon/power_manager_stub.cc b/daemon/power_manager_stub.cc
index 7e71bcd..bb2588a 100644
--- a/daemon/power_manager_stub.cc
+++ b/daemon/power_manager_stub.cc
@@ -17,22 +17,14 @@
#include <base/format_macros.h>
#include <base/logging.h>
#include <base/strings/stringprintf.h>
+#include <binderwrapper/binder_wrapper.h>
#include <nativepower/power_manager_stub.h>
#include <utils/String8.h>
+#include "wake_lock_manager_stub.h"
+
namespace android {
-PowerManagerStub::LockInfo::LockInfo() : uid(-1) {}
-
-PowerManagerStub::LockInfo::LockInfo(const LockInfo& info) = default;
-
-PowerManagerStub::LockInfo::LockInfo(const std::string& tag,
- const std::string& package,
- int uid)
- : tag(tag),
- package(package),
- uid(uid) {}
-
PowerManagerStub::SuspendRequest::SuspendRequest(int64_t event_time_ms,
int reason,
int flags)
@@ -41,10 +33,11 @@
flags(flags) {}
// static
-std::string PowerManagerStub::ConstructLockString(const std::string& tag,
- const std::string& package,
- int uid) {
- return base::StringPrintf("%s,%s,%d", tag.c_str(), package.c_str(), uid);
+std::string PowerManagerStub::ConstructWakeLockString(
+ const std::string& tag,
+ const std::string& package,
+ uid_t uid) {
+ return WakeLockManagerStub::ConstructRequestString(tag, package, uid);
}
// static
@@ -55,17 +48,18 @@
return base::StringPrintf("%" PRId64 ",%d,%d", event_time_ms, reason, flags);
}
-PowerManagerStub::PowerManagerStub() = default;
+PowerManagerStub::PowerManagerStub()
+ : wake_lock_manager_(new WakeLockManagerStub()) {}
PowerManagerStub::~PowerManagerStub() = default;
-std::string PowerManagerStub::GetLockString(const sp<IBinder>& binder) const {
- const auto it = locks_.find(binder);
- if (it == locks_.end())
- return std::string();
+int PowerManagerStub::GetNumWakeLocks() const {
+ return wake_lock_manager_->num_requests();
+}
- const LockInfo& info = it->second;
- return ConstructLockString(info.tag, info.package, info.uid);
+std::string PowerManagerStub::GetWakeLockString(
+ const sp<IBinder>& binder) const {
+ return wake_lock_manager_->GetRequestString(binder);
}
std::string PowerManagerStub::GetSuspendRequestString(size_t index) const {
@@ -82,10 +76,9 @@
const String16& tag,
const String16& packageName,
bool isOneWay) {
- CHECK(!locks_.count(lock)) << "Got acquireWakeLock() request for "
- << "already-registered binder " << lock.get();
- locks_[lock] =
- LockInfo(String8(tag).string(), String8(packageName).string(), -1);
+ CHECK(wake_lock_manager_->AddRequest(lock, String8(tag).string(),
+ String8(packageName).string(),
+ BinderWrapper::Get()->GetCallingUid()));
return OK;
}
@@ -95,19 +88,16 @@
const String16& packageName,
int uid,
bool isOneWay) {
- CHECK(!locks_.count(lock)) << "Got acquireWakeLockWithUid() request for "
- << "already-registered binder " << lock.get();
- locks_[lock] =
- LockInfo(String8(tag).string(), String8(packageName).string(), uid);
+ CHECK(wake_lock_manager_->AddRequest(lock, String8(tag).string(),
+ String8(packageName).string(),
+ static_cast<uid_t>(uid)));
return OK;
}
status_t PowerManagerStub::releaseWakeLock(const sp<IBinder>& lock,
int flags,
bool isOneWay) {
- CHECK(locks_.count(lock)) << "Got releaseWakeLock() request for unregistered "
- << "binder " << lock.get();
- locks_.erase(lock);
+ CHECK(wake_lock_manager_->RemoveRequest(lock));
return OK;
}
diff --git a/daemon/power_manager_unittest.cc b/daemon/power_manager_unittest.cc
index 148035c..a81f935 100644
--- a/daemon/power_manager_unittest.cc
+++ b/daemon/power_manager_unittest.cc
@@ -88,13 +88,33 @@
}
TEST_F(PowerManagerTest, AcquireAndReleaseWakeLock) {
+ const char kTag[] = "foo";
+ const char kPackage[] = "bar";
sp<BBinder> binder = binder_wrapper()->CreateLocalBinder();
- EXPECT_EQ(OK, interface_->acquireWakeLock(0, binder, String16(), String16()));
- ASSERT_EQ(1u, wake_lock_manager_->request_binders().size());
- EXPECT_TRUE(wake_lock_manager_->has_request_binder(binder));
+
+ // Check that PowerManager looks up the calling UID when necessary.
+ const uid_t kCallingUid = 100;
+ binder_wrapper()->set_calling_uid(kCallingUid);
+ EXPECT_EQ(OK, interface_->acquireWakeLock(0, binder, String16(kTag),
+ String16(kPackage)));
+ EXPECT_EQ(1, wake_lock_manager_->num_requests());
+ EXPECT_EQ(
+ WakeLockManagerStub::ConstructRequestString(kTag, kPackage, kCallingUid),
+ wake_lock_manager_->GetRequestString(
+ binder_wrapper()->local_binders()[0]));
EXPECT_EQ(OK, interface_->releaseWakeLock(binder, 0));
- EXPECT_TRUE(wake_lock_manager_->request_binders().empty());
+ EXPECT_EQ(0, wake_lock_manager_->num_requests());
+
+ // If a UID is passed, it should be used instead.
+ const uid_t kPassedUid = 200;
+ EXPECT_EQ(OK, interface_->acquireWakeLockWithUid(
+ 0, binder, String16(kTag), String16(kPackage), kPassedUid));
+ EXPECT_EQ(1, wake_lock_manager_->num_requests());
+ EXPECT_EQ(
+ WakeLockManagerStub::ConstructRequestString(kTag, kPackage, kPassedUid),
+ wake_lock_manager_->GetRequestString(
+ binder_wrapper()->local_binders()[0]));
}
TEST_F(PowerManagerTest, GoToSleep) {
diff --git a/daemon/wake_lock_manager.cc b/daemon/wake_lock_manager.cc
index 3c875f6..37de1e3 100644
--- a/daemon/wake_lock_manager.cc
+++ b/daemon/wake_lock_manager.cc
@@ -51,14 +51,14 @@
WakeLockManager::Request::Request(const std::string& tag,
const std::string& package,
- int uid)
+ uid_t uid)
: tag(tag),
package(package),
uid(uid) {}
WakeLockManager::Request::Request(const Request& request) = default;
-WakeLockManager::Request::Request() : uid(0) {}
+WakeLockManager::Request::Request() : uid(-1) {}
WakeLockManager::WakeLockManager()
: lock_path_(kLockPath),
@@ -82,7 +82,7 @@
bool WakeLockManager::AddRequest(sp<IBinder> client_binder,
const std::string& tag,
const std::string& package,
- int uid) {
+ uid_t uid) {
const bool new_request = !requests_.count(client_binder);
LOG(INFO) << (new_request ? "Adding" : "Updating") << " request for binder "
<< client_binder.get() << ": tag=\"" << tag << "\""
diff --git a/daemon/wake_lock_manager.h b/daemon/wake_lock_manager.h
index 471ad10..cc4dcc9 100644
--- a/daemon/wake_lock_manager.h
+++ b/daemon/wake_lock_manager.h
@@ -17,6 +17,8 @@
#ifndef SYSTEM_NATIVEPOWER_DAEMON_WAKE_LOCK_MANAGER_H_
#define SYSTEM_NATIVEPOWER_DAEMON_WAKE_LOCK_MANAGER_H_
+#include <sys/types.h>
+
#include <map>
#include <string>
@@ -37,8 +39,20 @@
virtual bool AddRequest(sp<IBinder> client_binder,
const std::string& tag,
const std::string& package,
- int uid) = 0;
+ uid_t uid) = 0;
virtual bool RemoveRequest(sp<IBinder> client_binder) = 0;
+
+ protected:
+ // Information about a request from a client.
+ struct Request {
+ Request(const std::string& tag, const std::string& package, uid_t uid);
+ Request(const Request& request);
+ Request();
+
+ std::string tag;
+ std::string package;
+ uid_t uid;
+ };
};
class WakeLockManager : public WakeLockManagerInterface {
@@ -61,21 +75,10 @@
bool AddRequest(sp<IBinder> client_binder,
const std::string& tag,
const std::string& package,
- int uid) override;
+ uid_t uid) override;
bool RemoveRequest(sp<IBinder> client_binder) override;
private:
- // Information about a request from a client.
- struct Request {
- Request(const std::string& tag, const std::string& package, int uid);
- Request(const Request& request);
- Request();
-
- std::string tag;
- std::string package;
- int uid;
- };
-
void HandleBinderDeath(sp<IBinder> binder);
base::FilePath lock_path_;
diff --git a/daemon/wake_lock_manager_stub.cc b/daemon/wake_lock_manager_stub.cc
index da7dc5e..3b56c4e 100644
--- a/daemon/wake_lock_manager_stub.cc
+++ b/daemon/wake_lock_manager_stub.cc
@@ -16,27 +16,46 @@
#include "wake_lock_manager_stub.h"
+#include <base/strings/stringprintf.h>
#include <binder/IBinder.h>
namespace android {
+// static
+std::string WakeLockManagerStub::ConstructRequestString(
+ const std::string& tag,
+ const std::string& package,
+ uid_t uid) {
+ return base::StringPrintf("%s,%s,%d", tag.c_str(), package.c_str(), uid);
+}
+
WakeLockManagerStub::WakeLockManagerStub() = default;
WakeLockManagerStub::~WakeLockManagerStub() = default;
+std::string WakeLockManagerStub::GetRequestString(
+ const sp<IBinder>& binder) const {
+ const auto it = requests_.find(binder);
+ if (it == requests_.end())
+ return std::string();
+
+ const Request& req = it->second;
+ return ConstructRequestString(req.tag, req.package, req.uid);
+}
+
bool WakeLockManagerStub::AddRequest(sp<IBinder> client_binder,
const std::string& tag,
const std::string& package,
- int uid) {
- request_binders_.insert(client_binder);
+ uid_t uid) {
+ requests_[client_binder] = Request(tag, package, uid);
return true;
}
bool WakeLockManagerStub::RemoveRequest(sp<IBinder> client_binder) {
- if (!request_binders_.count(client_binder))
+ if (!requests_.count(client_binder))
return false;
- request_binders_.erase(client_binder);
+ requests_.erase(client_binder);
return true;
}
diff --git a/daemon/wake_lock_manager_stub.h b/daemon/wake_lock_manager_stub.h
index 3b73749..d720143 100644
--- a/daemon/wake_lock_manager_stub.h
+++ b/daemon/wake_lock_manager_stub.h
@@ -19,6 +19,7 @@
#include <set>
#include <string>
+#include <sys/types.h>
#include <base/macros.h>
#include <utils/StrongPointer.h>
@@ -32,26 +33,31 @@
// Stub implementation used by tests.
class WakeLockManagerStub : public WakeLockManagerInterface {
public:
- using BinderSet = std::set<sp<IBinder>>;
-
WakeLockManagerStub();
~WakeLockManagerStub() override;
- const BinderSet& request_binders() const { return request_binders_; }
- bool has_request_binder(const sp<IBinder>& binder) const {
- return request_binders_.count(binder);
- }
+ // Constructs a string that can be compared with one returned by
+ // GetRequestString().
+ static std::string ConstructRequestString(const std::string& tag,
+ const std::string& package,
+ uid_t uid);
+
+ int num_requests() const { return requests_.size(); }
+
+ // Returns a string describing the request associated with |binder|, or an
+ // empty string if no request is present.
+ std::string GetRequestString(const sp<IBinder>& binder) const;
// WakeLockManagerInterface:
bool AddRequest(sp<IBinder> client_binder,
const std::string& tag,
const std::string& package,
- int uid) override;
+ uid_t uid) override;
bool RemoveRequest(sp<IBinder> client_binder) override;
private:
- // Binders passed to AddRequest() for currently-active requests.
- BinderSet request_binders_;
+ // Currently-active requests, keyed by client binders.
+ std::map<sp<IBinder>, Request> requests_;
DISALLOW_COPY_AND_ASSIGN(WakeLockManagerStub);
};
diff --git a/include/nativepower/power_manager_stub.h b/include/nativepower/power_manager_stub.h
index d31c130..47ae1dd 100644
--- a/include/nativepower/power_manager_stub.h
+++ b/include/nativepower/power_manager_stub.h
@@ -14,7 +14,10 @@
* limitations under the License.
*/
+#include <sys/types.h>
+
#include <map>
+#include <memory>
#include <string>
#include <vector>
@@ -23,17 +26,21 @@
namespace android {
+class WakeLockManagerStub;
+
// Stub implementation of BnPowerManager for use in tests.
+//
+// The BinderWrapper singleton must be initialized before using this class.
class PowerManagerStub : public BnPowerManager {
public:
PowerManagerStub();
~PowerManagerStub() override;
// Constructs a string that can be compared with one returned by
- // GetLockString().
- static std::string ConstructLockString(const std::string& tag,
- const std::string& package,
- int uid);
+ // GetWakeLockString().
+ static std::string ConstructWakeLockString(const std::string& tag,
+ const std::string& package,
+ uid_t uid);
// Constructs a string that can be compared with one returned by
// GetSuspendRequestString().
@@ -41,7 +48,6 @@
int reason,
int flags);
- size_t num_locks() const { return locks_.size(); }
size_t num_suspend_requests() const { return suspend_requests_.size(); }
const std::vector<std::string>& reboot_reasons() const {
return reboot_reasons_;
@@ -50,9 +56,12 @@
return shutdown_reasons_;
}
- // Returns a string describing the lock registered for |binder|, or an empty
- // string if no lock is present.
- std::string GetLockString(const sp<IBinder>& binder) const;
+ // Returns the number of currently-registered wake locks.
+ int GetNumWakeLocks() const;
+
+ // Returns a string describing the wake lock registered for |binder|, or an
+ // empty string if no wake lock is present.
+ std::string GetWakeLockString(const sp<IBinder>& binder) const;
// Returns a string describing position |index| in |suspend_requests_|.
std::string GetSuspendRequestString(size_t index) const;
@@ -83,22 +92,6 @@
status_t crash(const String16& message) override;
private:
- // Contains information passed to acquireWakeLock() or
- // acquireWakeLockWithUid().
- struct LockInfo {
- LockInfo();
- LockInfo(const LockInfo& info);
- LockInfo(const std::string& tag,
- const std::string& package,
- int uid);
-
- std::string tag;
- std::string package;
-
- // -1 if acquireWakeLock() was used.
- int uid;
- };
-
// Details about a request passed to goToSleep().
struct SuspendRequest {
SuspendRequest(int64 uptime_ms, int reason, int flags);
@@ -108,8 +101,7 @@
int flags;
};
- using LockInfoMap = std::map<sp<IBinder>, LockInfo>;
- LockInfoMap locks_;
+ std::unique_ptr<WakeLockManagerStub> wake_lock_manager_;
// Information about calls to goToSleep(), in the order they were made.
using SuspendRequests = std::vector<SuspendRequest>;
