Merge lp:~alan-griffiths/mir/fix-1526658 into lp:mir

[Alan Griffiths]

common, client: a more flexible way to probe modules: once we've found a good current platform we don't even try to load an older one. (More cleanup related to cases of lp:1526658)

common, client: a more flexible way to probe modules: once we've found a good current platform we don't even try to load an older one. (More cleanup related to cases of lp:1526658)

Some background from the bug history:

1. the mesa.so.2 client platform module leaks symbols and dl handle to itself when *loaded*.

2. the mesa.so.3 client platform module leaks symbols and dl handle to itself when *used*. It can be loaded and probed safely.

3. the mesa.so.4 client platform module leaks symbols when *used*. It can be loaded and probed safely.

libmirclient8 from Mir-0.13 (say) needs mesa.so.2 but can load and probe mesa.so.3/4 (see 2, 3 above).

Various fixes have been applied in the libmirclient9 series, and in 0.19 for_path() ordered the modules by "so number descending", but still loaded all the modules before doing so. As mentioned in 1 above this is problematic. The workaround in the bug is to uninstall mesa2.

This proposal continues to orders modules by "so number descending" but create_client_platform() probes them as they are loaded. Once a compatible driver is selected *the search is quit* so once mesa4 is selected neither mesa3 nor mesa2 is loaded.

That means that libmirclient8/mesa2 can safely co-exist with libmirclient9/mesa4.

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

PASSED: Continuous integration, rev:3283
https://mir-jenkins.ubuntu.com/job/mir-ci/187/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/188/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3283
http://jenkins.qa.ubuntu.com/job/mir-ci/6160/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5738
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4645/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5694
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/484
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/484/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/484
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/484/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5691
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5691/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8118
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27170

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6160/rebuild

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

8: [ FAILED ] TestClientCursorAPI.cursor_passed_through_nested_server (60036 ms)

lp:1525003?

[Daniel van Vugt (vanvugt)]

There's an additional fix I want to do on this bug this week:
Add debian packaging magic to simply force the broken packages to be uninstalled.

Because once you've loaded and tried to probe mir-client-platform-mesa2 finding out it's not the right one, it's already too late. It has already leaked a handle to itself (which will happen with this branch too).

That change is much more important than this one. But I haven't reviewed this in detail yet.

review: Abstain

[Alan Griffiths (alan-griffiths)]

> There's an additional fix I want to do on this bug this week:
> Add debian packaging magic to simply force the broken packages to be
> uninstalled.

Removing these older packages is a workaround that works for most cases. However, a hypothetical parallel installation that requires mesa2 would be broken by forced removal.

Debian packaging magic to ensure that any installed mir-graphics-drivers-desktop/mir-graphics-drivers-android is updated would be useful though.

> Because once you've loaded and tried to probe mir-client-platform-mesa2
> finding out it's not the right one, it's already too late. It has already
> leaked a handle to itself (which will happen with this branch too).

The point of this MP is that we don't probe the badly behaved mesa2 once we've found that mesa4 works.

> That change is much more important than this one. But I haven't reviewed this
> in detail yet.

I disagree. Automating a workaround for our failed support for side-by-side installations is far less important than fixing the support.

[Daniel van Vugt (vanvugt)]

I did initially agree with your "hypothetical parallel installation that requires mesa2" assertion. However that's not true if the older Mir installation uses libmirclient9. All recent Mir releases use libmirclient9, which means even old clients will start using the new libmirclient9 immediately and only need mir-client-platform-mesa4 to work.

[Daniel van Vugt (vanvugt)]

Manual testing suggests this branch works well. Because mesa.so.2 is never loaded according to MIR_CLIENT_SHARED_LIBRARY_PROBER_REPORT=log.

Unfortunately I can't fully test for the original crash of bug 1526658 because my filesystem insists on finding mesa.so.4 in the directory before mesa.so.2, so it is working around the bug by pure luck.

Approved, but I suggest it might be nicer to stop adding functions to mircommon and instead just glob the directory from inside src/client/probing_client_platform_factory.cpp

review: Approve

[Alan Griffiths (alan-griffiths)]

> Manual testing suggests this branch works well. Because mesa.so.2 is never
> loaded according to MIR_CLIENT_SHARED_LIBRARY_PROBER_REPORT=log.
>
> Unfortunately I can't fully test for the original crash of bug 1526658 because
> my filesystem insists on finding mesa.so.4 in the directory before mesa.so.2,
> so it is working around the bug by pure luck.

You're aware that libraries_for_path() orders the libraries by so version (descending)?

[Daniel van Vugt (vanvugt)]

Whoops. My test methodology was flawed and my previous statement still holds (if you substitute mir-client-platform-mesa2 for mesa.so.[123]):

"Because once you've loaded and tried to probe mir-client-platform-mesa2 finding out it's not the right one, it's already too late. It has already leaked a handle to itself (which will happen with this branch too)."

If your directory happens to hash in such a way that the OS finds mesa.so.3 (from Mir 0.17) first, then the simple act of dlopening it (regardless of probe outcome) will make it leak. And your clients will then crash. So I think this branch won't fix anything. We're just failing to see it crash by pure luck, per above.

Note that the delicacy of the problem isn't really in Mir 0.18 as much as Mir 0.17 where we had the constructor attribute :(

http://bazaar.launchpad.net/~mir-team/mir/0.17/view/head:/src/platforms/mesa/client/client_platform.cpp#L62

If there is any chance at all that you might dlopen an old mesa.so.[123] then there's a chance you'll leak that old one and Mesa will still crash. So this branch is not a useful fix.

review: Disapprove

[Daniel van Vugt (vanvugt)]

> You're aware that libraries_for_path() orders the libraries by so version
> (descending)?

Well, the bug exists so that's clearly not helping. Actually it's the order in which you dlopen the libraries that's important, and not the order in which they are returned in our API.

[Alan Griffiths (alan-griffiths)]

> Whoops. My test methodology was flawed and my previous statement still holds
> (if you substitute mir-client-platform-mesa2 for mesa.so.[123]):
>
> "Because once you've loaded and tried to probe mir-client-platform-mesa2
> finding out it's not the right one, it's already too late. It has already
> leaked a handle to itself (which will happen with this branch too)."
>
> If your directory happens to hash in such a way that the OS finds mesa.so.3
> (from Mir 0.17) first, then the simple act of dlopening it (regardless of
> probe outcome) will make it leak. And your clients will then crash. So I think
> this branch won't fix anything. We're just failing to see it crash by pure
> luck, per above.
>
> Note that the delicacy of the problem isn't really in Mir 0.18 as much as Mir
> 0.17 where we had the constructor attribute :(
>
> http://bazaar.launchpad.net/~mir-
> team/mir/0.17/view/head:/src/platforms/mesa/client/client_platform.cpp#L62
>
> If there is any chance at all that you might dlopen an old mesa.so.[123] then
> there's a chance you'll leak that old one and Mesa will still crash. So this
> branch is not a useful fix.

You're describing the problem this MP fixes.

The previous implementation using libraries_for_path() ordered the modules by "so number descending", but still loaded all the modules. As you point out that was problematic.

This MP select_libraries_for_path() also orders by "so number descending" but create_client_platform() probes them as they are loaded. Once a compatible driver is selected the search is quit and *earlier ones are neither loaded nor probed*.

[Alberto Aguirre (albaguirre)]

Makes sense to me.

review: Approve

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

Even if we didn't have the bug, what this MP is doing makes sense. Once we've found a recent version of a shared object that works, I can't think of a reason to keep looking at the older libraries.

review: Approve

[Daniel van Vugt (vanvugt)]

Oh I failed to look at the sorting implementation. I assumed it had RAII constructed the SharedLibraries before it sorted them. It actually sorts just the filename strings.

That will teach me to try and comment on code while cooking dinner.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/2382/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5768/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4675
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5724
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-autolanding/214
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-autolanding/214/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-autolanding/214
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-autolanding/214/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5721
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5721/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8134
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27223

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Building remotely on prodstack-worker-med-3 (pbuilder-mir pbuilder-medium pbuilder-cloud-medium-test) in workspace /var/lib/jenkins/slaves/prodstack-worker-med-3/workspace/mir-android-vivid-i386-build
java.io.IOException: remote file operation failed: /var/lib/jenkins/slaves/prodstack-worker-med-3/workspace/mir-android-vivid-i386-build at hudson.remoting.Channel@77b86652:prodstack-worker-med-3: hudson.remoting.ChannelClosedException: channel is already closed
 at hudson.FilePath.act(FilePath.java:987)

Reapproving

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/client/probing_client_platform_factory.cpp'
--- src/client/probing_client_platform_factory.cpp	2016-01-29 08:18:22 +0000
+++ src/client/probing_client_platform_factory.cpp	2016-01-29 12:17:29 +0000
@@ -31,6 +31,25 @@
     // than it takes to choose the right one. So this list is local:
     std::vector<std::shared_ptr<mir::SharedLibrary>> platform_modules;
 
+    auto const module_selector = [context, &platform_modules](std::shared_ptr<mir::SharedLibrary> const& module)
+    {
+        try
+        {
+            auto probe = module->load_function<ClientPlatformProbe>("is_appropriate_module", CLIENT_PLATFORM_VERSION);
+            if (probe(context))
+            {
+                platform_modules.push_back(module);
+                return Selection::quit;
+            }
+        }
+        catch (std::runtime_error const&)
+        {
+            // Assume we were handed a SharedLibrary that's not a client platform module of the correct vintage.
+        }
+
+        return Selection::persist;
+    };
+
     if (!platform_overrides.empty())
     {
         // Even forcing a choice, platform is only loaded on demand. It's good
@@ -39,37 +58,19 @@
         // if you really wanted to.
 
         for (auto const& platform : platform_overrides)
-            platform_modules.push_back(
-                std::make_shared<mir::SharedLibrary>(platform));
+            module_selector(std::make_shared<mir::SharedLibrary>(platform));
     }
     else
     {
         for (auto const& path : platform_paths)
-        {
-            // This is sub-optimal. We shouldn't need to have all drivers
-            // loaded simultaneously, but we do for now due to this API:
-            auto modules = mir::libraries_for_path(path,
-                                               *shared_library_prober_report);
-            for (auto const& module : modules)
-                 platform_modules.push_back(module);
-        }
+            select_libraries_for_path(path, module_selector, *shared_library_prober_report);
     }
 
     for (auto& module : platform_modules)
     {
-        try
-        {
-            auto probe = module->load_function<mir::client::ClientPlatformProbe>("is_appropriate_module", CLIENT_PLATFORM_VERSION);
-            if (probe(context))
-            {
-                auto factory = module->load_function<mir::client::CreateClientPlatform>("create_client_platform", CLIENT_PLATFORM_VERSION);
-                return factory(context);
-            }
-        }
-        catch(std::runtime_error const&)
-        {
-            // We were handed a SharedLibrary that's not a client platform module?
-        }
+        auto factory = module->load_function<CreateClientPlatform>("create_client_platform", CLIENT_PLATFORM_VERSION);
+        return factory(context);
     }
+
     BOOST_THROW_EXCEPTION(std::runtime_error{"No appropriate client platform module found"});
 }

=== modified file 'src/common/sharedlibrary/CMakeLists.txt'
--- src/common/sharedlibrary/CMakeLists.txt	2016-01-20 23:59:18 +0000
+++ src/common/sharedlibrary/CMakeLists.txt	2016-01-29 12:17:29 +0000
@@ -17,7 +17,7 @@
 add_library(mirsharedsharedlibrary OBJECT
   module_deleter.cpp
   shared_library.cpp
-  shared_library_prober.cpp
+  shared_library_prober.cpp ${PROJECT_SOURCE_DIR}/src/include/common/mir/shared_library_prober.h
 )
 
 list(APPEND MIR_COMMON_SOURCES

=== modified file 'src/common/sharedlibrary/shared_library_prober.cpp'
--- src/common/sharedlibrary/shared_library_prober.cpp	2016-01-29 08:18:22 +0000
+++ src/common/sharedlibrary/shared_library_prober.cpp	2016-01-29 12:17:29 +0000
@@ -63,8 +63,10 @@
 }
 }
 
-std::vector<std::shared_ptr<mir::SharedLibrary>>
-mir::libraries_for_path(std::string const& path, mir::SharedLibraryProberReport& report)
+void mir::select_libraries_for_path(
+    std::string const& path,
+    std::function<Selection(std::shared_ptr<mir::SharedLibrary> const&)> const& selector,
+    mir::SharedLibraryProberReport& report)
 {
     report.probing_path(path);
     // We use the error_code overload because we want to throw a std::system_error
@@ -87,20 +89,33 @@
 
     std::sort(libraries.begin(), libraries.end(), &greater_soname_version);
 
-    std::vector<std::shared_ptr<mir::SharedLibrary>> result;
-
     for(auto& lib : libraries)
     {
         try
         {
             report.loading_library(lib);
-            result.emplace_back(std::make_shared<mir::SharedLibrary>(lib.string()));
+            auto const shared_lib = std::make_shared<mir::SharedLibrary>(lib.string());
+
+            if (selector(shared_lib) == Selection::quit)
+                return;
         }
         catch (std::runtime_error const& err)
         {
             report.loading_failed(lib, err);
         }
     }
+}
+
+std::vector<std::shared_ptr<mir::SharedLibrary>>
+mir::libraries_for_path(std::string const& path, mir::SharedLibraryProberReport& report)
+{
+    std::vector<std::shared_ptr<mir::SharedLibrary>> result;
+
+    select_libraries_for_path(
+        path,
+        [&](std::shared_ptr<mir::SharedLibrary> const& shared_lib)
+            { result.push_back(shared_lib); return Selection::persist; },
+        report);
 
     return result;
 }

=== modified file 'src/common/symbols.map'
--- src/common/symbols.map	2016-01-29 12:17:29 +0000
+++ src/common/symbols.map	2016-01-29 12:17:29 +0000
@@ -211,3 +211,10 @@
       mir::detail::libname_impl*;
     };
 } MIR_COMMON_5.1;
+
+MIR_COMMON_unreleased { # New functions in Mir 0.20
+  global:
+    extern "C++" {
+      mir::select_libraries_for_path*;
+    };
+} MIR_COMMON_5v19;
\ No newline at end of file

=== modified file 'src/include/common/mir/shared_library_prober.h'
--- src/include/common/mir/shared_library_prober.h	2015-02-22 07:46:25 +0000
+++ src/include/common/mir/shared_library_prober.h	2016-01-29 12:17:29 +0000
@@ -30,6 +30,14 @@
 class SharedLibrary;
 
 std::vector<std::shared_ptr<SharedLibrary>> libraries_for_path(std::string const& path, SharedLibraryProberReport& report);
+
+// The selector can tell select_libraries_for_path() to persist or quit after each library
+enum class Selection { persist, quit };
+
+void select_libraries_for_path(
+    std::string const& path,
+    std::function<Selection(std::shared_ptr<SharedLibrary> const&)> const& selector,
+    SharedLibraryProberReport& report);
 }