Mir

Merge lp:~raof/mir/robust-surface-is-valid into lp:mir

  1. robust-surface-is-valid
  2. Merge into development-branch
Proposed by Chris Halse Rogers
Status: Merged
Approved by: Alan Griffiths
Approved revision: no longer in the source branch.
Merged at revision: 1658
Proposed branch: lp:~raof/mir/robust-surface-is-valid
Merge into: lp:mir
Diff against target: 136 lines (+46/-6)
5 files modified
src/client/mir_surface.cpp (+16/-4)
src/client/mir_surface.h (+2/-1)
src/client/mir_surface_api.cpp (+1/-1)
tests/unit-tests/client/test_client.cpp (+13/-0)
tests/unit-tests/client/test_client_mir_surface.cpp (+14/-0)
To merge this branch: bzr merge lp:~raof/mir/robust-surface-is-valid
Reviewer Review Type Date Requested Status
Andreas Pokorny (community) Approve
Alberto Aguirre (community) Approve
Robert Carr (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+220762@code.launchpad.net

Commit message

Make mir_surface_is_valid robust against passing in invalid surfaces.

Fixes: https://bugs.launchpad.net/mir/+bug/1248474

Description of the change

Make mir_surface_is_valid robust against passing in invalid surfaces.

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

PASSED: Continuous integration, rev:1657
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1720/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/316
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/317
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/316
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/239
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/239/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/238
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/238/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/854
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/854/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1560
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7575

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1720/rebuild

review: Approve (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote :

Seems like a good choice.

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

Seems sensible

review: Approve
Revision history for this message
Andreas Pokorny (andreas-pokorny) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/client/mir_surface.cpp'
2--- src/client/mir_surface.cpp 2014-05-22 11:31:21 +0000
3+++ src/client/mir_surface.cpp 2014-05-23 07:57:17 +0000
4@@ -37,6 +37,9 @@
5 namespace
6 {
7 void null_callback(MirSurface*, void*) {}
8+
9+std::mutex handle_mutex;
10+std::unordered_set<MirSurface*> valid_surfaces;
11 }
12
13 MirSurface::MirSurface(
14@@ -66,6 +69,9 @@
15 attrib_cache[mir_surface_attrib_type] = mir_surface_type_normal;
16 attrib_cache[mir_surface_attrib_state] = mir_surface_state_unknown;
17 attrib_cache[mir_surface_attrib_swapinterval] = 1;
18+
19+ std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
20+ valid_surfaces.insert(this);
21 }
22
23 MirSurface::~MirSurface()
24@@ -115,11 +121,14 @@
25 return surface.id().value();
26 }
27
28-bool MirSurface::is_valid() const
29+bool MirSurface::is_valid(MirSurface* query)
30 {
31- std::lock_guard<decltype(mutex)> lock(mutex);
32-
33- return !surface.has_error();
34+ std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
35+
36+ if (valid_surfaces.count(query))
37+ return !query->surface.has_error();
38+
39+ return false;
40 }
41
42 void MirSurface::get_cpu_region(MirGraphicsRegion& region_out)
43@@ -234,6 +243,9 @@
44 mir_surface_callback callback,
45 void * context)
46 {
47+ std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
48+ valid_surfaces.erase(this);
49+
50 return connection->release_surface(this, callback, context);
51 }
52
53
54=== modified file 'src/client/mir_surface.h'
55--- src/client/mir_surface.h 2014-05-22 11:31:21 +0000
56+++ src/client/mir_surface.h 2014-05-23 07:57:17 +0000
57@@ -32,6 +32,7 @@
58 #include <memory>
59 #include <functional>
60 #include <mutex>
61+#include <unordered_set>
62
63 namespace mir
64 {
65@@ -74,7 +75,6 @@
66 MirSurfaceParameters get_parameters() const;
67 char const * get_error_message();
68 int id() const;
69- bool is_valid() const;
70 MirWaitHandle* next_buffer(mir_surface_callback callback, void * context);
71 MirWaitHandle* get_create_wait_handle();
72
73@@ -97,6 +97,7 @@
74 void request_and_wait_for_next_buffer();
75 void request_and_wait_for_configure(MirSurfaceAttrib a, int value);
76
77+ static bool is_valid(MirSurface* query);
78 private:
79 mutable std::mutex mutex; // Protects all members of *this
80
81
82=== modified file 'src/client/mir_surface_api.cpp'
83--- src/client/mir_surface_api.cpp 2014-05-22 11:31:21 +0000
84+++ src/client/mir_surface_api.cpp 2014-05-23 07:57:17 +0000
85@@ -83,7 +83,7 @@
86
87 MirBool mir_surface_is_valid(MirSurface* surface)
88 {
89- return surface->is_valid() ? mir_true : mir_false;
90+ return MirSurface::is_valid(surface) ? mir_true : mir_false;
91 }
92
93 char const* mir_surface_get_error_message(MirSurface* surface)
94
95=== modified file 'tests/unit-tests/client/test_client.cpp'
96--- tests/unit-tests/client/test_client.cpp 2013-04-24 05:22:20 +0000
97+++ tests/unit-tests/client/test_client.cpp 2014-05-23 07:57:17 +0000
98@@ -36,3 +36,16 @@
99 ASSERT_FALSE(mir_connection_is_valid(not_a_mir_connection_on_the_stack));
100 ASSERT_FALSE(mir_connection_is_valid(not_a_mir_connection_on_the_heap));
101 }
102+
103+TEST(MirClientTest, mir_surface_is_valid_handles_invalid_pointers)
104+{
105+ MirSurface* null_pointer = NULL;
106+ double stack_variable;
107+ MirSurface* not_a_mir_surface_on_the_stack = reinterpret_cast<MirSurface*>(&stack_variable);
108+ auto heap_variable = std::make_shared<int>();
109+ MirSurface* not_a_mir_surface_on_the_heap = reinterpret_cast<MirSurface*>(heap_variable.get());
110+
111+ ASSERT_FALSE(mir_surface_is_valid(null_pointer));
112+ ASSERT_FALSE(mir_surface_is_valid(not_a_mir_surface_on_the_stack));
113+ ASSERT_FALSE(mir_surface_is_valid(not_a_mir_surface_on_the_heap));
114+}
115
116=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
117--- tests/unit-tests/client/test_client_mir_surface.cpp 2014-03-06 06:05:17 +0000
118+++ tests/unit-tests/client/test_client_mir_surface.cpp 2014-05-23 07:57:17 +0000
119@@ -731,3 +731,17 @@
120 EXPECT_EQ(mock_server_tool->pf_sent, region.pixel_format);
121 }
122 }
123+
124+TEST_F(MirClientSurfaceTest, valid_surface_is_valid)
125+{
126+ auto surface = std::make_shared<MirSurface> (connection.get(),
127+ *client_comm_channel,
128+ mock_buffer_factory,
129+ input_platform,
130+ params,
131+ &empty_callback,
132+ nullptr);
133+ surface->get_create_wait_handle()->wait_for_all();
134+
135+ EXPECT_TRUE(surface->is_valid(surface.get()));
136+}

Subscribers

People subscribed via source and target branches