Merge lp:~raof/mir/mir-module-throw-exception into lp:mir

[Chris Halse Rogers]

Add MIR_MODULE_THROW_EXCEPTION.

This is like BOOST_THROW_EXCEPTION, but it additionally ensures that the dynamic library
that contains the throwing code is not unloaded before exception processing finishes.

This prevents annoying-to-debug SIGSEGVs when throwing an exception from a platform module
and stack unwinding causes the mir::UniqueModulePtr to be destroyed (and the DSO unloaded)
before the catch {} attempts to access code (such as destructors) or data (such as
static strings) from the now-unloaded DSO with hilarious consequences!

Add MIR_MODULE_THROW_EXCEPTION.

This is like BOOST_THROW_EXCEPTION, but it additionally ensures that the dynamic library
that contains the throwing code is not unloaded before exception processing finishes.

This prevents annoying-to-debug SIGSEGVs when throwing an exception from a platform module
and stack unwinding causes the mir::UniqueModulePtr to be destroyed (and the DSO unloaded)
before the catch {} attempts to access code (such as destructors) or data (such as
static strings) from the now-unloaded DSO with hilarious consequences!

Part of the infrastructure required to fix https://bugs.launchpad.net/ubuntu/+source/unity-system-compositor/+bug/1553549

I ran into this with graphics-eglstream. This, not anything the nvidia driver was doing, was the reason that all my exceptions caused SIGSEGV within libstdc++!

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3469
https://mir-jenkins.ubuntu.com/job/mir-ci/879/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/904/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/941
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/932
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/932
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/914/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/914/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/914/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/914/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/914/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/879/rebuild

review: Needs Fixing (continuous-integration)

[lp:~raof/mir/mir-module-throw-exception]

3470. By Chris Halse Rogers on 2016-04-21

Babysit g++5 by explicitly using std::hash<uint32_t>.

I don't really know why g++5 can't handle std::unordered_map<MyEnumClass, bool> while g++6 can,
but that's apparently the case.

Make life easier on poor little g++5

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3470
https://mir-jenkins.ubuntu.com/job/mir-ci/882/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/908/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/945
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/936
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/936
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/918/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/918/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/918/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/918/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/918
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/918/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/918/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/882/rebuild

review: Needs Fixing (continuous-integration)

Unmerged revisions

3470. By Chris Halse Rogers on 2016-04-21

Babysit g++5 by explicitly using std::hash<uint32_t>.

I don't really know why g++5 can't handle std::unordered_map<MyEnumClass, bool> while g++6 can,
but that's apparently the case.

Make life easier on poor little g++5

3469. By Chris Halse Rogers on 2016-04-21

Use MIR_MODULE_THROW_EXCEPTION in graphics-throw.so

3468. By Chris Halse Rogers on 2016-04-21

Add MIR_MODULE_THROW_EXCEPTION.

This is like BOOST_THROW_EXCEPTION, but it additionally ensures that the dynamic library
that contains the throwing code is not unloaded before exception processing finishes.

This prevents annoying-to-debug SIGSEGVs when throwing an exception from a platform module
and stack unwinding causes the mir::UniqueModulePtr to be destroyed (and the DSO unloaded)
before the catch {} attempts to access code (such as destructors) or data (such as
static strings) from the now-unloaded DSO with hilarious consequences!

3467. By Chris Halse Rogers on 2016-04-20

Test exception-safety of all mg::Platform methods.

Spoiler: only the constructor is exception-safe. All other methods SIGSEGV trying to run
code or load data found in the DSO that's just been unloaded...

=== modified file 'include/test/mir/test/doubles/null_platform.h'
--- include/test/mir/test/doubles/null_platform.h	2016-01-29 08:18:22 +0000
+++ include/test/mir/test/doubles/null_platform.h	2016-04-21 07:37:38 +0000
@@ -46,11 +46,6 @@
         return mir::make_module_ptr<NullDisplay>();
     }
 
-    std::shared_ptr<graphics::PlatformIPCPackage> connection_ipc_package()
-    {
-        return std::make_shared<graphics::PlatformIPCPackage>();
-    }
-
     mir::UniqueModulePtr<graphics::PlatformIpcOperations> make_ipc_operations() const override
     {
         return mir::make_module_ptr<NullPlatformIpcOperations>();

=== modified file 'src/common/sharedlibrary/CMakeLists.txt'
--- src/common/sharedlibrary/CMakeLists.txt	2016-03-23 06:39:56 +0000
+++ src/common/sharedlibrary/CMakeLists.txt	2016-04-21 07:37:38 +0000
@@ -17,7 +17,10 @@
 add_library(mirsharedsharedlibrary OBJECT
   module_deleter.cpp
   shared_library.cpp
-  shared_library_prober.cpp ${PROJECT_SOURCE_DIR}/src/include/common/mir/shared_library_prober.h
+  shared_library_prober.cpp
+  ${PROJECT_SOURCE_DIR}/src/include/common/mir/shared_library_prober.h
+  module_throw_exception.cpp
+  ${PROJECT_SOURCE_DIR}/src/include/common/mir/module_throw_exception.h
 )
 
 list(APPEND MIR_COMMON_SOURCES

=== added file 'src/common/sharedlibrary/module_throw_exception.cpp'
--- src/common/sharedlibrary/module_throw_exception.cpp	1970-01-01 00:00:00 +0000
+++ src/common/sharedlibrary/module_throw_exception.cpp	2016-04-21 07:37:38 +0000
@@ -0,0 +1,94 @@
+/*
+ * Copyright © 2016 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Christopher James Halse Rogers <[e-mail]>
+ */
+
+#include "mir/module_throw_exception.h"
+
+#include "mir/shared_library.h"
+#include "mir/log.h"
+
+#include <memory>
+#include <boost/exception/info.hpp>
+#include <boost/exception/enable_error_info.hpp>
+
+
+namespace
+{
+
+/*
+ * We use a ModuleGuard type with custom to_string() implementation to control
+ * how boost::diagnostic_information displays the error_info.
+ */
+struct ModuleGuard
+{
+    ModuleGuard(char const* library)
+        : library_name{library},
+          library{std::make_shared<mir::SharedLibrary>(library)}
+    {
+    }
+
+    std::string library_name;
+    std::shared_ptr<mir::SharedLibrary> library;
+};
+
+std::string to_string(ModuleGuard const& library)
+{
+    return library.library_name;
+}
+
+typedef boost::error_info<struct thrown_from_dso, ModuleGuard> module_guard;
+}
+
+void mir::detail::module_throw_exception(
+    std::exception_ptr exception,
+    char const* library,
+    char const* function_name,
+    char const* file_name,
+    int line_number)
+{
+    /*
+     * We can't accept a boost::exception const&, add the information, and then throw it because
+     * boost::exception is abstract.
+     *
+     * Likewise, we *shouldn't* accept a std::exception const&, add the information, and throw it because
+     * that erases the original type of the exception.
+     *
+     * So, instead we take a std::exception_ptr, rethrow it, and add the data in the catch branch.
+     * This preserves the original exception type.
+     */
+    try
+    {
+        std::rethrow_exception(exception);
+    }
+    catch (boost::exception const& e)
+    {
+        e << ::boost::throw_function(function_name);
+        e << ::boost::throw_file(file_name);
+        e << ::boost::throw_line(line_number);
+        e << module_guard(library);
+        throw;
+    }
+    catch (...)
+    {
+        mir::log_warning("Called module_throw_exception with an exception not deriving from boost::exception.");
+        mir::log_warning("This will *NOT* ensure the module remains live for the duration of exception processing.");
+        mir::log_warning("If Mir subsequently crashes without useful error message, this was likely the cause.");
+        mir::log_warning("Called from:\n\tDSO: %s\n\tfile: %s\n\tfunction: %s\n\tline number: %i",
+            library, file_name, function_name, line_number);
+        throw;
+    }
+}

=== modified file 'src/common/symbols.map'
--- src/common/symbols.map	2016-04-04 03:30:04 +0000
+++ src/common/symbols.map	2016-04-21 07:37:38 +0000
@@ -222,6 +222,7 @@
 MIR_COMMON_unreleased {
   global:
     extern "C++" {
+       mir::detail::module_throw_exception*;
        MirEvent::to_surface*;
        MirEvent::to_resize*;
        MirEvent::to_orientation*;

=== added file 'src/include/common/mir/module_throw_exception.h'
--- src/include/common/mir/module_throw_exception.h	1970-01-01 00:00:00 +0000
+++ src/include/common/mir/module_throw_exception.h	2016-04-21 07:37:38 +0000
@@ -0,0 +1,69 @@
+/*
+ * Copyright © 2016 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Christopher James Halse Rogers <[e-mail]>
+ */
+
+#ifndef MIR_MODULE_THROW_EXCEPTION_H_
+#define MIR_MODULE_THROW_EXCEPTION_H_
+
+#include "mir/libname.h"
+
+#include <exception>
+#include <boost/exception/enable_error_info.hpp>
+
+/**
+ * Throw an exception, ensuring the DSO outlives exception processing
+ *
+ * This acts like BOOST_THROW_EXCEPTION, except that it additionally ensures that the
+ * DSO that contains the call will not be unloaded until exception processing has completed.
+ *
+ * This is useful, and should be used, for throwing exceptions from Mir platform modules,
+ * where otherwise the destruction of mir::UniqueModulePtr during stack unwinding may result
+ * in the dynamic object containing exception information being unloaded before that exception
+ * information is referenced.
+ *
+ * All exceptions thrown from platform modules should be thrown with this macro.
+ *
+ * \param   [in] exception  The exception to throw. There are no constraints on this parameter,
+ *                          but, like all exceptions, it SHOULD derive from std::exception.
+ */
+#define MIR_MODULE_THROW_EXCEPTION(exception) \
+    ::mir::detail::module_throw_exception( \
+        std::make_exception_ptr(::boost::enable_error_info(exception)), \
+        mir::libname(), \
+        __PRETTY_FUNCTION__, \
+        __FILE__, \
+        (int)__LINE__)
+
+namespace mir
+{
+namespace detail
+{
+/**
+ * Implementation detail of MIR_MODULE_THROW_EXCEPTION.
+ *
+ * It SHOULD NOT be called directly.
+ */
+void module_throw_exception(
+    std::exception_ptr exception,
+    char const* library,
+    char const* function_name,
+    char const* file_name,
+    int line_number) __attribute__((noreturn));
+}
+}
+
+#endif //MIR_MODULE_THROW_EXCEPTION_H_

=== modified file 'tests/acceptance-tests/test_server_shutdown.cpp'
--- tests/acceptance-tests/test_server_shutdown.cpp	2016-03-23 06:39:56 +0000
+++ tests/acceptance-tests/test_server_shutdown.cpp	2016-04-21 07:37:38 +0000
@@ -55,10 +55,13 @@
     }
 }
 
+using ServerShutdownWithGraphicsPlatformException = ::testing::TestWithParam<char const*>;
+
 // Regression test for LP: #1528135
-TEST(ServerShutdownWithException, clean_shutdown_on_plugin_construction_exception)
+TEST_P(ServerShutdownWithGraphicsPlatformException, clean_shutdown_on_plugin_construction_exception)
 {
     char const* argv = "ServerShutdownWithException";
+    mtf::TemporaryEnvironmentValue exception_set("MIR_TEST_FRAMEWORK_THROWING_PLATFORM_EXCEPTIONS", GetParam());
     mtf::TemporaryEnvironmentValue graphics_platform("MIR_SERVER_PLATFORM_GRAPHICS_LIB", mtf::server_platform("graphics-throw.so").c_str());
     mtf::TemporaryEnvironmentValue input_platform("MIR_SERVER_PLATFORM_INPUT_LIB", mtf::server_platform("input-stub.so").c_str());
     mir::Server server;
@@ -70,6 +73,17 @@
     server.run();
 }
 
+INSTANTIATE_TEST_CASE_P(
+    PlatformExceptions,
+    ServerShutdownWithGraphicsPlatformException,
+    ::testing::Values(
+        "constructor",
+        "create_buffer_allocator",
+        "create_display",
+        "make_ipc_operations",
+        "egl_native_display"
+    ));
+
 using ServerShutdownDeathTest = ServerShutdown;
 
 TEST_F(ServerShutdownDeathTest, abort_removes_endpoint)

=== modified file 'tests/mir_test_framework/platform_graphics_throw.cpp'
--- tests/mir_test_framework/platform_graphics_throw.cpp	2016-01-18 08:54:58 +0000
+++ tests/mir_test_framework/platform_graphics_throw.cpp	2016-04-21 07:37:38 +0000
@@ -20,8 +20,14 @@
 #include "mir/test/doubles/null_platform.h"
 #include "mir/assert_module_entry_point.h"
 #include "mir/libname.h"
+#include "mir/module_throw_exception.h"
 
 #include <boost/throw_exception.hpp>
+#include <stdlib.h>
+#include <unordered_map>
+#include <libgen.h>
+#include <malloc.h>
+#include <mir/shared_library.h>
 
 namespace mg = mir::graphics;
 namespace mo = mir::options;
@@ -35,13 +41,87 @@
 
 namespace
 {
-class ExceptionThrowingPlatform : public mir::test::doubles::NullPlatform
+class ExceptionThrowingPlatform : public mg::Platform
 {
 public:
     ExceptionThrowingPlatform()
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Exception during construction"));
-    }
+        : should_throw{parse_exception_request(getenv("MIR_TEST_FRAMEWORK_THROWING_PLATFORM_EXCEPTIONS"))}
+    {
+        if (should_throw.at(ExceptionLocation::at_constructor))
+            MIR_MODULE_THROW_EXCEPTION(std::runtime_error("Exception during construction"));
+
+        std::unique_ptr<char, void(*)(void*)> library_path{strdup(mir::libname()), &free};
+        auto platform_path = dirname(library_path.get());
+
+        mir::SharedLibrary stub_platform_library{std::string(platform_path) + "/graphics-dummy.so"};
+        auto create_stub_platform = stub_platform_library.load_function<mg::CreateHostPlatform>("create_host_platform");
+
+        stub_platform = create_stub_platform(nullptr, nullptr, nullptr);
+    }
+
+    mir::UniqueModulePtr<mg::GraphicBufferAllocator> create_buffer_allocator() override
+    {
+        if (should_throw.at(ExceptionLocation::at_create_buffer_allocator))
+            MIR_MODULE_THROW_EXCEPTION(std::runtime_error("Exception during create_buffer_allocator"));
+
+        return stub_platform->create_buffer_allocator();
+    }
+
+    mir::UniqueModulePtr<mg::Display> create_display(
+        std::shared_ptr<mg::DisplayConfigurationPolicy> const& ptr,
+        std::shared_ptr<mg::GLConfig> const& shared_ptr) override
+    {
+        if (should_throw.at(ExceptionLocation::at_create_display))
+            MIR_MODULE_THROW_EXCEPTION(std::runtime_error("Exception during create_display"));
+
+        return stub_platform->create_display(ptr, shared_ptr);
+    }
+
+    mir::UniqueModulePtr<mg::PlatformIpcOperations> make_ipc_operations() const override
+    {
+        if (should_throw.at(ExceptionLocation::at_make_ipc_operations))
+            MIR_MODULE_THROW_EXCEPTION(std::runtime_error("Exception during make_ipc_operations"));
+
+        return stub_platform->make_ipc_operations();
+    }
+
+    EGLNativeDisplayType egl_native_display() const override
+    {
+        if (should_throw.at(ExceptionLocation::at_egl_native_display))
+            MIR_MODULE_THROW_EXCEPTION(std::runtime_error("Exception during egl_native_display"));
+
+        return stub_platform->egl_native_display();
+    }
+
+private:
+    enum ExceptionLocation : uint32_t
+    {
+        at_constructor,
+        at_create_buffer_allocator,
+        at_create_display,
+        at_make_ipc_operations,
+        at_egl_native_display
+    };
+
+    static std::unordered_map<ExceptionLocation, bool, std::hash<uint32_t>> parse_exception_request(char const* request)
+    {
+        std::unordered_map<ExceptionLocation, bool, std::hash<uint32_t>> requested_exceptions;
+        requested_exceptions[ExceptionLocation::at_constructor] =
+            static_cast<bool>(strstr(request, "constructor"));
+        requested_exceptions[ExceptionLocation::at_create_buffer_allocator] =
+            static_cast<bool>(strstr(request, "create_buffer_allocator"));
+        requested_exceptions[ExceptionLocation::at_create_display] =
+            static_cast<bool>(strstr(request, "create_display"));
+        requested_exceptions[ExceptionLocation::at_make_ipc_operations] =
+            static_cast<bool>(strstr(request, "make_ipc_operations"));
+        requested_exceptions[ExceptionLocation::at_egl_native_display] =
+            static_cast<bool>(strstr(request, "egl_native_display"));
+
+        return requested_exceptions;
+    };
+
+    std::unordered_map<ExceptionLocation, bool, std::hash<uint32_t>> const should_throw;
+    mir::UniqueModulePtr<mg::Platform> stub_platform;
 };
 
 }