Merge into development-branch : fix-1527449 : Code : Mir

Reviewer	Review Type	Date Requested	Status
Chris Halse Rogers		2016-01-18	Needs Fixing on 2016-01-18
PS Jenkins bot (community)	continuous-integration		Needs Fixing on 2016-01-18
Alan Griffiths		2016-01-08	Abstain on 2016-01-15
Mir CI Bot	continuous-integration		Approve on 2016-01-14
Review via email: mp+281969@code.launchpad.net

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-01-08:

#

FAILED: Continuous integration, rev:3246
http://jenkins.qa.ubuntu.com/job/mir-ci/5955/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5457
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4364
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5413/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/279/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/279/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5410/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-01-08:

#

Hmm, Valgrind's just reporting FD leaks when we die with SIGABRT. So yes, it should leak and that's not an error. I wonder why we don't see that more often...

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-01-08:

#

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

review: Approve (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-01-08:

#

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

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-01-12:

#

+/**
+ * Plugin is a base class designed to underpin any class whose implementation
+ * exists in a dynamically run-time loaded library.
+ * Plugin provides a foolproof check that you don't accidentally unload the
+ * library whose code you're still executing.
+ */
+class Plugin
+{
+public:
+ // Must never be inlined!
+ Plugin();
+ virtual ~Plugin() noexcept;
+
+ void keep_library_loaded(std::shared_ptr<void> const&);
+ std::shared_ptr<void> keep_library_loaded() const;
+private:
+ std::shared_ptr<void> library;
+};

This looks like an intrusive design to tackle the same problem addressed more elegantly by mir::make_module_ptr<>. Is there any reason we need both?

review: Needs Information

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-01-13:

#

Good question. Yes, a few reasons for that:

1. I wrote Plugin long before noticing make_module_ptr existed.
2. Plugin is simpler, more elegant, smaller, fewer templates, easier to understand, and provides a handy debugging feature which tells you you've made a mistake before you encounter cryptic unmapped memory crashes.
3. AFAIK Plugin is also more reliable, as make_module_ptr is still known to cause serious problems -> bug 1528135

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-01-13:

#

I mean Plugin vs make_module_ptr and the full contents of module_deleter.h. Although on second thoughts I think I'm finally starting to understand module_deleter.h and could make it work too.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-01-13:

#

OK, I switched to mir::make_module_ptr<> and got an ugly crash on shutdown. But thank you, because that led me to understand why earlier experiments over the past couple of weeks got the same crash.

The reason is that the act of constructing a mesa platform from mesa.so.4 :
mir::make_module_ptr<mclm::ClientPlatform>(
makes the compiler instantiate that UniqueModulePtr template and its destructor inside mesa.so.4.

So indeed make_module_ptr delays the unload of mesa.so.4 a little to a safer time, it's not safe enough. Because after unloading mesa.so.4 it has to try and destroy the UniqueModulePtr whose template instance was in mesa.so.4 but was unmapped from memory just before we need to call it. Hence *crash* when using make_module_ptr.

This seems to be a fundamental flaw in make_module_ptr and might explain bug 1528135 too. But suffice to say make_module_ptr doesn't work, and will cause crashes. Whereas mir::Plugin does work without crashing.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-01-13:

#

The key difference and the reason why make_module_ptr doesn't work is something I documented here:

40 + // Must never be inlined!

whereas a template instance like UniqueModulePtr is instantiated in the plugin module itself. So unloading such a plugin module that implements the UniqueModulePtr will crash and can never be safe.

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-01-13:

#

No, the problem of bug #1528135 is a bit more complex than that.

The destruction handling of UniqueModulePtr is not part of the platform library. The call to the destructor, and the unref of the library which is attached to the deleter happens at the owner of the ptr.

But of course you can annul the mechanic of make_module_ptr. Instead of making the code that loads the platform and creates instances from it the owner of the UniqueModuleptr(s).. you can stick it into a shared_ptr while you are still in the scope of the client platform. That will type erase the deleter handling, thus the shared_ptr deleter is now the owner.. and the code for it is generated inside the client platform...

To make proper use of UniqueModulePtr you need to use it as the return type of the factory functions that get dlsymed.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-01-14:

#

Returning UniqueModulePtr from your factory ABI within a plugin theoretically sounds like an improvement but requires a few pieces to be moved around and I'm not yet convinced it's sufficiently safe.

Forgive me for repeating but I want to restate the problem more concisely for other readers' and my own benefit:

Let's imagine you have a plugin named mesa.so which implements interface Platform with its own MesaPlatform class. Your application knows nothing about "mesa" and contains no symbols with "mesa" in their name. Everything named "mesa" lives in mesa.so.
  Now you load your plugin "mesa.so" and it returns a UniqueModulePtr<MesaPlatform>. Because the only part of your project that knows the word "mesa" is mesa.so, the UniqueModulePtr<MesaPlatform> is fully and only instantiated inside mesa.so.
  Now you unload your plugin and three things get torn down, in this order:
   1. ~MesaPlatform inside mesa.so
   2. Unload/unmap mesa.so from memory.
   3. ~UniqueModulePtr<MesaPlatform> which was inside mesa.so, but is no longer loaded so you crash.

Yes, in theory you could avoid the crash, but only by either:
(a) Leaking mesa.so by any means, accidental or intentional; or
(b) Ensuring UniqueModulePtr is only used on the interface: UniqueModulePtr<Platform>.

I think (b) won't work right now without more fixes because UniqueModulePtr relies on being given the implementation class in order to figure out which shared library to keep resident. And (a) I know we have done both accidentally and intentionally, so did not notice the problem for a while.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-01-14:

#

So in summary, this proposal is the only solution that presently works.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-01-14:

#

Also, lp:~andreas-pokorny/mir/fix-1528135 does not solve the problem. Just works around it sufficiently to declare bug 1528135 fixed. But that doesn't help us at all here.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-01-14:

#

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

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-01-14:

#

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

review: Approve (continuous-integration)

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-01-14:

#

Download full text (3.7 KiB)

Oh I am mostly arguing against your claim that UniqueModulePtr is unsafe. I havent looked into this bug properly...

> Returning UniqueModulePtr from your factory ABI within a plugin theoretically
> sounds like an improvement but requires a few pieces to be moved around and
> I'm not yet convinced it's sufficiently safe.

No as stated above this is the way it is meant to be used. And you might have noticed that already use it on the server side for that. And a closer look will show you that bug: #1528135 only exists because UniqueModulePtr allowed us to remove global variables that kept plugins alive.

> Forgive me for repeating but I want to restate the problem more concisely for
> other readers' and my own benefit:
>
> Let's imagine you have a plugin named mesa.so which implements interface
> Platform with its own MesaPlatform class. Your application knows nothing about
> "mesa" and contains no symbols with "mesa" in their name. Everything named
> "mesa" lives in mesa.so.
> Now you load your plugin "mesa.so" and it returns a
> UniqueModulePtr<MesaPlatform>. Because the only part of your project that
> knows the word "mesa" is mesa.so, the UniqueModulePtr<MesaPlatform> is fully
> and only instantiated inside mesa.so.
> Now you unload your plugin and three things get torn down, in this order:
> 1. ~MesaPlatform inside mesa.so
> 2. Unload/unmap mesa.so from memory.
> 3. ~UniqueModulePtr<MesaPlatform> which was inside mesa.so, but is no
> longer loaded so you crash.

The starting idea "Because the only part of your project that
knows the word "mesa" is mesa.so, the UniqueModulePtr<MesaPlatform> is fully
and only instantiated inside mesa.so." is already wrong.

Instead it should read. "A part of mirclient needs to create the ClientPlatfrom to be used, So it loads the right plugin and resolved create_client_platform and stores the returned UniqueModulePtr somewhere, then forgets about the mir::SharedLibrary it needed for that".
Then exactly 3 is not happening; the destructor call to the mir::client::ClientPlatform is encoded where the destructor call of the unique_ptr is encoded. The Deleter of that UniqueModulePtr will ensure that the SharedLibrary outlives the ClientPlatform object. Even when someone stores another UniqueModulePtr inside an object 'owned' by mesa.so (==ClientPlatform) wouldnt matter since someone still owns mesa.so(==ClientPlatform) so keeps it alive even when the object created and stored inside for whatever reason goes away. The important part here is that the plugin lifetime is now tied to the object lifetime created form it.

If for some reason the ClientPlatform creates objects for others to own that may outlive the ClientPlatform instance those need to be UniqueModulePtr too. See mir::graphics::Platform. Which is kind of a hint that graphics::Platform might be two or three separate things actually.

>
> Yes, in theory you could avoid the crash, but only by either:
> (a) Leaking mesa.so by any means, accidental or intentional; or
> (b) Ensuring UniqueModulePtr is only used on the interface:
> UniqueModulePtr<Platform>.
>
> I think (b) won't work right now without more fixes because UniqueModulePtr
> re...

Oh I am mostly arguing against your claim that UniqueModulePtr is unsafe. I havent looked into this bug properly...

> Returning UniqueModulePtr from your factory ABI within a plugin theoretically
> sounds like an improvement but requires a few pieces to be moved around and
> I'm not yet convinced it's sufficiently safe.

No as stated above this is the way it is meant to be used. And you might have noticed that already use it on the server side for that. And a closer look will show you that bug: #1528135 only exists because UniqueModulePtr allowed us to remove global variables that kept plugins alive.

> Forgive me for repeating but I want to restate the problem more concisely for
> other readers' and my own benefit:
> 
> Let's imagine you have a plugin named mesa.so which implements interface
> Platform with its own MesaPlatform class. Your application knows nothing about
> "mesa" and contains no symbols with "mesa" in their name. Everything named
> "mesa" lives in mesa.so.
>   Now you load your plugin "mesa.so" and it returns a
> UniqueModulePtr<MesaPlatform>. Because the only part of your project that
> knows the word "mesa" is mesa.so, the UniqueModulePtr<MesaPlatform> is fully
> and only instantiated inside mesa.so.
>   Now you unload your plugin and three things get torn down, in this order:
>    1. ~MesaPlatform inside mesa.so
>    2. Unload/unmap mesa.so from memory.
>    3. ~UniqueModulePtr<MesaPlatform> which was inside mesa.so, but is no
> longer loaded so you crash.

The starting idea "Because the only part of your project that
knows the word "mesa" is mesa.so, the UniqueModulePtr<MesaPlatform> is fully
and only instantiated inside mesa.so." is already wrong.

Instead it should read. "A part of mirclient needs to create the ClientPlatfrom to be used, So it loads the right plugin and resolved create_client_platform and stores the returned UniqueModulePtr somewhere, then forgets about the mir::SharedLibrary it needed for that".
Then exactly 3 is not happening; the destructor call to the mir::client::ClientPlatform is encoded where the destructor call of the unique_ptr is encoded. The Deleter of that UniqueModulePtr will ensure that the SharedLibrary outlives the ClientPlatform object. Even when someone stores another UniqueModulePtr inside an object 'owned' by mesa.so (==ClientPlatform) wouldnt matter since someone still owns mesa.so(==ClientPlatform) so keeps it alive even when the object created and stored inside for whatever reason goes away. The important part here is that the plugin lifetime is now tied to the object lifetime created form it.

If for some reason the ClientPlatform creates objects for others to own that may outlive the ClientPlatform instance those need to be UniqueModulePtr too. See mir::graphics::Platform. Which is kind of a hint that graphics::Platform might be two or three separate things actually.

> 
> Yes, in theory you could avoid the crash, but only by either:
>   (a) Leaking mesa.so by any means, accidental or intentional; or
>   (b) Ensuring UniqueModulePtr is only used on the interface:
> UniqueModulePtr<Platform>.
> 
> I think (b) won't work right now without more fixes because UniqueModulePtr
> relies on being given the implementation class in order to figure out which
> shared library to keep resident. And (a) I know we have done both accidentally
> and intentionally, so did not notice the problem for a while.

b) even works when you use UniqueModulePtr inside, as long as the lifetime of those instances is tied to objects created at the interface of the plugin. This is not really different than other lifetime semantics. And of course just like with the normal unique_ptr you do not have to use the derived class name in the interface and use the base class instead.