Mir

Merge lp:~brandontschaefer/mir/dont-divide-by-zero-miral into lp:mir

Proposed by Brandon Schaefer on 2017-09-13
Status: Rejected
Rejected by: Brandon Schaefer on 2017-09-18
Proposed branch: lp:~brandontschaefer/mir/dont-divide-by-zero-miral
Merge into: lp:mir
Diff against target: 116 lines (+66/-4)
3 files modified
src/miral/window_info.cpp (+4/-4)
tests/miral/CMakeLists.txt (+1/-0)
tests/miral/window_info.cpp (+61/-0)
To merge this branch: bzr merge lp:~brandontschaefer/mir/dont-divide-by-zero-miral
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Needs Fixing on 2017-09-14
Alan Griffiths Needs Information on 2017-09-14
Alberto Aguirre (community) 2017-09-13 Approve on 2017-09-13
Review via email: mp+330702@code.launchpad.net

Commit message

When a client gives a negative window width or height and leaves the default aspect ratio a divide by zero will happen when calculating width/height correction.

Description of the change

When a user gives a negative window width or height and leaves the default aspect ratio a divide by zero will happen when calculating width/height correction.

Stacktrace
http://paste.ubuntu.com/25523431/

To post a comment you must log in.
Alberto Aguirre (albaguirre) wrote :

Ok.

review: Approve
Michał Sawicz (saviq) wrote :

Until we have new Mir in xenial, would you please submit that against lp:miral as well, this way we'd get it sooner in mir-libs.

Alan Griffiths (alan-griffiths) wrote :

Isn't this fixing the wrong thing?

We should not be allowing WindowInfo::width_inc()/height_inc() to be non-positive.

AFAICS a client attempting this ought to have its request rejected.

review: Needs Information
Brandon Schaefer (brandontschaefer) wrote :

https://code.launchpad.net/~brandontschaefer/miral/fix-unsigned-to-long-overflow/+merge/330778

WIP for now here, will most likely merge ^ if that one is approved

Brandon Schaefer (brandontschaefer) wrote :

Alan has a better fix, rejecting!

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/miral/window_info.cpp'
2--- src/miral/window_info.cpp 2017-08-21 14:18:55 +0000
3+++ src/miral/window_info.cpp 2017-09-13 19:50:44 +0000
4@@ -256,14 +256,14 @@
5 {
6 auto const width = new_size.width.as_int() - min_width().as_int();
7 auto inc = width_inc().as_int();
8- if (width % inc)
9+ if (inc > 0 && width % inc)
10 new_size.width = min_width() + DeltaX{inc*(((2L*width + inc)/2)/inc)};
11 }
12
13 {
14 auto const height = new_size.height.as_int() - min_height().as_int();
15 auto inc = height_inc().as_int();
16- if (height % inc)
17+ if (inc > 0 && height % inc)
18 new_size.height = min_height() + DeltaY{inc*(((2L*height + inc)/2)/inc)};
19 }
20
21@@ -272,7 +272,7 @@
22
23 auto const error = new_size.height.as_int()*long(ar.width) - new_size.width.as_int()*long(ar.height);
24
25- if (error > 0)
26+ if (error > 0 && ar.width > 0 && ar.height > 0)
27 {
28 // Add (denominator-1) to numerator to ensure rounding up
29 auto const width_correction = (error+(ar.height-1))/ar.height;
30@@ -294,7 +294,7 @@
31
32 auto const error = new_size.width.as_int()*long(ar.height) - new_size.height.as_int()*long(ar.width);
33
34- if (error > 0)
35+ if (error > 0 && ar.width > 0 && ar.height > 0)
36 {
37 // Add (denominator-1) to numerator to ensure rounding up
38 auto const height_correction = (error+(ar.width-1))/ar.width;
39
40=== modified file 'tests/miral/CMakeLists.txt'
41--- tests/miral/CMakeLists.txt 2017-08-31 15:12:09 +0000
42+++ tests/miral/CMakeLists.txt 2017-09-13 19:50:44 +0000
43@@ -44,6 +44,7 @@
44 workspaces.cpp
45 drag_and_drop.cpp
46 client_mediated_gestures.cpp
47+ window_info.cpp
48 )
49
50 target_link_libraries(miral-test
51
52=== added file 'tests/miral/window_info.cpp'
53--- tests/miral/window_info.cpp 1970-01-01 00:00:00 +0000
54+++ tests/miral/window_info.cpp 2017-09-13 19:50:44 +0000
55@@ -0,0 +1,61 @@
56+/*
57+ * Copyright © 2017 Canonical Ltd.
58+ *
59+ * This program is free software: you can redistribute it and/or modify it
60+ * under the terms of the GNU General Public License version 2 or 3 as
61+ * published by the Free Software Foundation.
62+ *
63+ * This program is distributed in the hope that it will be useful,
64+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
65+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
66+ * GNU General Public License for more details.
67+ *
68+ * You should have received a copy of the GNU General Public License
69+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
70+ *
71+ * Authored by: Brandon Schaefer <brandon.schaefer@canonical.com>
72+ */
73+
74+#include <miral/window_info.h>
75+
76+#include "mir_test_framework/process.h"
77+
78+#include <gtest/gtest.h>
79+#include <gmock/gmock.h>
80+
81+using namespace miral;
82+namespace mtf = mir_test_framework;
83+
84+namespace
85+{
86+static int a_successful_exit_function()
87+{
88+ return EXIT_SUCCESS;
89+}
90+}
91+
92+// Test for crash http://paste.ubuntu.com/25523431/
93+TEST(WindowInfo, negative_window_size_does_not_divide_by_zero)
94+{
95+ auto p = mtf::fork_and_run_in_a_different_process(
96+ [] {
97+ Window window;
98+ WindowSpecification params;
99+
100+ Point p{0, 0};
101+ Size s{-300, -300};
102+
103+ params.name() = "";
104+ params.top_left() = p;
105+ params.size() = s;
106+
107+ WindowInfo info(window, params);
108+
109+ info.min_width(s.width);
110+ info.min_height(s.height);
111+
112+ info.constrain_resize(p, s);
113+ }, a_successful_exit_function);
114+
115+ EXPECT_TRUE(p->wait_for_termination().succeeded());
116+}

Subscribers

People subscribed via source and target branches