libmirprotobuf's ABI can be broken when modifying protobuf message definitions
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Mir |
Fix Released
|
High
|
Alberto Aguirre | ||
0.13 |
Fix Released
|
High
|
Alberto Aguirre | ||
mir (Ubuntu) |
Fix Released
|
High
|
Unassigned |
Bug Description
libmirprotobuf's ABI can be broken when modifying protobuf message definitions.
This is normally not an issue because updates keep replacing libmirclient.so.8 unless we bump client ABI.
Existing clients linked to libmirclient8 (like glmark2) will use the old message layouts, however when linked to newer versions of libmirprotobuf.so.0 at runtime those messages can be bigger or potentially different. Allocating these messages on the stack in libmirclient means this causes stack corruption and crashes.
Ideally, we should bump libmirprotobuf ABI but that's not currently possible since:
- protobuf design does not allow symbols to be registered twice
- libmirprotobuf does not version its symbols
- consequently a libmirprotobuf.so.0 and libmirprotobuf.so.1 cannot be loaded in the same process at the same time
- Loading two libmirprotobuf library versions at the same time would be common since the mesa client platform module is linked against libmirclient - just probing the module would cause a new version of libmirprotobuf to load
The mir team most commonly just adds additional fields to a protobuf message. To improve compatibility in this case, we can avoid allocating protobuf messages on the stack and instead use the factory methods provided by the generated message classes which are defined in libmirprotobuf itself.
We still need to tread carefully when modifying protobuf message definitions however, to avoid changing class layouts in a non-compatible way (like removing fields or adding fields in the midle of existing ones)
Related branches
- Alan Griffiths: Approve
- Chris Halse Rogers: Approve
- Daniel van Vugt: Needs Fixing
- PS Jenkins bot (community): Approve (continuous-integration)
-
Diff: 1413 lines (+308/-238)12 files modifiedsrc/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)
- Alan Griffiths: Disapprove
- PS Jenkins bot (community): Needs Fixing (continuous-integration)
-
Diff: 32 lines (+3/-3)3 files modifieddebian/control (+1/-1)
debian/libmirprotobuf1.install (+1/-1)
src/protobuf/CMakeLists.txt (+1/-1)
Changed in mir: | |
milestone: | none → 0.14.0 |
summary: |
- avoid allocating mir protobuf message objects on the stack + MIRPROTOBUF_ABI is being broken by message design changes, but + MIRPROTOBUF_ABI is never bumped resulting in client crashes. |
summary: |
- MIRPROTOBUF_ABI is being broken by message design changes, but - MIRPROTOBUF_ABI is never bumped resulting in client crashes. + libmirprotobuf's ABI has been broken by message design changes, but + MIRPROTOBUF_ABI was never bumped, resulting in client crashes. |
description: | updated |
Changed in mir: | |
importance: | Medium → High |
description: | updated |
summary: |
- libmirprotobuf's ABI has been broken by message design changes, but - MIRPROTOBUF_ABI was never bumped, resulting in client crashes. + libmirprotobuf's ABI has been broken by message design changes, + resulting in client crashes and/or leaks |
summary: |
- libmirprotobuf's ABI has been broken by message design changes, - resulting in client crashes and/or leaks + avoid allocating mir protobuf message objects on the stack |
summary: |
- avoid allocating mir protobuf message objects on the stack + Avoid allocating mir protobuf message objects on the stack |
description: | updated |
description: | updated |
Changed in mir: | |
status: | Fix Committed → Fix Released |
I think bug 1341490 might cover this actually.