diff options
| -rw-r--r-- | src/client/windows/build/common.gypi | 6 | ||||
| -rw-r--r-- | src/client/windows/handler/exception_handler.cc | 18 | ||||
| -rw-r--r-- | src/client/windows/handler/exception_handler.h | 12 | ||||
| -rw-r--r-- | src/client/windows/tests/crash_generation_app/crash_generation_app.cc | 20 | ||||
| -rw-r--r-- | src/client/windows/unittests/exception_handler_death_test.cc | 76 | ||||
| -rwxr-xr-x | src/client/windows/unittests/exception_handler_nesting_test.cc | 4 | ||||
| -rw-r--r-- | src/client/windows/unittests/exception_handler_test.cc | 2 | ||||
| -rw-r--r-- | src/client/windows/unittests/minidump_test.cc | 3 | ||||
| -rw-r--r-- | src/common/string_conversion.cc | 2 | ||||
| -rw-r--r-- | src/common/windows/http_upload.cc | 26 | ||||
| -rw-r--r-- | src/google_breakpad/processor/minidump.h | 4 | ||||
| -rwxr-xr-x | src/processor/minidump.cc | 124 |
12 files changed, 173 insertions, 124 deletions
diff --git a/src/client/windows/build/common.gypi b/src/client/windows/build/common.gypi index dfd29bd0..60a24582 100644 --- a/src/client/windows/build/common.gypi +++ b/src/client/windows/build/common.gypi @@ -507,7 +507,7 @@ 'msvs_disabled_warnings': [4800], 'msvs_settings': { 'VCCLCompilerTool': { - 'WarnAsError': 'false', + 'WarnAsError': 'true', 'Detect64BitPortabilityProblems': 'false', }, }, @@ -1174,7 +1174,7 @@ '$(VSInstallDir)/VC/atlmfc/include', ], 'msvs_cygwin_dirs': ['<(DEPTH)/third_party/cygwin'], - 'msvs_disabled_warnings': [4396, 4503, 4819], + 'msvs_disabled_warnings': [4100, 4127, 4396, 4503, 4512, 4819, 4995], 'msvs_settings': { 'VCCLCompilerTool': { 'MinimalRebuild': 'false', @@ -1182,7 +1182,7 @@ 'BufferSecurityCheck': 'true', 'EnableFunctionLevelLinking': 'true', 'RuntimeTypeInfo': 'false', - 'WarningLevel': '3', + 'WarningLevel': '4', 'WarnAsError': 'true', 'DebugInformationFormat': '3', 'conditions': [ diff --git a/src/client/windows/handler/exception_handler.cc b/src/client/windows/handler/exception_handler.cc index 14c2cf86..3a2f824c 100644 --- a/src/client/windows/handler/exception_handler.cc +++ b/src/client/windows/handler/exception_handler.cc @@ -96,7 +96,7 @@ ExceptionHandler::ExceptionHandler(const wstring& dump_path, pipe_handle, NULL, // crash_generation_client custom_info); -} +} ExceptionHandler::ExceptionHandler( const wstring& dump_path, @@ -104,19 +104,19 @@ ExceptionHandler::ExceptionHandler( MinidumpCallback callback, void* callback_context, int handler_types, - MINIDUMP_TYPE dump_type, - CrashGenerationClient* crash_generation_client, - const CustomClientInfo* custom_info) { + CrashGenerationClient* crash_generation_client) { + // The dump_type, pipe_name and custom_info that are passed in to Initialize() + // are not used. The ones set in crash_generation_client are used instead. Initialize(dump_path, filter, callback, callback_context, handler_types, - MiniDumpNormal, - NULL, // pipe_name - NULL, // pipe_handle + MiniDumpNormal, // dump_type - not used + NULL, // pipe_name - not used + NULL, // pipe_handle crash_generation_client, - custom_info); + NULL); // custom_info - not used } ExceptionHandler::ExceptionHandler(const wstring &dump_path, @@ -875,7 +875,7 @@ BOOL CALLBACK ExceptionHandler::MinidumpWriteDumpCallback( callback_context->iter++; return TRUE; } - + // Include all modules. case IncludeModuleCallback: case ModuleCallback: diff --git a/src/client/windows/handler/exception_handler.h b/src/client/windows/handler/exception_handler.h index 2c8ff7d9..539c6662 100644 --- a/src/client/windows/handler/exception_handler.h +++ b/src/client/windows/handler/exception_handler.h @@ -61,9 +61,9 @@ #include <DbgHelp.h> #include <rpc.h> -#pragma warning( push ) +#pragma warning(push) // Disable exception handler warnings. -#pragma warning( disable : 4530 ) +#pragma warning(disable:4530) #include <list> #include <string> @@ -212,9 +212,7 @@ class ExceptionHandler { MinidumpCallback callback, void* callback_context, int handler_types, - MINIDUMP_TYPE dump_type, - CrashGenerationClient* crash_generation_client, - const CustomClientInfo* custom_info); + CrashGenerationClient* crash_generation_client); ~ExceptionHandler(); @@ -497,7 +495,7 @@ class ExceptionHandler { static CRITICAL_SECTION handler_stack_critical_section_; // The number of instances of this class. - volatile static LONG instance_count_; + static volatile LONG instance_count_; // disallow copy ctor and operator= explicit ExceptionHandler(const ExceptionHandler &); @@ -506,6 +504,6 @@ class ExceptionHandler { } // namespace google_breakpad -#pragma warning( pop ) +#pragma warning(pop) #endif // CLIENT_WINDOWS_HANDLER_EXCEPTION_HANDLER_H__ diff --git a/src/client/windows/tests/crash_generation_app/crash_generation_app.cc b/src/client/windows/tests/crash_generation_app/crash_generation_app.cc index e36c1d1e..12449ce9 100644 --- a/src/client/windows/tests/crash_generation_app/crash_generation_app.cc +++ b/src/client/windows/tests/crash_generation_app/crash_generation_app.cc @@ -345,9 +345,9 @@ void CleanUp() { // Processes messages for the main window. // -// WM_COMMAND - process the application menu. -// WM_PAINT - Paint the main window. -// WM_DESTROY - post a quit message and return. +// WM_COMMAND - process the application menu. +// WM_PAINT - Paint the main window. +// WM_DESTROY - post a quit message and return. LRESULT CALLBACK WndProc(HWND wnd, UINT message, WPARAM w_param, @@ -359,7 +359,7 @@ LRESULT CALLBACK WndProc(HWND wnd, #pragma warning(push) #pragma warning(disable:4312) - // Disable warning C4312: 'type cast' : conversion from 'LONG' to + // Disable warning C4312: 'type cast' : conversion from 'LONG' to // 'HINSTANCE' of greater size. // The value returned by GetwindowLong in the case below returns unsigned. HINSTANCE instance = (HINSTANCE)GetWindowLong(wnd, GWL_HINSTANCE); @@ -415,16 +415,16 @@ LRESULT CALLBACK WndProc(HWND wnd, instance, NULL); break; - case WM_SIZE: - // Make the edit control the size of the window's client area. - MoveWindow(client_status_edit_box, + case WM_SIZE: + // Make the edit control the size of the window's client area. + MoveWindow(client_status_edit_box, 0, 0, LOWORD(l_param), // width of client area. HIWORD(l_param), // height of client area. TRUE); // repaint window. break; - case WM_SETFOCUS: + case WM_SETFOCUS: SetFocus(client_status_edit_box); break; case WM_PAINT: @@ -501,7 +501,7 @@ int APIENTRY _tWinMain(HINSTANCE instance, MyRegisterClass(instance); // Perform application initialization. - if (!InitInstance (instance, command_show)) { + if (!InitInstance(instance, command_show)) { return FALSE; } @@ -518,5 +518,5 @@ int APIENTRY _tWinMain(HINSTANCE instance, } } - return (int)msg.wParam; + return static_cast<int>(msg.wParam); } diff --git a/src/client/windows/unittests/exception_handler_death_test.cc b/src/client/windows/unittests/exception_handler_death_test.cc index ba7be920..3a16e525 100644 --- a/src/client/windows/unittests/exception_handler_death_test.cc +++ b/src/client/windows/unittests/exception_handler_death_test.cc @@ -124,17 +124,19 @@ TEST_F(ExceptionHandlerDeathTest, InProcTest) { // the semantics of the exception handler being inherited/not // inherited across CreateProcess(). ASSERT_TRUE(DoesPathExist(temp_path_)); - google_breakpad::ExceptionHandler *exc = - new google_breakpad::ExceptionHandler( - temp_path_, NULL, &MinidumpWrittenCallback, NULL, - google_breakpad::ExceptionHandler::HANDLER_ALL); + scoped_ptr<google_breakpad::ExceptionHandler> exc( + new google_breakpad::ExceptionHandler( + temp_path_, + NULL, + &MinidumpWrittenCallback, + NULL, + google_breakpad::ExceptionHandler::HANDLER_ALL)); // Disable GTest SEH handler testing::DisableExceptionHandlerInScope disable_exception_handler; int *i = NULL; ASSERT_DEATH((*i)++, kSuccessIndicator); - delete exc; } static bool gDumpCallbackCalled = false; @@ -147,26 +149,24 @@ void clientDumpCallback(void *dump_context, void ExceptionHandlerDeathTest::DoCrashAccessViolation( const OutOfProcGuarantee out_of_proc_guarantee) { - google_breakpad::ExceptionHandler *exc = NULL; + scoped_ptr<google_breakpad::ExceptionHandler> exc; if (out_of_proc_guarantee == OUT_OF_PROC_GUARANTEED) { - google_breakpad::CrashGenerationClient *client = + google_breakpad::CrashGenerationClient *client = new google_breakpad::CrashGenerationClient(kPipeName, MiniDumpNormal, NULL); // custom_info ASSERT_TRUE(client->Register()); - exc = new google_breakpad::ExceptionHandler( + exc.reset(new google_breakpad::ExceptionHandler( temp_path_, NULL, // filter NULL, // callback NULL, // callback_context google_breakpad::ExceptionHandler::HANDLER_ALL, - MiniDumpNormal, - client, - NULL); // custom_info + client)); } else { ASSERT_TRUE(out_of_proc_guarantee == OUT_OF_PROC_BEST_EFFORT); - exc = new google_breakpad::ExceptionHandler( + exc.reset(new google_breakpad::ExceptionHandler( temp_path_, NULL, // filter NULL, // callback @@ -174,7 +174,7 @@ void ExceptionHandlerDeathTest::DoCrashAccessViolation( google_breakpad::ExceptionHandler::HANDLER_ALL, MiniDumpNormal, kPipeName, - NULL); // custom_info + NULL)); // custom_info } // Disable GTest SEH handler @@ -302,17 +302,20 @@ wstring find_minidump_in_directory(const wstring &directory) { filename = directory + L"\\" + find_data.cFileName; break; } - } while(FindNextFile(find_handle, &find_data)); + } while (FindNextFile(find_handle, &find_data)); FindClose(find_handle); return filename; } TEST_F(ExceptionHandlerDeathTest, InstructionPointerMemory) { ASSERT_TRUE(DoesPathExist(temp_path_)); - google_breakpad::ExceptionHandler *exc = + scoped_ptr<google_breakpad::ExceptionHandler> exc( new google_breakpad::ExceptionHandler( - temp_path_, NULL, NULL, NULL, - google_breakpad::ExceptionHandler::HANDLER_ALL); + temp_path_, + NULL, + NULL, + NULL, + google_breakpad::ExceptionHandler::HANDLER_ALL)); // Disable GTest SEH handler testing::DisableExceptionHandlerInScope disable_exception_handler; @@ -333,7 +336,7 @@ TEST_F(ExceptionHandlerDeathTest, InstructionPointerMemory) { // minidump should contain 128 bytes on either side of the // instruction pointer. memcpy(memory + kOffset, instructions, sizeof(instructions)); - + // Now execute the instructions, which should crash. typedef void (*void_function)(void); void_function memory_function = @@ -382,11 +385,10 @@ TEST_F(ExceptionHandlerDeathTest, InstructionPointerMemory) { uint8_t suffix_bytes[kMemorySize - kOffset - sizeof(instructions)]; memset(prefix_bytes, 0, sizeof(prefix_bytes)); memset(suffix_bytes, 0, sizeof(suffix_bytes)); - EXPECT_TRUE(memcmp(bytes, prefix_bytes, sizeof(prefix_bytes)) == 0); - EXPECT_TRUE(memcmp(bytes + kOffset, instructions, - sizeof(instructions)) == 0); - EXPECT_TRUE(memcmp(bytes + kOffset + sizeof(instructions), - suffix_bytes, sizeof(suffix_bytes)) == 0); + EXPECT_EQ(0, memcmp(bytes, prefix_bytes, sizeof(prefix_bytes))); + EXPECT_EQ(0, memcmp(bytes + kOffset, instructions, sizeof(instructions))); + EXPECT_EQ(0, memcmp(bytes + kOffset + sizeof(instructions), + suffix_bytes, sizeof(suffix_bytes))); } DeleteFileW(minidump_filename_wide.c_str()); @@ -394,10 +396,13 @@ TEST_F(ExceptionHandlerDeathTest, InstructionPointerMemory) { TEST_F(ExceptionHandlerDeathTest, InstructionPointerMemoryMinBound) { ASSERT_TRUE(DoesPathExist(temp_path_)); - google_breakpad::ExceptionHandler *exc = + scoped_ptr<google_breakpad::ExceptionHandler> exc( new google_breakpad::ExceptionHandler( - temp_path_, NULL, NULL, NULL, - google_breakpad::ExceptionHandler::HANDLER_ALL); + temp_path_, + NULL, + NULL, + NULL, + google_breakpad::ExceptionHandler::HANDLER_ALL)); // Disable GTest SEH handler testing::DisableExceptionHandlerInScope disable_exception_handler; @@ -426,7 +431,7 @@ TEST_F(ExceptionHandlerDeathTest, InstructionPointerMemoryMinBound) { // minidump should contain 128 bytes on either side of the // instruction pointer. memcpy(memory + kOffset, instructions, sizeof(instructions)); - + // Now execute the instructions, which should crash. typedef void (*void_function)(void); void_function memory_function = @@ -484,10 +489,13 @@ TEST_F(ExceptionHandlerDeathTest, InstructionPointerMemoryMinBound) { TEST_F(ExceptionHandlerDeathTest, InstructionPointerMemoryMaxBound) { ASSERT_TRUE(DoesPathExist(temp_path_)); - google_breakpad::ExceptionHandler *exc = + scoped_ptr<google_breakpad::ExceptionHandler> exc( new google_breakpad::ExceptionHandler( - temp_path_, NULL, NULL, NULL, - google_breakpad::ExceptionHandler::HANDLER_ALL); + temp_path_, + NULL, + NULL, + NULL, + google_breakpad::ExceptionHandler::HANDLER_ALL)); // Disable GTest SEH handler testing::DisableExceptionHandlerInScope disable_exception_handler; @@ -511,7 +519,7 @@ TEST_F(ExceptionHandlerDeathTest, InstructionPointerMemoryMaxBound) { // Write some instructions that will crash. memcpy(memory + kOffset, instructions, sizeof(instructions)); - + // Now execute the instructions, which should crash. typedef void (*void_function)(void); void_function memory_function = @@ -559,9 +567,9 @@ TEST_F(ExceptionHandlerDeathTest, InstructionPointerMemoryMaxBound) { uint8_t prefix_bytes[kPrefixSize]; memset(prefix_bytes, 0, sizeof(prefix_bytes)); - EXPECT_TRUE(memcmp(bytes, prefix_bytes, sizeof(prefix_bytes)) == 0); - EXPECT_TRUE(memcmp(bytes + kPrefixSize, - instructions, sizeof(instructions)) == 0); + EXPECT_EQ(0, memcmp(bytes, prefix_bytes, sizeof(prefix_bytes))); + EXPECT_EQ(0, memcmp(bytes + kPrefixSize, + instructions, sizeof(instructions))); } DeleteFileW(minidump_filename_wide.c_str()); diff --git a/src/client/windows/unittests/exception_handler_nesting_test.cc b/src/client/windows/unittests/exception_handler_nesting_test.cc index 0cf30013..3ae1d7cd 100755 --- a/src/client/windows/unittests/exception_handler_nesting_test.cc +++ b/src/client/windows/unittests/exception_handler_nesting_test.cc @@ -149,7 +149,7 @@ void InstallExceptionHandlerAndCrash(bool install_filter, ASSERT_TRUE(DoesPathExist(temp_path)); google_breakpad::ExceptionHandler exc( temp_path, - install_filter ? + install_filter ? (filter_return_value ? &CrashHandlerFilter<true> : &CrashHandlerFilter<false>) : @@ -178,7 +178,7 @@ TEST(AssertDeathSanity, Regex) { std::string(kFoo) + std::string(kEndOfLine)); - ASSERT_DEATH(DoCrash(kBar), + ASSERT_DEATH(DoCrash(kBar), std::string(kStartOfLine) + std::string(kBar) + std::string(kEndOfLine)); diff --git a/src/client/windows/unittests/exception_handler_test.cc b/src/client/windows/unittests/exception_handler_test.cc index 7b50e301..55275323 100644 --- a/src/client/windows/unittests/exception_handler_test.cc +++ b/src/client/windows/unittests/exception_handler_test.cc @@ -390,7 +390,7 @@ TEST_F(ExceptionHandlerTest, WriteMinidumpTest) { // Read the minidump and verify some info. Minidump minidump(minidump_filename); ASSERT_TRUE(minidump.Read()); - //TODO(ted): more comprehensive tests... + // TODO(ted): more comprehensive tests... } // Test that an additional memory region can be included in the minidump. diff --git a/src/client/windows/unittests/minidump_test.cc b/src/client/windows/unittests/minidump_test.cc index 62f212f8..e3097f28 100644 --- a/src/client/windows/unittests/minidump_test.cc +++ b/src/client/windows/unittests/minidump_test.cc @@ -161,7 +161,8 @@ bool HasFileInfo(const std::wstring& file_path) { } TEST_F(MinidumpTest, Version) { - API_VERSION* version = ::ImagehlpApiVersion(); + // Loads DbgHelp.dll in process + ImagehlpApiVersion(); HMODULE dbg_help = ::GetModuleHandle(L"dbghelp.dll"); ASSERT_TRUE(dbg_help != NULL); diff --git a/src/common/string_conversion.cc b/src/common/string_conversion.cc index c4107faf..9b06e86b 100644 --- a/src/common/string_conversion.cc +++ b/src/common/string_conversion.cc @@ -140,7 +140,7 @@ string UTF16ToUTF8(const vector<uint16_t> &in, bool swap) { scoped_array<UTF8> target_buffer(new UTF8[target_capacity]); UTF8 *target_ptr = target_buffer.get(); UTF8 *target_end_ptr = target_ptr + target_capacity; - ConversionResult result = ConvertUTF16toUTF8(&source_ptr, source_end_ptr, + ConversionResult result = ConvertUTF16toUTF8(&source_ptr, source_end_ptr, &target_ptr, target_end_ptr, strictConversion); diff --git a/src/common/windows/http_upload.cc b/src/common/windows/http_upload.cc index aabb9a46..838185b7 100644 --- a/src/common/windows/http_upload.cc +++ b/src/common/windows/http_upload.cc @@ -30,7 +30,7 @@ #include <assert.h> // Disable exception handler warnings. -#pragma warning( disable : 4530 ) +#pragma warning(disable:4530) #include <fstream> @@ -163,7 +163,7 @@ bool HTTPUpload::SendRequest(const wstring &url, fwprintf(stderr, L"Could not unset receive timeout, continuing...\n"); } } - + if (!HttpSendRequest(request.get(), NULL, 0, const_cast<char *>(request_body.data()), static_cast<DWORD>(request_body.size()))) { @@ -215,8 +215,7 @@ bool HTTPUpload::ReadResponse(HINTERNET request, wstring *response) { BOOL return_code; while (((return_code = InternetQueryDataAvailable(request, &bytes_available, - 0, 0)) != 0) && bytes_available > 0) { - + 0, 0)) != 0) && bytes_available > 0) { vector<char> response_buffer(bytes_available); DWORD size_read; @@ -323,6 +322,7 @@ bool HTTPUpload::GenerateRequestBody(const map<wstring, wstring> ¶meters, // static bool HTTPUpload::GetFileContents(const wstring &filename, vector<char> *contents) { + bool rv = false; // The "open" method on pre-MSVC8 ifstream implementations doesn't accept a // wchar_t* filename, so use _wfopen directly in that case. For VC8 and // later, _wfopen has been deprecated in favor of _wfopen_s, which does @@ -336,15 +336,21 @@ bool HTTPUpload::GetFileContents(const wstring &filename, if (file.is_open()) { file.seekg(0, ios::end); std::streamoff length = file.tellg(); - contents->resize(length); - if (length != 0) { - file.seekg(0, ios::beg); - file.read(&((*contents)[0]), length); + // Check for loss of data when converting lenght from std::streamoff into + // std::vector<char>::size_type + std::vector<char>::size_type vector_size = + static_cast<std::vector<char>::size_type>(length); + if (static_cast<std::streamoff>(vector_size) == length) { + contents->resize(vector_size); + if (length != 0) { + file.seekg(0, ios::beg); + file.read(&((*contents)[0]), length); + } + rv = true; } file.close(); - return true; } - return false; + return rv; } // static diff --git a/src/google_breakpad/processor/minidump.h b/src/google_breakpad/processor/minidump.h index 87c00276..8b2d0fe8 100644 --- a/src/google_breakpad/processor/minidump.h +++ b/src/google_breakpad/processor/minidump.h @@ -477,7 +477,7 @@ class MinidumpModule : public MinidumpObject, // True after a successful Read. This is different from valid_, which is // not set true until ReadAuxiliaryData also completes successfully. // module_valid_ is only used by ReadAuxiliaryData and the functions it - // calls to determine whether the object is ready for auxiliary data to + // calls to determine whether the object is ready for auxiliary data to // be read. bool module_valid_; @@ -821,7 +821,7 @@ class MinidumpMemoryInfo : public MinidumpObject { uint64_t GetBase() const { return valid_ ? memory_info_.base_address : 0; } // The size, in bytes, of the memory region. - uint32_t GetSize() const { return valid_ ? memory_info_.region_size : 0; } + uint64_t GetSize() const { return valid_ ? memory_info_.region_size : 0; } // Return true if the memory protection allows execution. bool IsExecutable() const; diff --git a/src/processor/minidump.cc b/src/processor/minidump.cc index 8d4bab77..4ad82053 100755 --- a/src/processor/minidump.cc +++ b/src/processor/minidump.cc @@ -65,8 +65,6 @@ #include "processor/basic_code_modules.h" #include "processor/logging.h" - - namespace google_breakpad { @@ -226,19 +224,19 @@ static string* UTF16ToUTF8(const vector<uint16_t>& in, // Convert the Unicode code point (unichar) into its UTF-8 representation, // appending it to the out string. if (unichar < 0x80) { - (*out) += unichar; + (*out) += static_cast<char>(unichar); } else if (unichar < 0x800) { - (*out) += 0xc0 | (unichar >> 6); - (*out) += 0x80 | (unichar & 0x3f); + (*out) += 0xc0 | static_cast<char>(unichar >> 6); + (*out) += 0x80 | static_cast<char>(unichar & 0x3f); } else if (unichar < 0x10000) { - (*out) += 0xe0 | (unichar >> 12); - (*out) += 0x80 | ((unichar >> 6) & 0x3f); - (*out) += 0x80 | (unichar & 0x3f); + (*out) += 0xe0 | static_cast<char>(unichar >> 12); + (*out) += 0x80 | static_cast<char>((unichar >> 6) & 0x3f); + (*out) += 0x80 | static_cast<char>(unichar & 0x3f); } else if (unichar < 0x200000) { - (*out) += 0xf0 | (unichar >> 18); - (*out) += 0x80 | ((unichar >> 12) & 0x3f); - (*out) += 0x80 | ((unichar >> 6) & 0x3f); - (*out) += 0x80 | (unichar & 0x3f); + (*out) += 0xf0 | static_cast<char>(unichar >> 18); + (*out) += 0x80 | static_cast<char>((unichar >> 12) & 0x3f); + (*out) += 0x80 | static_cast<char>((unichar >> 6) & 0x3f); + (*out) += 0x80 | static_cast<char>(unichar & 0x3f); } else { BPLOG(ERROR) << "UTF16ToUTF8 cannot represent high value " << HexString(unichar) << " in UTF-8"; @@ -329,7 +327,7 @@ bool MinidumpContext::Read(uint32_t expected_size) { } if (cpu_type != MD_CONTEXT_AMD64) { - //TODO: fall through to switch below? + // TODO: fall through to switch below? // need a Tell method to be able to SeekSet back to beginning // http://code.google.com/p/google-breakpad/issues/detail?id=224 BPLOG(ERROR) << "MinidumpContext not actually amd64 context"; @@ -388,7 +386,7 @@ bool MinidumpContext::Read(uint32_t expected_size) { Swap(&context_amd64->r14); Swap(&context_amd64->r15); Swap(&context_amd64->rip); - //FIXME: I'm not sure what actually determines + // FIXME: I'm not sure what actually determines // which member of the union {flt_save, sse_registers} // is valid. We're not currently using either, // but it would be good to have them swapped properly. @@ -408,10 +406,9 @@ bool MinidumpContext::Read(uint32_t expected_size) { context_flags_ = context_amd64->context_flags; context_.amd64 = context_amd64.release(); - } - // |context_flags| of MDRawContextPPC64 is 64 bits, but other MDRawContext - // in the else case have 32 bits |context_flags|, so special case it here. - else if (expected_size == sizeof(MDRawContextPPC64)) { + } else if (expected_size == sizeof(MDRawContextPPC64)) { + // |context_flags| of MDRawContextPPC64 is 64 bits, but other MDRawContext + // in the else case have 32 bits |context_flags|, so special case it here. uint64_t context_flags; if (!minidump_->ReadBytes(&context_flags, sizeof(context_flags))) { BPLOG(ERROR) << "MinidumpContext could not read context flags"; @@ -421,9 +418,9 @@ bool MinidumpContext::Read(uint32_t expected_size) { Swap(&context_flags); uint32_t cpu_type = context_flags & MD_CONTEXT_CPU_MASK; - scoped_ptr<MDRawContextPPC64> context_ppc64(new MDRawContextPPC64()); + // Set the context_flags member, which has already been read, and // read the rest of the structure beginning with the first member // after context_flags. @@ -477,11 +474,18 @@ bool MinidumpContext::Read(uint32_t expected_size) { Swap(&context_ppc64->vector_save.save_vrvalid); } - context_flags_ = context_ppc64->context_flags; - context_.ppc64 = context_ppc64.release(); - } + context_flags_ = static_cast<uint32_t>(context_ppc64->context_flags); + + // Check for data loss when converting context flags from uint64_t into + // uint32_t + if (static_cast<uint64_t>(context_flags_) != + context_ppc64->context_flags) { + BPLOG(ERROR) << "Data loss detected when converting PPC64 context_flags"; + return false; + } - else { + context_.ppc64 = context_ppc64.release(); + } else { uint32_t context_flags; if (!minidump_->ReadBytes(&context_flags, sizeof(context_flags))) { BPLOG(ERROR) << "MinidumpContext could not read context flags"; @@ -1205,7 +1209,7 @@ void MinidumpContext::Print() { printf(" r14 = 0x%" PRIx64 "\n", context_amd64->r14); printf(" r15 = 0x%" PRIx64 "\n", context_amd64->r15); printf(" rip = 0x%" PRIx64 "\n", context_amd64->rip); - //TODO: print xmm, vector, debug registers + // TODO: print xmm, vector, debug registers printf("\n"); break; } @@ -1673,7 +1677,8 @@ bool MinidumpThreadList::Read(uint32_t expected_size) { thread_count * sizeof(MDRawThread)) { uint32_t useless; if (!minidump_->ReadBytes(&useless, 4)) { - BPLOG(ERROR) << "MinidumpThreadList cannot read threadlist padded bytes"; + BPLOG(ERROR) << "MinidumpThreadList cannot read threadlist padded " + "bytes"; return false; } } else { @@ -1950,7 +1955,7 @@ string MinidumpModule::code_identifier() const { case MD_OS_IOS: case MD_OS_SOLARIS: case MD_OS_ANDROID: - case MD_OS_LINUX: + case MD_OS_LINUX: case MD_OS_PS3: { // TODO(mmentovai): support uuid extension if present, otherwise fall // back to version (from LC_ID_DYLIB?), otherwise fall back to something @@ -2560,7 +2565,8 @@ bool MinidumpModuleList::Read(uint32_t expected_size) { module_count * MD_MODULE_SIZE) { uint32_t useless; if (!minidump_->ReadBytes(&useless, 4)) { - BPLOG(ERROR) << "MinidumpModuleList cannot read modulelist padded bytes"; + BPLOG(ERROR) << "MinidumpModuleList cannot read modulelist padded " + "bytes"; return false; } } else { @@ -2806,12 +2812,13 @@ bool MinidumpMemoryList::Read(uint32_t expected_size) { region_count * sizeof(MDMemoryDescriptor)) { uint32_t useless; if (!minidump_->ReadBytes(&useless, 4)) { - BPLOG(ERROR) << "MinidumpMemoryList cannot read memorylist padded bytes"; + BPLOG(ERROR) << "MinidumpMemoryList cannot read memorylist padded " + "bytes"; return false; } } else { BPLOG(ERROR) << "MinidumpMemoryList size mismatch, " << expected_size << - " != " << sizeof(region_count) + + " != " << sizeof(region_count) + region_count * sizeof(MDMemoryDescriptor); return false; } @@ -3142,7 +3149,7 @@ bool MinidumpAssertion::Read(uint32_t expected_size) { if (new_expression.get()) expression_ = *new_expression; } - + // assertion word_length = UTF16codeunits(assertion_.function, sizeof(assertion_.function)); @@ -3791,7 +3798,7 @@ bool MinidumpMemoryInfoList::Read(uint32_t expected_size) { } // Sanity check that the header is the expected size. - //TODO(ted): could possibly handle this more gracefully, assuming + // TODO(ted): could possibly handle this more gracefully, assuming // that future versions of the structs would be backwards-compatible. if (header.size_of_header != sizeof(MDRawMemoryInfoList)) { BPLOG(ERROR) << "MinidumpMemoryInfoList header size mismatch, " << @@ -3824,9 +3831,20 @@ bool MinidumpMemoryInfoList::Read(uint32_t expected_size) { return false; } + // Check for data loss when converting header.number_of_entries from + // uint64_t into MinidumpMemoryInfos::size_type (uint32_t) + MinidumpMemoryInfos::size_type header_number_of_entries = + static_cast<unsigned int>(header.number_of_entries); + if (static_cast<uint64_t>(header_number_of_entries) != + header.number_of_entries) { + BPLOG(ERROR) << "Data loss detected when converting " + "the header's number_of_entries"; + return false; + } + if (header.number_of_entries != 0) { scoped_ptr<MinidumpMemoryInfos> infos( - new MinidumpMemoryInfos(header.number_of_entries, + new MinidumpMemoryInfos(header_number_of_entries, MinidumpMemoryInfo(minidump_))); for (unsigned int index = 0; @@ -3842,7 +3860,7 @@ bool MinidumpMemoryInfoList::Read(uint32_t expected_size) { } uint64_t base_address = info->GetBase(); - uint32_t region_size = info->GetSize(); + uint64_t region_size = info->GetSize(); if (!range_map_->StoreRange(base_address, region_size, index)) { BPLOG(ERROR) << "MinidumpMemoryInfoList could not store" @@ -3857,7 +3875,7 @@ bool MinidumpMemoryInfoList::Read(uint32_t expected_size) { infos_ = infos.release(); } - info_count_ = header.number_of_entries; + info_count_ = header_number_of_entries; valid_ = true; return true; @@ -4312,17 +4330,27 @@ bool Minidump::ReadBytes(void* bytes, size_t count) { return false; } stream_->read(static_cast<char*>(bytes), count); - size_t bytes_read = stream_->gcount(); - if (bytes_read != count) { - if (bytes_read == size_t(-1)) { - string error_string; - int error_code = ErrnoString(&error_string); - BPLOG(ERROR) << "ReadBytes: error " << error_code << ": " << error_string; - } else { - BPLOG(ERROR) << "ReadBytes: read " << bytes_read << "/" << count; - } + std::streamsize bytes_read = stream_->gcount(); + if (bytes_read == -1) { + string error_string; + int error_code = ErrnoString(&error_string); + BPLOG(ERROR) << "ReadBytes: error " << error_code << ": " << error_string; + return false; + } + + // Convert to size_t and check for data loss + size_t bytes_read_converted = static_cast<size_t>(bytes_read); + if (static_cast<std::streamsize>(bytes_read_converted) != bytes_read) { + BPLOG(ERROR) << "ReadBytes: conversion data loss detected when converting " + << bytes_read << " to " << bytes_read_converted; return false; } + + if (bytes_read_converted != count) { + BPLOG(ERROR) << "ReadBytes: read " << bytes_read_converted << "/" << count; + return false; + } + return true; } @@ -4348,7 +4376,15 @@ off_t Minidump::Tell() { return (off_t)-1; } - return stream_->tellg(); + // Check for conversion data loss + std::streamoff std_streamoff = stream_->tellg(); + off_t rv = static_cast<off_t>(std_streamoff); + if (static_cast<std::streamoff>(rv) == std_streamoff) { + return rv; + } else { + BPLOG(ERROR) << "Data loss detected"; + return (off_t)-1; + } } |