From 38715910ea552ad5198b6dba8bca3a57f46c1e9a Mon Sep 17 00:00:00 2001 From: "derat@chromium.org" Date: Mon, 24 Feb 2014 15:59:40 +0000 Subject: [PATCH] chromeos: Make dbus::MessageReader memory ownership explicit Make memory returned by MessageReader::PopArrayOfBytes() const to make it clearer that ownership remains with the MessageReader. Also update PopArrayOfStrings() and PopArrayOfObjectPaths() to clear the passed-in vectors before appending to them. BUG=none TBR=isherman@chromium.org,mvanouwerkerk@chromium.org Review URL: https://codereview.chromium.org/176693003 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@252922 0039d316-1c4b-4281-b951-d872f2087c98 --- .../password_manager/native_backend_kwallet_x.cc | 6 +++--- .../native_backend_kwallet_x_unittest.cc | 2 +- chromeos/dbus/cryptohome_client.cc | 12 ++++++------ chromeos/dbus/debug_daemon_client.cc | 6 ++---- chromeos/dbus/session_manager_client.cc | 4 ++-- content/browser/geolocation/wifi_data_provider_linux.cc | 2 +- dbus/message.cc | 13 ++++++++----- dbus/message.h | 17 ++++++++++------- dbus/message_unittest.cc | 4 ++-- .../media_transfer_protocol_daemon_client.cc | 2 +- 10 files changed, 36 insertions(+), 32 deletions(-) diff --git a/chrome/browser/password_manager/native_backend_kwallet_x.cc b/chrome/browser/password_manager/native_backend_kwallet_x.cc index c915a548836f..d66e68728286 100644 --- a/chrome/browser/password_manager/native_backend_kwallet_x.cc +++ b/chrome/browser/password_manager/native_backend_kwallet_x.cc @@ -392,7 +392,7 @@ bool NativeBackendKWallet::RemoveLoginsCreatedBetween( continue; } dbus::MessageReader reader(response.get()); - uint8_t* bytes = NULL; + const uint8_t* bytes = NULL; size_t length = 0; if (!reader.PopArrayOfBytes(&bytes, &length)) { LOG(ERROR) << "Error reading response from kwalletd (readEntry): " @@ -502,7 +502,7 @@ bool NativeBackendKWallet::GetLoginsList(PasswordFormList* forms, return false; } dbus::MessageReader reader(response.get()); - uint8_t* bytes = NULL; + const uint8_t* bytes = NULL; size_t length = 0; if (!reader.PopArrayOfBytes(&bytes, &length)) { LOG(ERROR) << "Error reading response from kwalletd (readEntry): " @@ -610,7 +610,7 @@ bool NativeBackendKWallet::GetAllLogins(PasswordFormList* forms, continue; } dbus::MessageReader reader(response.get()); - uint8_t* bytes = NULL; + const uint8_t* bytes = NULL; size_t length = 0; if (!reader.PopArrayOfBytes(&bytes, &length)) { LOG(ERROR) << "Error reading response from kwalletd (readEntry): " diff --git a/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc b/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc index f425e33ccaae..438663c80347 100644 --- a/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc +++ b/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc @@ -420,7 +420,7 @@ dbus::Response* NativeBackendKWalletTest::KWalletMethodCall( int handle = NativeBackendKWalletStub::kInvalidKWalletHandle; std::string folder_name; std::string key; - uint8_t* bytes = NULL; + const uint8_t* bytes = NULL; size_t length = 0; std::string app_name; EXPECT_TRUE(reader.PopInt32(&handle)); diff --git a/chromeos/dbus/cryptohome_client.cc b/chromeos/dbus/cryptohome_client.cc index f0bbd18f5dbf..c58ea2098c67 100644 --- a/chromeos/dbus/cryptohome_client.cc +++ b/chromeos/dbus/cryptohome_client.cc @@ -357,7 +357,7 @@ class CryptohomeClientImpl : public CryptohomeClient { if (!response.get()) return false; dbus::MessageReader reader(response.get()); - uint8* bytes = NULL; + const uint8* bytes = NULL; size_t length = 0; if (!reader.PopArrayOfBytes(&bytes, &length) || !reader.PopBool(successful)) @@ -737,7 +737,7 @@ class CryptohomeClientImpl : public CryptohomeClient { return; } dbus::MessageReader reader(response); - uint8* bytes = NULL; + const uint8* bytes = NULL; size_t length = 0; if (!reader.PopArrayOfBytes(&bytes, &length)) { callback.Run(DBUS_METHOD_CALL_FAILURE, std::vector()); @@ -827,7 +827,7 @@ class CryptohomeClientImpl : public CryptohomeClient { return; } dbus::MessageReader reader(response); - uint8* data_buffer = NULL; + const uint8* data_buffer = NULL; size_t data_length = 0; bool result = false; if (!reader.PopArrayOfBytes(&data_buffer, &data_length) || @@ -835,7 +835,7 @@ class CryptohomeClientImpl : public CryptohomeClient { callback.Run(DBUS_METHOD_CALL_FAILURE, false, std::string()); return; } - std::string data(reinterpret_cast(data_buffer), data_length); + std::string data(reinterpret_cast(data_buffer), data_length); callback.Run(DBUS_METHOD_CALL_SUCCESS, result, data); } @@ -900,7 +900,7 @@ class CryptohomeClientImpl : public CryptohomeClient { dbus::MessageReader reader(signal); int async_id = 0; bool return_status = false; - uint8* return_data_buffer = NULL; + const uint8* return_data_buffer = NULL; size_t return_data_length = 0; if (!reader.PopInt32(&async_id) || !reader.PopBool(&return_status) || @@ -909,7 +909,7 @@ class CryptohomeClientImpl : public CryptohomeClient { return; } if (!async_call_status_data_handler_.is_null()) { - std::string return_data(reinterpret_cast(return_data_buffer), + std::string return_data(reinterpret_cast(return_data_buffer), return_data_length); async_call_status_data_handler_.Run(async_id, return_status, return_data); } diff --git a/chromeos/dbus/debug_daemon_client.cc b/chromeos/dbus/debug_daemon_client.cc index 9d06e5f72100..25a55e331f75 100644 --- a/chromeos/dbus/debug_daemon_client.cc +++ b/chromeos/dbus/debug_daemon_client.cc @@ -514,12 +514,10 @@ class DebugDaemonClientImpl : public DebugDaemonClient { } dbus::MessageReader reader(response); - uint8* buffer = NULL; + const uint8* buffer = NULL; size_t buf_size = 0; - if (!reader.PopArrayOfBytes(reinterpret_cast( - &buffer), &buf_size)) { + if (!reader.PopArrayOfBytes(&buffer, &buf_size)) return; - } // TODO(asharif): Figure out a way to avoid this copy. data.insert(data.end(), buffer, buffer + buf_size); diff --git a/chromeos/dbus/session_manager_client.cc b/chromeos/dbus/session_manager_client.cc index 7def286cda74..c01f28f75900 100644 --- a/chromeos/dbus/session_manager_client.cc +++ b/chromeos/dbus/session_manager_client.cc @@ -403,14 +403,14 @@ class SessionManagerClientImpl : public SessionManagerClient { return; } dbus::MessageReader reader(response); - uint8* values = NULL; + const uint8* values = NULL; size_t length = 0; if (!reader.PopArrayOfBytes(&values, &length)) { LOG(ERROR) << "Invalid response: " << response->ToString(); return; } // static_cast does not work due to signedness. - extracted->assign(reinterpret_cast(values), length); + extracted->assign(reinterpret_cast(values), length); } // Called when kSessionManagerRetrievePolicy or diff --git a/content/browser/geolocation/wifi_data_provider_linux.cc b/content/browser/geolocation/wifi_data_provider_linux.cc index 1e4365822e4b..5838c048e35d 100644 --- a/content/browser/geolocation/wifi_data_provider_linux.cc +++ b/content/browser/geolocation/wifi_data_provider_linux.cc @@ -246,7 +246,7 @@ bool NetworkManagerWlanApi::GetAccessPointsForAdapter( << ": " << response->ToString(); continue; } - uint8* ssid_bytes = NULL; + const uint8* ssid_bytes = NULL; size_t ssid_length = 0; if (!variant_reader.PopArrayOfBytes(&ssid_bytes, &ssid_length)) { LOG(WARNING) << "Unexpected response for " << access_point_path.value() diff --git a/dbus/message.cc b/dbus/message.cc index 918b86009045..eaf3c9bed9c4 100644 --- a/dbus/message.cc +++ b/dbus/message.cc @@ -807,7 +807,7 @@ bool MessageReader::PopVariant(MessageReader* sub_reader) { return PopContainer(DBUS_TYPE_VARIANT, sub_reader); } -bool MessageReader::PopArrayOfBytes(uint8** bytes, size_t* length) { +bool MessageReader::PopArrayOfBytes(const uint8** bytes, size_t* length) { MessageReader array_reader(message_); if (!PopArray(&array_reader)) return false; @@ -829,9 +829,10 @@ bool MessageReader::PopArrayOfBytes(uint8** bytes, size_t* length) { bool MessageReader::PopArrayOfStrings( std::vector *strings) { + strings->clear(); MessageReader array_reader(message_); if (!PopArray(&array_reader)) - return false; + return false; while (array_reader.HasMoreData()) { std::string string; if (!array_reader.PopString(&string)) @@ -843,9 +844,10 @@ bool MessageReader::PopArrayOfStrings( bool MessageReader::PopArrayOfObjectPaths( std::vector *object_paths) { + object_paths->clear(); MessageReader array_reader(message_); if (!PopArray(&array_reader)) - return false; + return false; while (array_reader.HasMoreData()) { ObjectPath object_path; if (!array_reader.PopObjectPath(&object_path)) @@ -858,9 +860,10 @@ bool MessageReader::PopArrayOfObjectPaths( bool MessageReader::PopArrayOfBytesAsProto( google::protobuf::MessageLite* protobuf) { DCHECK(protobuf != NULL); - char* serialized_buf = NULL; + const char* serialized_buf = NULL; size_t buf_size = 0; - if (!PopArrayOfBytes(reinterpret_cast(&serialized_buf), &buf_size)) { + if (!PopArrayOfBytes( + reinterpret_cast(&serialized_buf), &buf_size)) { LOG(ERROR) << "Error reading array of bytes"; return false; } diff --git a/dbus/message.h b/dbus/message.h index 54ca036cceb9..db3456acd0cd 100644 --- a/dbus/message.h +++ b/dbus/message.h @@ -408,12 +408,14 @@ class CHROME_DBUS_EXPORT MessageReader { // Arrays of bytes are often used for exchanging binary blobs hence it's // worth having a specialized function. // - // |bytes| must be copied if the contents will be referenced after the - // MessageReader is destroyed. - bool PopArrayOfBytes(uint8** bytes, size_t* length); - - // Gets the array of strings at the current iterator position. - // Returns true and advances the iterator on success. + // Ownership of the memory pointed to by |bytes| remains with the + // MessageReader; |bytes| must be copied if the contents will be referenced + // after the MessageReader is destroyed. + bool PopArrayOfBytes(const uint8** bytes, size_t* length); + + // Gets the array of strings at the current iterator position. |strings| is + // cleared before being modified. Returns true and advances the iterator on + // success. // // Arrays of strings are often used to communicate with D-Bus // services like KWallet, hence it's worth having a specialized @@ -421,7 +423,8 @@ class CHROME_DBUS_EXPORT MessageReader { bool PopArrayOfStrings(std::vector* strings); // Gets the array of object paths at the current iterator position. - // Returns true and advances the iterator on success. + // |object_paths| is cleared before being modified. Returns true and advances + // the iterator on success. // // Arrays of object paths are often used to communicate with D-Bus // services like NetworkManager, hence it's worth having a specialized diff --git a/dbus/message_unittest.cc b/dbus/message_unittest.cc index 16348df5826a..0c944d9d6171 100644 --- a/dbus/message_unittest.cc +++ b/dbus/message_unittest.cc @@ -208,7 +208,7 @@ TEST(MessageTest, ArrayOfBytes) { writer.AppendArrayOfBytes(bytes.data(), bytes.size()); MessageReader reader(message.get()); - uint8* output_bytes = NULL; + const uint8* output_bytes = NULL; size_t length = 0; ASSERT_TRUE(reader.PopArrayOfBytes(&output_bytes, &length)); ASSERT_FALSE(reader.HasMoreData()); @@ -225,7 +225,7 @@ TEST(MessageTest, ArrayOfBytes_Empty) { writer.AppendArrayOfBytes(bytes.data(), bytes.size()); MessageReader reader(message.get()); - uint8* output_bytes = NULL; + const uint8* output_bytes = NULL; size_t length = 0; ASSERT_TRUE(reader.PopArrayOfBytes(&output_bytes, &length)); ASSERT_FALSE(reader.HasMoreData()); diff --git a/device/media_transfer_protocol/media_transfer_protocol_daemon_client.cc b/device/media_transfer_protocol/media_transfer_protocol_daemon_client.cc index 41ac37e0c8bf..5df7ef00c098 100644 --- a/device/media_transfer_protocol/media_transfer_protocol_daemon_client.cc +++ b/device/media_transfer_protocol/media_transfer_protocol_daemon_client.cc @@ -349,7 +349,7 @@ class MediaTransferProtocolDaemonClientImpl return; } - uint8* data_bytes = NULL; + const uint8* data_bytes = NULL; size_t data_length = 0; dbus::MessageReader reader(response); if (!reader.PopArrayOfBytes(&data_bytes, &data_length)) { -- 2.11.4.GIT