Merge lp:~vanvugt/mir/fix-1493721 into lp:mir

[Daniel van Vugt]

Fix Mir server crash when Xmir -sw connects (or any fullscreen opaque
software client in theory) (LP: #1493721)

The problem was the bypass logic prematurely cast native buffer objects
into GBM buffer objects, without checking if that's safe.

.

[Alan Griffiths (alan-griffiths)]

+ Mock::VerifyAndClearExpectations(&mock_gbm);
...
+
+ Mock::VerifyAndClearExpectations(&mock_gbm);
...
+
+ Mock::VerifyAndClearExpectations(&mock_gbm);
...
+
+ Mock::VerifyAndClearExpectations(&mock_gbm);
...
+
+ Mock::VerifyAndClearExpectations(&mock_gbm);

Something seems wrong here. mock_gbm is destroyed on test exit and its expectations validated there. Why do we need these (apparently redundant) explicit check just before test exit?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2985
http://jenkins.qa.ubuntu.com/job/mir-ci/5090/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4214
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3121
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4156
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/114/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1244
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1244/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/114/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4157
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4157/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6835
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23841
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/115/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2988
http://jenkins.qa.ubuntu.com/job/mir-ci/5105/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4231
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3138
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4173
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/131/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1259
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1259/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/131/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4174
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4174/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6847
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23871
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/132/console

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

+ Mock::VerifyAndClearExpectations(&mock_gbm);

Something seems wrong here. mock_gbm is destroyed on test exit and its expectations validated there. Why do we need this (apparently redundant) explicit check just before test exit?

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> It was required at the time to fix apparent expectation leakage between
> successive tests. But seemingly isn't needed today.

Whatever happened I think you misinterpreted it. The test object is torn down and destructed after each test (and that verifies the expectations). Could you have been seeing a race between satisfying the expectation and the tear down and destruction of the test object?

[Daniel van Vugt (vanvugt)]

Torn down _and_ destructed? I thought it was only Teardown() but not destructed. Hence mock_gbm lingers between tests.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2990
http://jenkins.qa.ubuntu.com/job/mir-ci/5129/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4262
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3169
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4204
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/162/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1283
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1283/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/162/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4205
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4205/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6875
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23930
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/163/console

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> Torn down _and_ destructed? I thought it was only Teardown() but not
> destructed. Hence mock_gbm lingers between tests.

Yes, hence it doesn't linger.

review: Approve

[Chris Halse Rogers (raof)]

Seems sensible.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1941/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4307
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3214
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4249/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/207/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/496/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/207/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4250
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4250/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6909/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23985
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/208/console

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2992
http://jenkins.qa.ubuntu.com/job/mir-ci/5185/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4330
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3237
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4272
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/230/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1339
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1339/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/230/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4273
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4273/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/6933
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24018
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/231/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1946/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4338
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3245/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4280
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/238/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/501
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/501/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/238/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4281
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4281/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/6940
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24026
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/239/console

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> [ FAILED ] AndroidInputReceiverSetup.slow_raw_input_doesnt_cause_frameskipping

Doesn't seem related to this MP.

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/platforms/mesa/server/kms/display_buffer.cpp'
--- src/platforms/mesa/server/kms/display_buffer.cpp	2015-10-01 08:58:30 +0000
+++ src/platforms/mesa/server/kms/display_buffer.cpp	2015-10-07 02:52:34 +0000
@@ -200,24 +200,23 @@
         {
             auto bypass_buffer = (*bypass_it)->buffer();
             auto native = bypass_buffer->native_buffer_handle();
-            auto gbm_native = static_cast<mgm::GBMNativeBuffer*>(native.get());
-            auto bufobj = get_buffer_object(gbm_native->bo);
-            if (bufobj &&
-                native->flags & mir_buffer_flag_can_scanout &&
+            if (native->flags & mir_buffer_flag_can_scanout &&
                 bypass_buffer->size() == geom::Size{fb_width,fb_height})
             {
-                bypass_buf = bypass_buffer;
-                bypass_bufobj = bufobj;
-                return true;
-            }
-            else
-            {
-                bypass_buf = nullptr;
-                bypass_bufobj = nullptr;
+                auto gbm_native =
+                    static_cast<mgm::GBMNativeBuffer*>(native.get());
+                if (auto bufobj = get_buffer_object(gbm_native->bo))
+                {
+                    bypass_buf = bypass_buffer;
+                    bypass_bufobj = bufobj;
+                    return true;
+                }
             }
         }
     }
 
+    bypass_buf = nullptr;
+    bypass_bufobj = nullptr;
     return false;
 }
 

=== modified file 'tests/unit-tests/graphics/mesa/kms/test_display_buffer.cpp'
--- tests/unit-tests/graphics/mesa/kms/test_display_buffer.cpp	2015-10-01 08:58:30 +0000
+++ tests/unit-tests/graphics/mesa/kms/test_display_buffer.cpp	2015-10-07 02:52:34 +0000
@@ -50,10 +50,15 @@
 
     MesaDisplayBufferTest()
         : mock_bypassable_buffer{std::make_shared<NiceMock<MockBuffer>>()}
+        , mock_software_buffer{std::make_shared<NiceMock<MockBuffer>>()}
         , fake_bypassable_renderable{
              std::make_shared<FakeRenderable>(display_area)}
+        , fake_software_renderable{
+             std::make_shared<FakeRenderable>(display_area)}
         , stub_gbm_native_buffer{
              std::make_shared<StubGBMNativeBuffer>(display_area.size)}
+        , stub_shm_native_buffer{
+             std::make_shared<MirNativeBuffer>()}
         , bypassable_list{fake_bypassable_renderable}
     {
         ON_CALL(mock_egl, eglChooseConfig(_,_,_,1,_))
@@ -88,6 +93,15 @@
         ON_CALL(*mock_bypassable_buffer, native_buffer_handle())
             .WillByDefault(Return(stub_gbm_native_buffer));
         fake_bypassable_renderable->set_buffer(mock_bypassable_buffer);
+
+        memset(stub_shm_native_buffer.get(), 0, sizeof(MirNativeBuffer));
+        stub_shm_native_buffer->flags = 0;  // Is not a hardware/GBM buffer
+
+        ON_CALL(*mock_software_buffer, size())
+            .WillByDefault(Return(display_area.size));
+        ON_CALL(*mock_software_buffer, native_buffer_handle())
+            .WillByDefault(Return(stub_shm_native_buffer));
+        fake_software_renderable->set_buffer(mock_software_buffer);
     }
 
     // The platform has an implicit dependency on mock_gbm etc so must be
@@ -106,15 +120,17 @@
     NiceMock<MockGL>  mock_gl;
     NiceMock<MockDRM> mock_drm; 
     std::shared_ptr<MockBuffer> mock_bypassable_buffer;
+    std::shared_ptr<MockBuffer> mock_software_buffer;
     std::shared_ptr<FakeRenderable> fake_bypassable_renderable;
+    std::shared_ptr<FakeRenderable> fake_software_renderable;
     std::shared_ptr<mesa::GBMNativeBuffer> stub_gbm_native_buffer;
+    std::shared_ptr<MirNativeBuffer> stub_shm_native_buffer;
     gbm_bo*           fake_bo;
     gbm_bo_handle     fake_handle;
     UdevEnvironment   fake_devices;
     std::shared_ptr<MockKMSOutput> mock_kms_output;
     StubGLConfig gl_config;
     mir::graphics::RenderableList const bypassable_list;
-
 };
 
 TEST_F(MesaDisplayBufferTest, unrotated_view_area_is_untouched)
@@ -308,6 +324,49 @@
     EXPECT_FALSE(db.post_renderables_if_optimizable(bypassable_list));
 }
 
+TEST_F(MesaDisplayBufferTest, fullscreen_software_buffer_cannot_bypass)
+{
+    graphics::RenderableList const list{fake_software_renderable};
+
+    // Passes the bypass candidate test:
+    EXPECT_EQ(fake_software_renderable->buffer()->size(), display_area.size);
+
+    graphics::mesa::DisplayBuffer db(
+        create_platform(),
+        null_display_report(),
+        {},
+        nullptr,
+        display_area,
+        mir_orientation_normal,
+        gl_config,
+        mock_egl.fake_egl_context);
+
+    EXPECT_FALSE(db.post_renderables_if_optimizable(list));
+}
+
+TEST_F(MesaDisplayBufferTest, fullscreen_software_buffer_not_used_as_gbm_bo)
+{   // Also checks it doesn't crash (LP: #1493721)
+    graphics::RenderableList const list{fake_software_renderable};
+
+    // Passes the bypass candidate test:
+    EXPECT_EQ(fake_software_renderable->buffer()->size(), display_area.size);
+
+    graphics::mesa::DisplayBuffer db(
+        create_platform(),
+        null_display_report(),
+        {},
+        nullptr,
+        display_area,
+        mir_orientation_normal,
+        gl_config,
+        mock_egl.fake_egl_context);
+
+    // If you find yourself using gbm_ functions on a Shm buffer then you're
+    // asking for a crash (LP: #1493721) ...
+    EXPECT_CALL(mock_gbm, gbm_bo_get_user_data(_)).Times(0);
+    db.post_renderables_if_optimizable(list);
+}
+
 TEST_F(MesaDisplayBufferTest, orientation_not_implemented_internally)
 {
     graphics::mesa::DisplayBuffer db(