From 4f09b58d1fbd4be3eb8cef2d1682c37854f90df6 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 16 Nov 2015 13:39:01 -0500 Subject: [PATCH] Add RandomString() and its test, and use it everywhere it makes sense This unifies several things that used a 16-character random string, and a few other users of random identifiers where it also made sense to use a 16-character random string. TEST=crashpad_util_test RandomString.RandomString R=scottmg@chromium.org Review URL: https://codereview.chromium.org/1451793002 . --- test/mac/mach_multiprocess.cc | 6 +-- test/scoped_temp_dir_win.cc | 4 +- util/mac/service_management_test.mm | 7 +-- util/mach/child_port_handshake.cc | 5 +- util/mach/mach_extensions_test.cc | 6 +-- util/misc/random_string.cc | 29 +++++++++++ util/misc/random_string.h | 31 +++++++++++ util/misc/random_string_test.cc | 71 ++++++++++++++++++++++++++ util/net/http_transport_test.cc | 13 ++--- util/net/http_transport_test_server.py | 6 +-- util/util.gyp | 2 + util/util_test.gyp | 1 + util/win/exception_handler_server.cc | 6 +-- util/win/process_info_test.cc | 6 +-- 14 files changed, 158 insertions(+), 35 deletions(-) create mode 100644 util/misc/random_string.cc create mode 100644 util/misc/random_string.h create mode 100644 util/misc/random_string_test.cc diff --git a/test/mac/mach_multiprocess.cc b/test/mac/mach_multiprocess.cc index 0458ef3..641eeba 100644 --- a/test/mac/mach_multiprocess.cc +++ b/test/mac/mach_multiprocess.cc @@ -23,7 +23,6 @@ #include "base/logging.h" #include "base/mac/scoped_mach_port.h" #include "base/memory/scoped_ptr.h" -#include "base/rand_util.h" #include "gtest/gtest.h" #include "test/errors.h" #include "test/mac/mach_errors.h" @@ -31,6 +30,7 @@ #include "util/mach/mach_extensions.h" #include "util/mach/mach_message.h" #include "util/misc/implicit_cast.h" +#include "util/misc/random_string.h" #include "util/misc/scoped_forbid_return.h" namespace { @@ -93,9 +93,7 @@ void MachMultiprocess::PreFork() { // forking, so that it’s guaranteed to be there when the child attempts to // look it up. info_->service_name = "org.chromium.crashpad.test.mach_multiprocess."; - for (int index = 0; index < 16; ++index) { - info_->service_name.append(1, base::RandInt('A', 'Z')); - } + info_->service_name.append(RandomString()); info_->local_port = BootstrapCheckIn(info_->service_name); ASSERT_TRUE(info_->local_port.is_valid()); diff --git a/test/scoped_temp_dir_win.cc b/test/scoped_temp_dir_win.cc index 14674d9..cc4820d 100644 --- a/test/scoped_temp_dir_win.cc +++ b/test/scoped_temp_dir_win.cc @@ -17,10 +17,10 @@ #include #include "base/logging.h" -#include "base/rand_util.h" #include "base/strings/string16.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" +#include "util/misc/random_string.h" #include "gtest/gtest.h" namespace crashpad { @@ -34,7 +34,7 @@ base::FilePath GenerateCandidateName() { PCHECK(path_len != 0) << "GetTempPath"; base::FilePath system_temp_dir(temp_path); base::string16 new_dir_name = base::UTF8ToUTF16(base::StringPrintf( - "crashpad.test.%d.%I64x", GetCurrentProcessId(), base::RandUint64())); + "crashpad.test.%d.%s", GetCurrentProcessId(), RandomString().c_str())); return system_temp_dir.Append(new_dir_name); } diff --git a/util/mac/service_management_test.mm b/util/mac/service_management_test.mm index 9f08b57..cdfd082 100644 --- a/util/mac/service_management_test.mm +++ b/util/mac/service_management_test.mm @@ -24,9 +24,9 @@ #include "base/mac/scoped_cftyperef.h" #include "base/strings/stringprintf.h" #include "base/strings/sys_string_conversions.h" -#include "base/rand_util.h" #include "gtest/gtest.h" #include "util/misc/clock.h" +#include "util/misc/random_string.h" #include "util/posix/process_info.h" #include "util/stdlib/objc.h" @@ -108,10 +108,7 @@ void ExpectProcessIsNotRunning(pid_t pid, std::string& last_arg) { TEST(ServiceManagement, SubmitRemoveJob) { @autoreleasepool { - std::string cookie; - for (int index = 0; index < 16; ++index) { - cookie.append(1, base::RandInt('A', 'Z')); - } + const std::string cookie = RandomString(); std::string shell_script = base::StringPrintf("sleep 10; echo %s", cookie.c_str()); diff --git a/util/mach/child_port_handshake.cc b/util/mach/child_port_handshake.cc index ff779df..e7cc7ee 100644 --- a/util/mach/child_port_handshake.cc +++ b/util/mach/child_port_handshake.cc @@ -37,6 +37,7 @@ #include "util/mach/mach_message.h" #include "util/mach/mach_message_server.h" #include "util/misc/implicit_cast.h" +#include "util/misc/random_string.h" namespace crashpad { namespace { @@ -97,10 +98,10 @@ mach_port_t ChildPortHandshakeServer::RunServer( errno = pthread_threadid_np(pthread_self(), &thread_id); PCHECK(errno == 0) << "pthread_threadid_np"; std::string service_name = base::StringPrintf( - "org.chromium.crashpad.child_port_handshake.%d.%llu.%016llx", + "org.chromium.crashpad.child_port_handshake.%d.%llu.%s", getpid(), thread_id, - base::RandUint64()); + RandomString().c_str()); // Check the new service in with the bootstrap server, obtaining a receive // right for it. diff --git a/util/mach/mach_extensions_test.cc b/util/mach/mach_extensions_test.cc index 99ddee9..358f235 100644 --- a/util/mach/mach_extensions_test.cc +++ b/util/mach/mach_extensions_test.cc @@ -15,10 +15,10 @@ #include "util/mach/mach_extensions.h" #include "base/mac/scoped_mach_port.h" -#include "base/rand_util.h" #include "gtest/gtest.h" #include "test/mac/mach_errors.h" #include "util/mac/mac_util.h" +#include "util/misc/random_string.h" namespace crashpad { namespace test { @@ -139,9 +139,7 @@ TEST(MachExtensions, BootstrapCheckInAndLookUp) { EXPECT_NE(report_crash, kMachPortNull); std::string service_name = "org.chromium.crashpad.test.bootstrap_check_in."; - for (int index = 0; index < 16; ++index) { - service_name.append(1, base::RandInt('A', 'Z')); - } + service_name.append(RandomString()); { // The new service hasn’t checked in yet, so this should fail. diff --git a/util/misc/random_string.cc b/util/misc/random_string.cc new file mode 100644 index 0000000..a522799 --- /dev/null +++ b/util/misc/random_string.cc @@ -0,0 +1,29 @@ +// 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/misc/random_string.h" + +#include "base/rand_util.h" + +namespace crashpad { + +std::string RandomString() { + std::string random_string; + for (int index = 0; index < 16; ++index) { + random_string.append(1, static_cast(base::RandInt('A', 'Z'))); + } + return random_string; +} + +} // namespace crashpad diff --git a/util/misc/random_string.h b/util/misc/random_string.h new file mode 100644 index 0000000..93b3a7f --- /dev/null +++ b/util/misc/random_string.h @@ -0,0 +1,31 @@ +// 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_MISC_RANDOM_STRING_H_ +#define CRASHPAD_UTIL_MISC_RANDOM_STRING_H_ + +#include + +namespace crashpad { + +//! \brief Returns a random string. +//! +//! The string consists of 16 uppercase characters chosen at random. The +//! returned string has over 75 bits of randomness (2616 > +//! 275). +std::string RandomString(); + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_MISC_RANDOM_STRING_H_ diff --git a/util/misc/random_string_test.cc b/util/misc/random_string_test.cc new file mode 100644 index 0000000..3ef85df --- /dev/null +++ b/util/misc/random_string_test.cc @@ -0,0 +1,71 @@ +// 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/misc/random_string.h" + +#include + +#include "base/basictypes.h" +#include "gtest/gtest.h" + +namespace crashpad { +namespace test { +namespace { + +TEST(RandomString, RandomString) { + // Explicitly list the allowed characters, rather than relying on a range. + // This prevents the test from having any dependency on the character set, so + // that the implementation is free to assume all uppercase letters are + // contiguous as in ASCII. + const std::string allowed_characters("ABCDEFGHIJKLMNOPQRSTUVWXYZ"); + + size_t character_counts[26] = {}; + ASSERT_EQ(arraysize(character_counts), allowed_characters.size()); + + std::set strings; + + for (size_t i = 0; i < 256; ++i) { + const std::string random_string = RandomString(); + EXPECT_EQ(16u, random_string.size()); + + // Make sure that the string is unique. It is possible, but extremely + // unlikely, for there to be collisions. + auto result = strings.insert(random_string); + EXPECT_TRUE(result.second) << random_string; + + for (char c : random_string) { + size_t character_index = allowed_characters.find(c); + + // Make sure that no unexpected characters appear. + EXPECT_NE(character_index, std::string::npos) << c; + + if (character_index != std::string::npos) { + ++character_counts[character_index]; + } + } + } + + // Make sure every character appears at least once. It is possible, but + // extremely unlikely, for a character to not appear at all. + for (size_t character_index = 0; + character_index < arraysize(character_counts); + ++character_index) { + EXPECT_GT(character_counts[character_index], 0u) + << allowed_characters[character_index]; + } +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/net/http_transport_test.cc b/util/net/http_transport_test.cc index fe40999..16b1cd0 100644 --- a/util/net/http_transport_test.cc +++ b/util/net/http_transport_test.cc @@ -24,7 +24,6 @@ #include "base/format_macros.h" #include "base/logging.h" #include "base/memory/scoped_ptr.h" -#include "base/rand_util.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" #include "build/build_config.h" @@ -32,6 +31,7 @@ #include "test/multiprocess_exec.h" #include "test/paths.h" #include "util/file/file_io.h" +#include "util/misc/random_string.h" #include "util/net/http_body.h" #include "util/net/http_headers.h" #include "util/net/http_multipart_builder.h" @@ -89,14 +89,11 @@ class HTTPTransportTestFixture : public MultiprocessExec { // The parent will also tell the web server what response body to send back. // The web server will only send the response body if the response code is // 200. - std::string expect_response_body; - for (size_t index = 0; index < 8; ++index) { - expect_response_body += static_cast(base::RandInt(' ', '~')); - } + const std::string random_string = RandomString(); ASSERT_TRUE(LoggingWriteFile(WritePipeHandle(), - expect_response_body.c_str(), - expect_response_body.size())); + random_string.c_str(), + random_string.size())); // Now execute the HTTP request. scoped_ptr transport(HTTPTransport::Create()); @@ -111,7 +108,7 @@ class HTTPTransportTestFixture : public MultiprocessExec { bool success = transport->ExecuteSynchronously(&response_body); if (response_code_ == 200) { EXPECT_TRUE(success); - expect_response_body += "\r\n"; + std::string expect_response_body = random_string + "\r\n"; EXPECT_EQ(expect_response_body, response_body); } else { EXPECT_FALSE(success); diff --git a/util/net/http_transport_test_server.py b/util/net/http_transport_test_server.py index 6b82aa0..3f085a5 100755 --- a/util/net/http_transport_test_server.py +++ b/util/net/http_transport_test_server.py @@ -20,7 +20,7 @@ When invoked, this server will write a short integer to stdout, indiciating on which port the server is listening. It will then read one integer from stdin, indiciating the response code to be sent in response to a request. It also reads -8 characters from stdin, which, after having "\r\n" appended, will form the +16 characters from stdin, which, after having "\r\n" appended, will form the response body in a successful response (one with code 200). The server will process one HTTP request, deliver the prearranged response to the client, and write the entire request to stdout. It will then terminate. @@ -144,9 +144,9 @@ def Main(): sys.stdout.flush() # Read the desired test response code as an unsigned short and the desired - # response body as an 8-byte string from the parent process. + # response body as a 16-byte string from the parent process. RequestHandler.response_code, RequestHandler.response_body = \ - struct.unpack('=H8s', sys.stdin.read(struct.calcsize('=H8s'))) + struct.unpack('=H16s', sys.stdin.read(struct.calcsize('=H16s'))) # Handle the request. server.handle_request() diff --git a/util/util.gyp b/util/util.gyp index 5f4064a..b491965 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -94,6 +94,8 @@ 'misc/initialization_state_dcheck.h', 'misc/pdb_structures.cc', 'misc/pdb_structures.h', + 'misc/random_string.cc', + 'misc/random_string.h', 'misc/scoped_forbid_return.cc', 'misc/scoped_forbid_return.h', 'misc/symbolic_constants_common.h', diff --git a/util/util_test.gyp b/util/util_test.gyp index 791a613..2d94ea7 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -59,6 +59,7 @@ 'misc/initialization_state_dcheck_test.cc', 'misc/initialization_state_test.cc', 'misc/scoped_forbid_return_test.cc', + 'misc/random_string_test.cc', 'misc/uuid_test.cc', 'net/http_body_test.cc', 'net/http_body_test_util.cc', diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index 3a41a1d..e122cdb 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -26,6 +26,7 @@ #include "snapshot/crashpad_info_client_options.h" #include "snapshot/win/process_snapshot_win.h" #include "util/file/file_writer.h" +#include "util/misc/random_string.h" #include "util/misc/tri_state.h" #include "util/misc/uuid.h" #include "util/win/get_function.h" @@ -318,10 +319,7 @@ std::wstring ExceptionHandlerServer::CreatePipe() { base::StringPrintf("\\\\.\\pipe\\crashpad_%d_", GetCurrentProcessId()); std::wstring pipe_name; do { - pipe_name = base::UTF8ToUTF16(pipe_name_base); - for (int index = 0; index < 16; ++index) { - pipe_name.append(1, static_cast(base::RandInt('A', 'Z'))); - } + pipe_name = base::UTF8ToUTF16(pipe_name_base + RandomString()); first_pipe_instance_.reset(CreateNamedPipeInstance(pipe_name, true)); diff --git a/util/win/process_info_test.cc b/util/win/process_info_test.cc index 52d8c78..e5bde2f 100644 --- a/util/win/process_info_test.cc +++ b/util/win/process_info_test.cc @@ -20,7 +20,6 @@ #include "base/files/file_path.h" #include "base/memory/scoped_ptr.h" -#include "base/rand_util.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" #include "build/build_config.h" @@ -30,6 +29,7 @@ #include "test/paths.h" #include "test/win/child_launcher.h" #include "util/file/file_io.h" +#include "util/misc/random_string.h" #include "util/misc/uuid.h" #include "util/win/command_line.h" #include "util/win/get_function.h" @@ -559,9 +559,9 @@ TEST(ProcessInfo, Handles) { ASSERT_TRUE(scoped_key.is_valid()); std::wstring mapping_name = - base::UTF8ToUTF16(base::StringPrintf("Local\\test_mapping_%d_%I64x", + base::UTF8ToUTF16(base::StringPrintf("Local\\test_mapping_%d_%s", GetCurrentProcessId(), - base::RandUint64())); + RandomString().c_str())); ScopedKernelHANDLE mapping(CreateFileMapping(INVALID_HANDLE_VALUE, nullptr, PAGE_READWRITE,