dbus: CHECK if a response is not sent after handling calls
This CL wraps ExportedObject::SendResponse in a helper to ensure that it is invoked before destruction. Bug: 400758194 Change-Id: I6515f164a0e2594e7313d6e7e0a5cdfe9fa2af94 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6325811 Commit-Queue: Xiyuan Xia <xiyuan@chromium.org> Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org> Reviewed-by: Michael Sun <michaelfsun@google.com> Cr-Commit-Position: refs/heads/main@{#1429663}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
b80cb987fa
commit
b90214d46d
@ -82,6 +82,7 @@ test("dbus_unittests") {
|
||||
"dbus_statistics_unittest.cc",
|
||||
"end_to_end_async_unittest.cc",
|
||||
"end_to_end_sync_unittest.cc",
|
||||
"exported_object_unittest.cc",
|
||||
"message_unittest.cc",
|
||||
"mock_unittest.cc",
|
||||
"object_manager_unittest.cc",
|
||||
|
@ -5,9 +5,13 @@
|
||||
#include "dbus/exported_object.h"
|
||||
|
||||
#include <stdint.h>
|
||||
|
||||
#include <string>
|
||||
#include <utility>
|
||||
|
||||
#include "base/check.h"
|
||||
#include "base/containers/contains.h"
|
||||
#include "base/debug/crash_logging.h"
|
||||
#include "base/functional/bind.h"
|
||||
#include "base/logging.h"
|
||||
#include "base/memory/ref_counted.h"
|
||||
@ -23,6 +27,59 @@
|
||||
|
||||
namespace dbus {
|
||||
|
||||
namespace {
|
||||
|
||||
// Helper to ensure the wrapped `ResponseSender` is invoked. To use it, use the
|
||||
// factory method to create wrapping callback owning an instance of this class.
|
||||
// The wrapping callback must run. Otherwise, CHECK in destructor will trigger.
|
||||
class ResponseSenderWrapper {
|
||||
public:
|
||||
ResponseSenderWrapper(std::string interface,
|
||||
std::string member,
|
||||
ExportedObject::ResponseSender sender)
|
||||
: interface_(std::move(interface)),
|
||||
member_(std::move(member)),
|
||||
sender_(std::move(sender)) {}
|
||||
|
||||
ResponseSenderWrapper(ResponseSenderWrapper&) = delete;
|
||||
ResponseSenderWrapper& operator&(ResponseSenderWrapper&) = delete;
|
||||
|
||||
~ResponseSenderWrapper() {
|
||||
SCOPED_CRASH_KEY_STRING32("ResponseSenderWrapper", "DBusInterface",
|
||||
interface_);
|
||||
SCOPED_CRASH_KEY_STRING32("ResponseSenderWrapper", "DBusMember", member_);
|
||||
|
||||
// `sender_` must have run (or not set).
|
||||
CHECK(sender_.is_null())
|
||||
<< "ResponseSender did not run for " << interface_ << "." << member_;
|
||||
;
|
||||
}
|
||||
|
||||
// Convenience factory.
|
||||
static ExportedObject::ResponseSender Create(
|
||||
std::string interface,
|
||||
std::string member,
|
||||
ExportedObject::ResponseSender sender) {
|
||||
return base::BindOnce(
|
||||
&ResponseSenderWrapper::RunSender,
|
||||
std::make_unique<ResponseSenderWrapper>(
|
||||
std::move(interface), std::move(member), std::move(sender)));
|
||||
}
|
||||
|
||||
private:
|
||||
void RunSender(std::unique_ptr<Response> response) {
|
||||
CHECK(sender_);
|
||||
|
||||
std::move(sender_).Run(std::move(response));
|
||||
}
|
||||
|
||||
const std::string interface_;
|
||||
const std::string member_;
|
||||
ExportedObject::ResponseSender sender_;
|
||||
};
|
||||
|
||||
} // namespace
|
||||
|
||||
ExportedObject::ExportedObject(Bus* bus,
|
||||
const ObjectPath& object_path)
|
||||
: bus_(bus),
|
||||
@ -229,8 +286,8 @@ DBusHandlerResult ExportedObject::HandleMessage(
|
||||
dbus_message_ref(raw_message);
|
||||
std::unique_ptr<MethodCall> method_call(
|
||||
MethodCall::FromRawMessage(raw_message));
|
||||
const std::string interface = method_call->GetInterface();
|
||||
const std::string member = method_call->GetMember();
|
||||
std::string interface = method_call->GetInterface();
|
||||
std::string member = method_call->GetMember();
|
||||
|
||||
if (interface.empty()) {
|
||||
// We don't support method calls without interface.
|
||||
@ -256,8 +313,10 @@ DBusHandlerResult ExportedObject::HandleMessage(
|
||||
} else {
|
||||
// If the D-Bus thread is not used, just call the method directly.
|
||||
MethodCall* method = method_call.get();
|
||||
iter->second.Run(method, base::BindOnce(&ExportedObject::SendResponse, this,
|
||||
std::move(method_call)));
|
||||
iter->second.Run(method, ResponseSenderWrapper::Create(
|
||||
std::move(interface), std::move(member),
|
||||
base::BindOnce(&ExportedObject::SendResponse,
|
||||
this, std::move(method_call))));
|
||||
}
|
||||
|
||||
// It's valid to say HANDLED here, and send a method response at a later
|
||||
@ -269,9 +328,11 @@ void ExportedObject::RunMethod(const MethodCallCallback& method_call_callback,
|
||||
std::unique_ptr<MethodCall> method_call) {
|
||||
bus_->AssertOnOriginThread();
|
||||
MethodCall* method = method_call.get();
|
||||
method_call_callback.Run(method,
|
||||
base::BindOnce(&ExportedObject::SendResponse, this,
|
||||
std::move(method_call)));
|
||||
method_call_callback.Run(
|
||||
method, ResponseSenderWrapper::Create(
|
||||
method->GetInterface(), method->GetMember(),
|
||||
base::BindOnce(&ExportedObject::SendResponse, this,
|
||||
std::move(method_call))));
|
||||
}
|
||||
|
||||
void ExportedObject::SendResponse(std::unique_ptr<MethodCall> method_call,
|
||||
|
72
dbus/exported_object_unittest.cc
Normal file
72
dbus/exported_object_unittest.cc
Normal file
@ -0,0 +1,72 @@
|
||||
// Copyright 2025 The Chromium Authors
|
||||
// Use of this source code is governed by a BSD-style license that can be
|
||||
// found in the LICENSE file.
|
||||
|
||||
#include "dbus/exported_object.h"
|
||||
|
||||
#include "base/memory/ref_counted.h"
|
||||
#include "base/run_loop.h"
|
||||
#include "base/test/bind.h"
|
||||
#include "base/test/task_environment.h"
|
||||
#include "dbus/bus.h"
|
||||
#include "dbus/message.h"
|
||||
#include "dbus/object_proxy.h"
|
||||
#include "dbus/test_service.h"
|
||||
#include "testing/gtest/include/gtest/gtest.h"
|
||||
|
||||
namespace dbus {
|
||||
namespace {
|
||||
|
||||
class ExportedObjectTest : public testing::Test {
|
||||
protected:
|
||||
ExportedObjectTest() = default;
|
||||
|
||||
void SetUp() override {
|
||||
Bus::Options bus_options;
|
||||
bus_options.bus_type = Bus::SESSION;
|
||||
bus_options.connection_type = Bus::PRIVATE;
|
||||
bus_ = new Bus(bus_options);
|
||||
}
|
||||
|
||||
void TearDown() override { bus_->ShutdownAndBlock(); }
|
||||
|
||||
base::test::TaskEnvironment task_environment_{
|
||||
base::test::TaskEnvironment::MainThreadType::IO};
|
||||
|
||||
scoped_refptr<Bus> bus_;
|
||||
};
|
||||
|
||||
// Tests that calling a method that doesn't send a response crashes.
|
||||
TEST_F(ExportedObjectTest, NotSendingResponseCrash) {
|
||||
TestService::Options options;
|
||||
TestService test_service(options);
|
||||
ObjectProxy* object_proxy = bus_->GetObjectProxy(
|
||||
test_service.service_name(), ObjectPath("/org/chromium/TestObject"));
|
||||
|
||||
base::RunLoop run_loop;
|
||||
object_proxy->WaitForServiceToBeAvailable(
|
||||
base::BindLambdaForTesting([&](bool service_available) {
|
||||
ASSERT_TRUE(service_available);
|
||||
run_loop.Quit();
|
||||
}));
|
||||
|
||||
ASSERT_TRUE(test_service.StartService());
|
||||
test_service.WaitUntilServiceIsStarted();
|
||||
ASSERT_TRUE(test_service.has_ownership());
|
||||
|
||||
// Spin a loop and wait for `TestService` to be available.
|
||||
run_loop.Run();
|
||||
|
||||
// Call the bad method and expect a CHECK crash.
|
||||
MethodCall method_call("org.chromium.TestInterface",
|
||||
"NotSendingResponseCrash");
|
||||
base::expected<std::unique_ptr<Response>, Error> result;
|
||||
EXPECT_DEATH_IF_SUPPORTED(
|
||||
result = object_proxy->CallMethodAndBlock(
|
||||
&method_call, ObjectProxy::TIMEOUT_USE_DEFAULT),
|
||||
"ResponseSender did not run for "
|
||||
"org.chromium.TestInterface.NotSendingResponseCrash");
|
||||
}
|
||||
|
||||
} // namespace
|
||||
} // namespace dbus
|
@ -29,8 +29,8 @@
|
||||
namespace dbus {
|
||||
|
||||
// Echo, SlowEcho, AsyncEcho, BrokenMethod, GetAll, Get, Set, PerformAction,
|
||||
// GetManagedObjects
|
||||
const int TestService::kNumMethodsToExport = 9;
|
||||
// GetManagedObjects, NotSendingResponseCrash.
|
||||
constexpr int TestService::kNumMethodsToExport = 10;
|
||||
|
||||
TestService::Options::Options()
|
||||
: request_ownership_options(Bus::REQUIRE_PRIMARY) {
|
||||
@ -259,6 +259,13 @@ void TestService::Run(base::RunLoop* run_loop) {
|
||||
base::BindOnce(&TestService::OnExported, base::Unretained(this)));
|
||||
++num_methods;
|
||||
|
||||
exported_object_->ExportMethod(
|
||||
"org.chromium.TestInterface", "NotSendingResponseCrash",
|
||||
base::BindRepeating(&TestService::NotSendingResponseCrash,
|
||||
base::Unretained(this)),
|
||||
base::BindOnce(&TestService::OnExported, base::Unretained(this)));
|
||||
++num_methods;
|
||||
|
||||
// Just print an error message as we don't want to crash tests.
|
||||
// Tests will fail at a call to WaitUntilServiceIsStarted().
|
||||
if (num_methods != kNumMethodsToExport) {
|
||||
@ -490,7 +497,6 @@ void TestService::OwnershipReleased(
|
||||
std::move(response_sender)));
|
||||
}
|
||||
|
||||
|
||||
void TestService::OwnershipRegained(
|
||||
MethodCall* method_call,
|
||||
ExportedObject::ResponseSender response_sender,
|
||||
@ -498,6 +504,11 @@ void TestService::OwnershipRegained(
|
||||
PerformActionResponse(method_call, std::move(response_sender));
|
||||
}
|
||||
|
||||
void TestService::NotSendingResponseCrash(
|
||||
MethodCall* method_call,
|
||||
dbus::ExportedObject::ResponseSender response_sender) {
|
||||
// Not invoking `response_sender` and CHECK crash.
|
||||
}
|
||||
|
||||
void TestService::GetManagedObjects(
|
||||
MethodCall* method_call,
|
||||
|
@ -208,6 +208,12 @@ class TestService : public base::Thread {
|
||||
dbus::ExportedObject::ResponseSender response_sender,
|
||||
bool success);
|
||||
|
||||
// Simulate DBus service that uses ExportedObject but forgets to send a
|
||||
// response. This would trigger a CHECK crash.
|
||||
void NotSendingResponseCrash(
|
||||
MethodCall* method_call,
|
||||
dbus::ExportedObject::ResponseSender response_sender);
|
||||
|
||||
// Name of this service.
|
||||
std::string service_name_;
|
||||
|
||||
|
@ -274,16 +274,12 @@ void BluetoothGattDescriptorServiceProviderImpl::ReadValue(
|
||||
// the delegate, which should know how to handle it.
|
||||
}
|
||||
|
||||
// GetValue() promises to only call either the success or error callback.
|
||||
auto split_response_sender =
|
||||
base::SplitOnceCallback(std::move(response_sender));
|
||||
|
||||
DCHECK(delegate_);
|
||||
delegate_->GetValue(
|
||||
device_path,
|
||||
base::BindOnce(&BluetoothGattDescriptorServiceProviderImpl::OnReadValue,
|
||||
weak_ptr_factory_.GetWeakPtr(), method_call,
|
||||
std::move(split_response_sender.first)));
|
||||
std::move(response_sender)));
|
||||
}
|
||||
|
||||
void BluetoothGattDescriptorServiceProviderImpl::WriteValue(
|
||||
|
Reference in New Issue
Block a user