Mir

Merge lp://qastaging/~albaguirre/mir/fix-1465883 into lp://qastaging/mir

  1. fix-1465883
  2. Merge into development-branch
Proposed by Alberto Aguirre
Status: Merged
Approved by: Alberto Aguirre
Approved revision: no longer in the source branch.
Merged at revision: 2674
Proposed branch: lp://qastaging/~albaguirre/mir/fix-1465883
Merge into: lp://qastaging/mir
Diff against target: 1413 lines (+308/-238)
12 files modified
src/client/buffer_stream.cpp (+38/-37)
src/client/buffer_stream.h (+3/-3)
src/client/make_protobuf_object.h (+42/-0)
src/client/mir_connection.cpp (+56/-49)
src/client/mir_connection.h (+8/-6)
src/client/mir_prompt_session.cpp (+21/-15)
src/client/mir_prompt_session.h (+5/-5)
src/client/mir_screencast.cpp (+24/-21)
src/client/mir_screencast.h (+4/-2)
src/client/mir_surface.cpp (+83/-76)
src/client/mir_surface.h (+6/-7)
src/client/rpc/mir_protobuf_rpc_channel.cpp (+18/-17)
To merge this branch: bzr merge lp://qastaging/~albaguirre/mir/fix-1465883
Reviewer Review Type Date Requested Status
Alan Griffiths Approve
Chris Halse Rogers Approve
Daniel van Vugt Needs Fixing
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+262160@code.qastaging.launchpad.net

Commit message

Avoid allocating mir::protobuf objects on the stack

When an addition is made to a protobuf message, a stack allocation of such object may differ from what the destructor of that message is expecting as the destructor is defined by libmirprotobuf and the allocation may have been made from an older definition of the message (by an older mirclient library for example) which can lead to stack corruption.

Ideally, the mirprotobuf library would have versioned symbols and have the ability to be loaded in parallel to a previous version but that's not currently possible.

As an alternative, avoid allocating mir defined protobuf objects on the stack and instead use the xxx::default_instance().New() factory methods which are defined in the mirprotobuf library.

Description of the change

Avoid allocating mir::protobuf objects on the stack

When an addition is made to a protobuf message, a stack allocation of such object may differ from what the destructor of that message is expecting as the destructor is defined by libmirprotobuf and the allocation may have been made from an older definition of the message (by an older mirclient library for example) which can lead to stack corruption.

Ideally, the mirprotobuf library would have versioned symbols and have the ability to be loaded in parallel to a previous version but that's not currently possible.

As an alternative, avoid allocating mir defined protobuf objects on the stack and instead use the xxx::default_instance().New() factory methods which are defined in the mirprotobuf library.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2671
http://jenkins.qa.ubuntu.com/job/mir-ci/4120/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2880
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/393
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2828
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/276
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/276/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2828
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2828/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5658
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21235

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

review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Slightly scary no one figured this out earlier.

A worse way to think of the bug is: If a new field is added to a message then any existing code will try to construct that message on the stack using its old (smaller) size while Protobuf itself fills out the new fields (which we didn't allow for in the stack allocation). Hence stack corruption.

The solution presented here however fails to work in all cases of ABI breaks such as when a member is removed from the middle of the object. Not likely, but there is a better option...

The issue is really that we're breaking MIRPROTOBUF_ABI but have never thought to bump it. If it was bumped any any change to message sizes/structure then everyone would always be linked to a version that is safe for them. And bumping MIRPROTOBUF_ABI would mean we could keep our nice fast stack variables.

review: Needs Information
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

So I think Disapprove/Needs fixing. A simple bump of MIRPROTOBUF_ABI will solve it more reliably and give us more efficient code. Unless anyone can prove there's a protobuf issue preventing us from doing that.

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

It's also worth noting you don't need versioned symbols at all if your major ABI number is changing instead.

Revision history for this message
Chris Halse Rogers (raof) wrote :

There *is* a protobuf issue preventing us from doing that; see for example the failures on input-methods-can-... when I bumped protobuf ABI (eg: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5597/console )

The issue is that protobuf will abort() if it's loaded twice, and bumping mirprotobuf ABI ensures that it's loaded twice. Otherwise we wouldn't be building libmirprotobuf.so at all, and would be statically linking it as nature intended.

Revision history for this message
Chris Halse Rogers (raof) wrote :

So, urgh. Yeah, this is better than what we've got.

Is there any obvious way to test that we don't stack-create protobuf objects? It'd be nice to have a test, but I can't think of one.

review: Approve
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> So I think Disapprove/Needs fixing. A simple bump of MIRPROTOBUF_ABI will
> solve it more reliably and give us more efficient code. Unless anyone can
> prove there's a protobuf issue preventing us from doing that.

It really isn't that simple: protobuf has a number of issues in this area. Essentially, we can only have one protobuf instantiation in process.

We've "got away" with it while the client ABI has remained at 8 (as only the latest limbirprotobuf gets loaded). Now we're proposing 9 we need to keep the same limbirprotobuf version.

Alexandros has tried to upstream some fixes to protobuf, and Kevin and Chris have recently run into problems.

AFAICS the approach proposed here is the least bad of the feasible options.

review: Approve
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

@Daniel,

As others have said, yes there are protobuf issues preventing loading two versions in the same process (for more comments see Chris branch https://code.launchpad.net/~raof/mir/input-methods-can-specify-foreign-parents/+merge/258001)

Loading two versions of protobuf in the same process is more common than is apparent at first glance, because the mesa client platform module links against libmirclient.

Indeed, just by probing the modules, a client that links against libmirclient8, will end up loading libmirprotobuf.so.1 since the module will load libmirclient9 (which is linked to libmirprotobuf.so.1)

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

"The solution presented here however fails to work in all cases of ABI breaks such as when a member is removed from the middle of the object. Not likely, but there is a better option..."

This is true, we still need to tread carefully when modifying protobuf message definitions.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches