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

[Alan Griffiths]

Try the newest versions of client graphics platform module ".so"s first.
(LP: #1488500)

Try the newest versions of client graphics platform module ".so"s first.

As well as being efficient, this is because if we attempt to load functions from mesa.so.2 before those from mesa.so.3 we get a segfault.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2880
http://jenkins.qa.ubuntu.com/job/mir-ci/4679/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3675
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/96
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2586
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3625
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/828
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/828/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3625
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3625/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6394
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22827

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

I think newest first is still overcomplicated and makes the system unreliable because the fallback to older ABIs likely will break, even crash. When we break ABIs we don't imply any compatibility between them. We should be trying newest only, instead of newest first...

Given that:
   set(MIR_CLIENT_PLATFORM_ABI 3)

Mir should only try to load:
   lib/client-modules/*.so.3

Or it should load * and be able to query the ABI level internally. Only accepting a perfect match for MIR_CLIENT_PLATFORM_ABI == 3.

A better file naming scheme would also help, but that can come later.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> As well as being efficient, this is because if we attempt to load functions from
> mesa.so.2 before those from mesa.so.3 we get a segfault.

Why does this happen? Is there a way to fix this at its core, instead of working around it?

review: Needs Information

[Alan Griffiths (alan-griffiths)]

Daniel:
We load versioned symbols, so loading from a driver that doesn't support the correct version fails and we don't ever call incompatible code. This isn't about ABI breaks, it is about something odd that our mesa driver does (see response to Alexandros below for my current guess).

Enforcing that the library is numbered for the ABI version would prevent a driver module exporting symbols for multiple versions. We don't do that currently but it could conceivably be a useful hack.

Alexandros:
You're right, I haven't root caused this yet. But the MP avoids an embarrassing problem while reducing the chance that we try to load symbols from an obsolete module.

I suspect the problem is caused by the ensure_loaded_with_rtld_global_mesa_client() hack used to force mir_client_mesa_egl_native_display_is_valid into the global symbol table having unintended side effects.

It is too late to fix that in mesa.so.2 even if I knew how.

[Alexandros Frantzis (afrantzis)]

Two issues/questions:

Do we know if the older to newer load order important for this bug to occur, or could it be caused by just loading two mesa modules in the future regardless of order?

If it's the latter, then the proposed solution is problematic, since in the future a server that needs to use an older mesa platform will run into the same segfault (e.g., server needs 4, loads mesa.so.5 first, then mesa.so.4).

30 + return atoi(++lhbuf) > atoi(++rhbuf);

Better to use strtol() that's guaranteed to have well defined behavior for all inputs strings.

Needs info/fixing

review: Needs Information

[Alan Griffiths (alan-griffiths)]

Yes, I would expect there to be a problem with a server that wanted the older mesa module.

But, while we release updates to all downstream servers with each Mir release, that is less bad than the current situation.

[Alexandros Frantzis (afrantzis)]

OK as a quick (and temporary?) workaround, as long as we are aware that we will probably come across this problem again in some form (e.g. using an older server/platform installed in parallel with newer ones) in the future.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2882
http://jenkins.qa.ubuntu.com/job/mir-ci/4695/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3700
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/121
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2611
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3650
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/844
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/844/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3650
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3650/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6415
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22866

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Alright, if it works...

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1817/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3714
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/135
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2625
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3664/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/370
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/370/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3664
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3664/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6424/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22878

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/common/sharedlibrary/shared_library_prober.cpp'
--- src/common/sharedlibrary/shared_library_prober.cpp	2015-07-16 07:03:19 +0000
+++ src/common/sharedlibrary/shared_library_prober.cpp	2015-08-27 11:27:36 +0000
@@ -19,8 +19,10 @@
 #include "mir/shared_library_prober.h"
 #include "mir/shared_library.h"
 
+#include <boost/filesystem.hpp>
+
 #include <system_error>
-#include <boost/filesystem.hpp>
+#include <cstring>
 
 namespace
 {
@@ -47,6 +49,20 @@
 
 }
 
+namespace
+{
+bool greater_soname_version(boost::filesystem::path const& lhs, boost::filesystem::path const& rhs)
+{
+    auto lhbuf = strrchr(lhs.c_str(), '.');
+    auto rhbuf = strrchr(rhs.c_str(), '.');
+
+    if (!rhbuf) return lhbuf;
+    if (!lhbuf) return false;
+
+    return strtol(++lhbuf, 0, 0) > strtol(++rhbuf, 0, 0);
+}
+}
+
 std::vector<std::shared_ptr<mir::SharedLibrary>>
 mir::libraries_for_path(std::string const& path, mir::SharedLibraryProberReport& report)
 {
@@ -62,21 +78,29 @@
         throw error;
     }
 
-    std::vector<std::shared_ptr<mir::SharedLibrary>> libraries;
+    std::vector<boost::filesystem::path> libraries;
     for (; iterator != boost::filesystem::directory_iterator() ; ++iterator)
     {
         if (path_has_library_extension(iterator->path()))
-        {
-            try
-            {
-                report.loading_library(iterator->path());
-                libraries.emplace_back(std::make_shared<mir::SharedLibrary>(iterator->path().string()));
-            }
-            catch (std::runtime_error const& err)
-            {
-                report.loading_failed(iterator->path(), err);
-            }
-        }
-    }
-    return libraries;
+            libraries.push_back(iterator->path().string());
+    }
+
+    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()));
+        }
+        catch (std::runtime_error const& err)
+        {
+            report.loading_failed(lib, err);
+        }
+    }
+
+    return result;
 }