summaryrefslogtreecommitdiff
path: root/desktop/source
diff options
context:
space:
mode:
authorMike Kaganski <mike.kaganski@collabora.com>2020-01-02 15:30:36 +0300
committerMike Kaganski <mike.kaganski@collabora.com>2020-01-10 18:05:01 +0100
commit12b5892cf9c78dd917f2e50672cd250478e6c7d6 (patch)
treebc131525b92075e3be986bfa05de060f7e632a67 /desktop/source
parentc12358166a9bd88fe10feabca45a6ad3f65dff8e (diff)
Delete google_breakpad::ExceptionHandler before calling _exit()
While debugging tdf#129712 on Windows, I saw this sequence: 1. nullptr was dereferenced (the reason for tdf#129712). 2. ExceptionHandler::HandleException was called (in workdir/UnpackedTarball/breakpad/src/client/windows/handler/exception_handler.cc). 3. It called ExceptionHandler::WriteMinidumpOnHandlerThread. 4. Minidump was created in ExceptionHandler::ExceptionHandlerThreadMain. 5. Document Recovery dialog was shown in Desktop::Exception (in desktop/source/app/app.cxx). 6. After closing dialog, _exit() was called in Desktop::Exception. 7. All threads except main were terminated. 8. Another access violation was thrown in the "minimal CRT cleanup". 9. ExceptionHandler::HandleException called again. 10. ExceptionHandler::WriteMinidumpOnHandlerThread hung on WaitForSingleObject because handler thread that should release the semaphore was terminated already at step 7. The process had to be killed manually. This change destroys the breakpad handler at the start of Desktop::Exception, which de-registers itself (on Windows it uses SetUnhandledExceptionFilter). Other than preventing the hang, the rationale also is that keeping the handler after first minidump creation is wrong: even if the second minidump creation succeeded, uploading it to crashdump server would give not the actual problem, but some unrelated stack. Change-Id: If12d0c7db519693f733b5ab3b8a288cef800a149 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/86104 Reviewed-by: Markus Mohrhard <markus.mohrhard@googlemail.com> Tested-by: Mike Kaganski <mike.kaganski@collabora.com>
Diffstat (limited to 'desktop/source')
-rw-r--r--desktop/source/app/app.cxx4
-rw-r--r--desktop/source/app/crashreport.cxx49
-rw-r--r--desktop/source/app/sofficemain.cxx58
3 files changed, 50 insertions, 61 deletions
diff --git a/desktop/source/app/app.cxx b/desktop/source/app/app.cxx
index 7fd35ee3d574..eff6f70df610 100644
--- a/desktop/source/app/app.cxx
+++ b/desktop/source/app/app.cxx
@@ -1136,6 +1136,10 @@ void Desktop::Exception(ExceptionCategory nCategory)
// protect against recursive calls
static bool bInException = false;
+#if HAVE_FEATURE_BREAKPAD
+ CrashReporter::removeExceptionHandler(); // disallow re-entry
+#endif
+
SystemWindowFlags nOldMode = Application::GetSystemWindowMode();
Application::SetSystemWindowMode( nOldMode & ~SystemWindowFlags::NOAUTOMODE );
if ( bInException )
diff --git a/desktop/source/app/crashreport.cxx b/desktop/source/app/crashreport.cxx
index 3420f20c4496..743ae43e7c9b 100644
--- a/desktop/source/app/crashreport.cxx
+++ b/desktop/source/app/crashreport.cxx
@@ -36,14 +36,44 @@
#if defined __clang__
#pragma clang diagnostic pop
#endif
+#include <locale>
+#include <codecvt>
#endif
osl::Mutex CrashReporter::maMutex;
-google_breakpad::ExceptionHandler* CrashReporter::mpExceptionHandler = nullptr;
+std::unique_ptr<google_breakpad::ExceptionHandler> CrashReporter::mpExceptionHandler;
bool CrashReporter::mbInit = false;
CrashReporter::vmaKeyValues CrashReporter::maKeyValues;
+#if defined( UNX ) && !defined MACOSX && !defined IOS && !defined ANDROID
+static bool dumpCallback(const google_breakpad::MinidumpDescriptor& descriptor, void* /*context*/, bool succeeded)
+{
+ CrashReporter::addKeyValue("DumpFile", OStringToOUString(descriptor.path(), RTL_TEXTENCODING_UTF8), CrashReporter::Write);
+ SAL_WARN("desktop", "minidump generated: " << descriptor.path());
+
+ return succeeded;
+}
+#elif defined WNT
+static bool dumpCallback(const wchar_t* path, const wchar_t* id,
+ void* /*context*/, EXCEPTION_POINTERS* /*exinfo*/,
+ MDRawAssertionInfo* /*assertion*/,
+ bool succeeded)
+{
+ // TODO: moggi: can we avoid this conversion
+#ifdef _MSC_VER
+#pragma warning (disable: 4996)
+#endif
+ std::wstring_convert<std::codecvt_utf8<wchar_t>> conv1;
+ std::string aPath = conv1.to_bytes(std::wstring(path)) + conv1.to_bytes(std::wstring(id)) + ".dmp";
+ CrashReporter::addKeyValue("DumpFile", OStringToOUString(aPath.c_str(), RTL_TEXTENCODING_UTF8), CrashReporter::AddItem);
+ CrashReporter::addKeyValue("GDIHandles", OUString::number(::GetGuiResources(::GetCurrentProcess(), GR_GDIOBJECTS)), CrashReporter::Write);
+ SAL_WARN("desktop", "minidump generated: " << aPath);
+ return succeeded;
+}
+#endif
+
+
void CrashReporter::writeToFile(std::ios_base::openmode Openmode)
{
std::ofstream ini_file(getIniFileName(), Openmode);
@@ -171,10 +201,21 @@ bool CrashReporter::readSendConfig(std::string& response)
return crashreport::readConfig(CrashReporter::getIniFileName(), &response);
}
-void CrashReporter::storeExceptionHandler(google_breakpad::ExceptionHandler* pExceptionHandler)
+void CrashReporter::installExceptionHandler()
+{
+ if (!IsDumpEnable())
+ return;
+#if defined( UNX ) && !defined MACOSX && !defined IOS && !defined ANDROID
+ google_breakpad::MinidumpDescriptor descriptor("/tmp");
+ mpExceptionHandler = std::make_unique<google_breakpad::ExceptionHandler>(descriptor, nullptr, dumpCallback, nullptr, true, -1);
+#elif defined WNT
+ mpExceptionHandler = std::make_unique<google_breakpad::ExceptionHandler>(L".", nullptr, dumpCallback, nullptr, google_breakpad::ExceptionHandler::HANDLER_ALL);
+#endif
+}
+
+void CrashReporter::removeExceptionHandler()
{
- if(IsDumpEnable())
- mpExceptionHandler = pExceptionHandler;
+ mpExceptionHandler.reset();
}
diff --git a/desktop/source/app/sofficemain.cxx b/desktop/source/app/sofficemain.cxx
index 70d396d232b6..064c08579ab9 100644
--- a/desktop/source/app/sofficemain.cxx
+++ b/desktop/source/app/sofficemain.cxx
@@ -46,22 +46,6 @@
#if HAVE_FEATURE_BREAKPAD
#include <desktop/crashreport.hxx>
-
-#if defined( UNX ) && !defined MACOSX && !defined IOS && !defined ANDROID
-#include <client/linux/handler/exception_handler.h>
-#elif defined WNT
-#if defined __clang__
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wmicrosoft-enum-value"
-#endif
-#include <client/windows/handler/exception_handler.h>
-#if defined __clang__
-#pragma clang diagnostic pop
-#endif
-#include <locale>
-#include <codecvt>
-#endif
-
#endif
#include <postwin.h>
@@ -75,52 +59,12 @@
# define LOGI(...) ((void)__android_log_print(ANDROID_LOG_INFO, LOGTAG, __VA_ARGS__))
#endif
-#if HAVE_FEATURE_BREAKPAD
-
-#if defined( UNX ) && !defined MACOSX && !defined IOS && !defined ANDROID
-static bool dumpCallback(const google_breakpad::MinidumpDescriptor& descriptor, void* /*context*/, bool succeeded)
-{
- CrashReporter::addKeyValue("DumpFile", OStringToOUString(descriptor.path(), RTL_TEXTENCODING_UTF8), CrashReporter::Write);
- SAL_WARN("desktop", "minidump generated: " << descriptor.path());
-
- return succeeded;
-}
-#elif defined WNT
-static bool dumpCallback(const wchar_t* path, const wchar_t* id,
- void* /*context*/, EXCEPTION_POINTERS* /*exinfo*/,
- MDRawAssertionInfo* /*assertion*/,
- bool succeeded)
-{
- // TODO: moggi: can we avoid this conversion
-#ifdef _MSC_VER
-#pragma warning (disable: 4996)
-#endif
- std::wstring_convert<std::codecvt_utf8<wchar_t>> conv1;
- std::string aPath = conv1.to_bytes(std::wstring(path)) + conv1.to_bytes(std::wstring(id)) + ".dmp";
- CrashReporter::addKeyValue("DumpFile", OStringToOUString(aPath.c_str(), RTL_TEXTENCODING_UTF8), CrashReporter::AddItem);
- CrashReporter::addKeyValue("GDIHandles", OUString::number(::GetGuiResources (::GetCurrentProcess(), GR_GDIOBJECTS)), CrashReporter::Write);
- SAL_WARN("desktop", "minidump generated: " << aPath);
- return succeeded;
-}
-#endif
-
-#endif
extern "C" int DESKTOP_DLLPUBLIC soffice_main()
{
sal_detail_initialize(sal::detail::InitializeSoffice, nullptr);
#if HAVE_FEATURE_BREAKPAD
-
-#if defined( UNX ) && !defined MACOSX && !defined IOS && !defined ANDROID
- google_breakpad::MinidumpDescriptor descriptor("/tmp");
- google_breakpad::ExceptionHandler eh(descriptor, nullptr, dumpCallback, nullptr, true, -1);
-
- CrashReporter::storeExceptionHandler(&eh);
-#elif defined WNT
- google_breakpad::ExceptionHandler eh(L".", nullptr, dumpCallback, nullptr, google_breakpad::ExceptionHandler::HANDLER_ALL);
-
- CrashReporter::storeExceptionHandler(&eh);
-#endif
+ CrashReporter::installExceptionHandler();
#endif
#if defined( UNX ) && !defined MACOSX && !defined IOS && !defined ANDROID && HAVE_FEATURE_UI && HAVE_FEATURE_OPENGL