diff --git a/handler/mac/crash_report_upload_thread.cc b/handler/mac/crash_report_upload_thread.cc index 5333c78d..e266b7f7 100644 --- a/handler/mac/crash_report_upload_thread.cc +++ b/handler/mac/crash_report_upload_thread.cc @@ -19,7 +19,6 @@ #include #include -#include #include "base/logging.h" #include "base/memory/scoped_ptr.h" @@ -31,6 +30,7 @@ #include "util/net/http_body.h" #include "util/net/http_multipart_builder.h" #include "util/net/http_transport.h" +#include "util/stdlib/map_insert.h" namespace crashpad { @@ -39,13 +39,10 @@ namespace { void InsertOrReplaceMapEntry(std::map* map, const std::string& key, const std::string& value) { - auto it = map->find(key); - if (it != map->end()) { + std::string old_value; + if (!MapInsertOrReplace(map, key, value, &old_value)) { LOG(WARNING) << "duplicate key " << key << ", discarding value " - << it->second; - it->second = value; - } else { - map->insert(std::make_pair(key, value)); + << old_value; } } @@ -75,11 +72,9 @@ std::map BreakpadHTTPFormParametersFromMinidump( std::string list_annotations; for (const ModuleSnapshot* module : minidump_process_snapshot.Modules()) { for (const auto& kv : module->AnnotationsSimpleMap()) { - if (parameters.find(kv.first) != parameters.end()) { + if (!parameters.insert(kv).second) { LOG(WARNING) << "duplicate key " << kv.first << ", discarding value " << kv.second; - } else { - parameters.insert(kv); } } diff --git a/handler/mac/main.cc b/handler/mac/main.cc index 1df183f9..7f0629bb 100644 --- a/handler/mac/main.cc +++ b/handler/mac/main.cc @@ -18,7 +18,6 @@ #include #include -#include #include "base/files/file_path.h" #include "base/logging.h" @@ -30,6 +29,7 @@ #include "handler/mac/exception_handler_server.h" #include "util/mach/child_port_handshake.h" #include "util/posix/close_stdio.h" +#include "util/stdlib/map_insert.h" #include "util/stdlib/string_number_conversion.h" #include "util/string/split_string.h" #include "util/synchronization/semaphore.h" @@ -97,13 +97,10 @@ int HandlerMain(int argc, char* argv[]) { ToolSupport::UsageHint(me, "--annotation requires KEY=VALUE"); return EXIT_FAILURE; } - auto it = options.annotations.find(key); - if (it != options.annotations.end()) { + std::string old_value; + if (!MapInsertOrReplace(&options.annotations, key, value, &old_value)) { LOG(WARNING) << "duplicate key " << key << ", discarding value " - << it->second; - it->second = value; - } else { - options.annotations.insert(std::make_pair(key, value)); + << old_value; } break; } diff --git a/minidump/minidump_thread_id_map.cc b/minidump/minidump_thread_id_map.cc index 1fab698e..327d12e1 100644 --- a/minidump/minidump_thread_id_map.cc +++ b/minidump/minidump_thread_id_map.cc @@ -38,11 +38,10 @@ void BuildMinidumpThreadIDMap( uint64_t thread_id_64 = thread_snapshot->ThreadID(); if (thread_id_map->find(thread_id_64) == thread_id_map->end()) { uint32_t thread_id_32 = static_cast(thread_id_64); - if (thread_ids_32.find(thread_id_32) != thread_ids_32.end()) { + if (!thread_ids_32.insert(thread_id_32).second) { collision = true; break; } - thread_ids_32.insert(thread_id_32); thread_id_map->insert(std::make_pair(thread_id_64, thread_id_32)); } } diff --git a/snapshot/mac/mach_o_image_annotations_reader.cc b/snapshot/mac/mach_o_image_annotations_reader.cc index 4623aa63..6c6831bd 100644 --- a/snapshot/mac/mach_o_image_annotations_reader.cc +++ b/snapshot/mac/mach_o_image_annotations_reader.cc @@ -17,6 +17,8 @@ #include #include +#include + #include "base/logging.h" #include "client/crashpad_info.h" #include "client/simple_string_dictionary.h" @@ -161,10 +163,7 @@ void MachOImageAnnotationsReader::ReadCrashpadSimpleAnnotations( if (key_length) { std::string key(entry.key, key_length); std::string value(entry.value, strnlen(entry.value, sizeof(entry.value))); - if (!simple_map_annotations->count(key)) { - simple_map_annotations->insert( - std::pair(key, value)); - } else { + if (!simple_map_annotations->insert(std::make_pair(key, value)).second) { LOG(INFO) << "duplicate simple annotation " << key << " in " << name_; } } diff --git a/snapshot/mac/mach_o_image_reader.cc b/snapshot/mac/mach_o_image_reader.cc index f9da354d..cb85baab 100644 --- a/snapshot/mac/mach_o_image_reader.cc +++ b/snapshot/mac/mach_o_image_reader.cc @@ -20,6 +20,7 @@ #include #include +#include #include "base/logging.h" #include "base/strings/stringprintf.h" @@ -543,15 +544,15 @@ bool MachOImageReader::ReadSegmentCommand( // become inconsistent or require cleanup. const std::string segment_name = segment->Name(); - const auto& iterator = segment_map_.find(segment_name); - if (iterator != segment_map_.end()) { + const auto insert_result = + segment_map_.insert(std::make_pair(segment_name, segment_index)); + if (!insert_result.second) { LOG(WARNING) << base::StringPrintf("duplicate %s segment at %zu and %zu", segment_name.c_str(), - iterator->second, + insert_result.first->second, segment_index) << load_command_info; return false; } - segment_map_[segment_name] = segment_index; mach_vm_size_t vmsize = segment->vmsize(); diff --git a/snapshot/mac/mach_o_image_segment_reader.cc b/snapshot/mac/mach_o_image_segment_reader.cc index 6adcd6f2..8a077dde 100644 --- a/snapshot/mac/mach_o_image_segment_reader.cc +++ b/snapshot/mac/mach_o_image_segment_reader.cc @@ -16,6 +16,8 @@ #include +#include + #include "base/logging.h" #include "base/strings/stringprintf.h" #include "snapshot/mac/process_reader.h" @@ -185,14 +187,14 @@ bool MachOImageSegmentReader::Initialize(ProcessReader* process_reader, return false; } - const auto& iterator = section_map_.find(section_name); - if (iterator != section_map_.end()) { + const auto insert_result = + section_map_.insert(std::make_pair(section_name, section_index)); + if (!insert_result.second) { LOG(WARNING) << base::StringPrintf("duplicate section name at %zu", - iterator->second) << section_info; + insert_result.first->second) + << section_info; return false; } - - section_map_[section_name] = section_index; } INITIALIZATION_STATE_SET_VALID(initialized_); diff --git a/snapshot/mac/mach_o_image_symbol_table_reader.cc b/snapshot/mac/mach_o_image_symbol_table_reader.cc index 12d8de7e..949e4608 100644 --- a/snapshot/mac/mach_o_image_symbol_table_reader.cc +++ b/snapshot/mac/mach_o_image_symbol_table_reader.cc @@ -17,6 +17,8 @@ #include #include +#include + #include "base/memory/scoped_ptr.h" #include "base/strings/stringprintf.h" #include "util/mac/checked_mach_address_range.h" @@ -153,15 +155,14 @@ class MachOImageSymbolTableReaderInitializer { return false; } - if (external_defined_symbols->count(name)) { - LOG(WARNING) << "duplicate symbol " << name << symbol_info; - return false; - } - MachOImageSymbolTableReader::SymbolInformation this_symbol_info; this_symbol_info.value = symbol.n_value; this_symbol_info.section = symbol.n_sect; - (*external_defined_symbols)[name] = this_symbol_info; + if (!external_defined_symbols->insert( + std::make_pair(name, this_symbol_info)).second) { + LOG(WARNING) << "duplicate symbol " << name << symbol_info; + return false; + } } else { // External indirect symbols may be found in the portion of the symbol // table used for external symbols as opposed to indirect symbols when diff --git a/snapshot/minidump/minidump_simple_string_dictionary_reader.cc b/snapshot/minidump/minidump_simple_string_dictionary_reader.cc index 085c1f60..0d8301c1 100644 --- a/snapshot/minidump/minidump_simple_string_dictionary_reader.cc +++ b/snapshot/minidump/minidump_simple_string_dictionary_reader.cc @@ -72,12 +72,10 @@ bool ReadMinidumpSimpleStringDictionary( return false; } - if (local_dictionary.find(key) != local_dictionary.end()) { - LOG(WARNING) << "duplicate key " << key << ", discarding value " << value; + if (!local_dictionary.insert(std::make_pair(key, value)).second) { + LOG(ERROR) << "duplicate key " << key; return false; } - - local_dictionary.insert(std::make_pair(key, value)); } dictionary->swap(local_dictionary); diff --git a/snapshot/minidump/process_snapshot_minidump.cc b/snapshot/minidump/process_snapshot_minidump.cc index 256e429e..fa87981b 100644 --- a/snapshot/minidump/process_snapshot_minidump.cc +++ b/snapshot/minidump/process_snapshot_minidump.cc @@ -309,16 +309,13 @@ bool ProcessSnapshotMinidump::InitializeModulesCrashpadInfo( ++crashpad_module_index) { const MinidumpModuleCrashpadInfoLink& minidump_link = minidump_links[crashpad_module_index]; - if (module_crashpad_info_links->find( - minidump_link.minidump_module_list_index) != - module_crashpad_info_links->end()) { + if (!module_crashpad_info_links + ->insert(std::make_pair(minidump_link.minidump_module_list_index, + minidump_link.location)).second) { LOG(WARNING) << "duplicate module_crashpad_info_list minidump_module_list_index " << minidump_link.minidump_module_list_index; return false; - } else { - module_crashpad_info_links->insert(std::make_pair( - minidump_link.minidump_module_list_index, minidump_link.location)); } } diff --git a/tools/mac/run_with_crashpad.cc b/tools/mac/run_with_crashpad.cc index 9d6679cf..0b4653aa 100644 --- a/tools/mac/run_with_crashpad.cc +++ b/tools/mac/run_with_crashpad.cc @@ -27,6 +27,7 @@ #include "base/logging.h" #include "client/crashpad_client.h" #include "tools/tool_support.h" +#include "util/stdlib/map_insert.h" #include "util/string/split_string.h" namespace crashpad { @@ -118,13 +119,10 @@ int RunWithCrashpadMain(int argc, char* argv[]) { ToolSupport::UsageHint(me, "--annotation requires KEY=VALUE"); return EXIT_FAILURE; } - auto it = options.annotations.find(key); - if (it != options.annotations.end()) { + std::string old_value; + if (!MapInsertOrReplace(&options.annotations, key, value, &old_value)) { LOG(WARNING) << "duplicate key " << key << ", discarding value " - << it->second; - it->second = value; - } else { - options.annotations.insert(std::make_pair(key, value)); + << old_value; } break; } diff --git a/util/net/http_transport_test.cc b/util/net/http_transport_test.cc index ddda100e..375ccfc3 100644 --- a/util/net/http_transport_test.cc +++ b/util/net/http_transport_test.cc @@ -222,7 +222,7 @@ TEST(HTTPTransport, ValidFormData) { builder.SetFormData("key2", "--abcdefg123"); HTTPHeaders headers; - headers.insert(builder.GetContentType()); + EXPECT_TRUE(headers.insert(builder.GetContentType()).second); HTTPTransportTestFixture test(headers, builder.GetBodyStream(), 200, &ValidFormData); diff --git a/util/stdlib/map_insert.h b/util/stdlib/map_insert.h new file mode 100644 index 00000000..956271b3 --- /dev/null +++ b/util/stdlib/map_insert.h @@ -0,0 +1,56 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_UTIL_STDLIB_MAP_INSERT_H_ +#define CRASHPAD_UTIL_STDLIB_MAP_INSERT_H_ + +#include +#include + +namespace crashpad { + +//! \brief Inserts a mapping from \a key to \a value into \a map, or replaces +//! an existing mapping so that \a key maps to \value. +//! +//! This behaves similarly to `std::map<>::insert_or_assign()` proposed for +//! C++17, except that the \a old_value parameter is added. +//! +//! \param[inout] map The map to operate on. +//! \param[in] key The key that should be mapped to \a value. +//! \param[in] value The value that \a key should map to. +//! \param[out] old_value If \a key was previously present in \a map, this will +//! be set to its previous value. This parameter is optional and may be +//! `nullptr` if this information is not required. +//! +//! \return `false` if \a key was previously present in \a map. If \a old_value +//! is not `nullptr`, it will be set to the previous value. `true` if \a +//! key was not present in the map and was inserted. +template +bool MapInsertOrReplace(T* map, + const typename T::key_type& key, + const typename T::mapped_type& value, + typename T::mapped_type* old_value) { + const auto result = map->insert(std::make_pair(key, value)); + if (!result.second) { + if (old_value) { + *old_value = result.first->second; + } + result.first->second = value; + } + return result.second; +} + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_STDLIB_MAP_INSERT_H_ diff --git a/util/stdlib/map_insert_test.cc b/util/stdlib/map_insert_test.cc new file mode 100644 index 00000000..d3197b96 --- /dev/null +++ b/util/stdlib/map_insert_test.cc @@ -0,0 +1,54 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "util/stdlib/map_insert.h" + +#include + +#include "gtest/gtest.h" + +namespace crashpad { +namespace test { +namespace { + +TEST(MapInsert, MapInsertOrReplace) { + std::map map; + int old_value; + EXPECT_TRUE(MapInsertOrReplace(&map, "key", 1, &old_value)); + std::map expect_map; + expect_map["key"] = 1; + EXPECT_EQ(expect_map, map); + + EXPECT_FALSE(MapInsertOrReplace(&map, "key", 2, &old_value)); + EXPECT_EQ(1, old_value); + expect_map["key"] = 2; + EXPECT_EQ(expect_map, map); + + EXPECT_TRUE(MapInsertOrReplace(&map, "another", 3, &old_value)); + expect_map["another"] = 3; + EXPECT_EQ(expect_map, map); + + // Make sure nullptr is accepted as old_value. + EXPECT_TRUE(MapInsertOrReplace(&map, "yet another", 5, nullptr)); + expect_map["yet another"] = 5; + EXPECT_EQ(expect_map, map); + + EXPECT_FALSE(MapInsertOrReplace(&map, "yet another", 6, nullptr)); + expect_map["yet another"] = 6; + EXPECT_EQ(expect_map, map); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/util.gyp b/util/util.gyp index e94377cf..404d69c8 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -121,6 +121,7 @@ 'posix/symbolic_constants_posix.cc', 'posix/symbolic_constants_posix.h', 'stdlib/cxx.h', + 'stdlib/map_insert.h', 'stdlib/objc.h', 'stdlib/pointer_container.h', 'stdlib/string_number_conversion.cc', @@ -319,6 +320,7 @@ 'numeric/int128_test.cc', 'posix/process_info_test.cc', 'posix/symbolic_constants_posix_test.cc', + 'stdlib/map_insert_test.cc', 'stdlib/string_number_conversion_test.cc', 'stdlib/strlcpy_test.cc', 'stdlib/strnlen_test.cc',