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

[Brandon Schaefer]

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.

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/

[Alberto Aguirre (albaguirre)]

Ok.

review: Approve

[Michał Sawicz (saviq)]

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)]

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

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:4247
https://mir-jenkins.ubuntu.com/job/mir-ci/3657/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/5005/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5233
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5221
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5221
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5221
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/5044/console
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/5044/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3657/rebuild

review: Needs Fixing (continuous-integration)

[Brandon Schaefer (brandontschaefer)]

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)]

Alan has a better fix, rejecting!

=== modified file 'src/miral/window_info.cpp'
--- src/miral/window_info.cpp	2017-08-21 14:18:55 +0000
+++ src/miral/window_info.cpp	2017-09-13 19:50:44 +0000
@@ -256,14 +256,14 @@
     {
         auto const width = new_size.width.as_int() - min_width().as_int();
         auto inc = width_inc().as_int();
-        if (width % inc)
+        if (inc > 0 && width % inc)
             new_size.width = min_width() + DeltaX{inc*(((2L*width + inc)/2)/inc)};
     }
 
     {
         auto const height = new_size.height.as_int() - min_height().as_int();
         auto inc = height_inc().as_int();
-        if (height % inc)
+        if (inc > 0 && height % inc)
             new_size.height = min_height() + DeltaY{inc*(((2L*height + inc)/2)/inc)};
     }
 
@@ -272,7 +272,7 @@
 
         auto const error = new_size.height.as_int()*long(ar.width) - new_size.width.as_int()*long(ar.height);
 
-        if (error > 0)
+        if (error > 0 && ar.width > 0 && ar.height > 0)
         {
             // Add (denominator-1) to numerator to ensure rounding up
             auto const width_correction  = (error+(ar.height-1))/ar.height;
@@ -294,7 +294,7 @@
 
         auto const error = new_size.width.as_int()*long(ar.height) - new_size.height.as_int()*long(ar.width);
 
-        if (error > 0)
+        if (error > 0 && ar.width > 0 && ar.height > 0)
         {
             // Add (denominator-1) to numerator to ensure rounding up
             auto const height_correction = (error+(ar.width-1))/ar.width;

=== modified file 'tests/miral/CMakeLists.txt'
--- tests/miral/CMakeLists.txt	2017-08-31 15:12:09 +0000
+++ tests/miral/CMakeLists.txt	2017-09-13 19:50:44 +0000
@@ -44,6 +44,7 @@
     workspaces.cpp
     drag_and_drop.cpp
     client_mediated_gestures.cpp
+    window_info.cpp
 )
 
 target_link_libraries(miral-test

=== added file 'tests/miral/window_info.cpp'
--- tests/miral/window_info.cpp	1970-01-01 00:00:00 +0000
+++ tests/miral/window_info.cpp	2017-09-13 19:50:44 +0000
@@ -0,0 +1,61 @@
+/*
+ * Copyright © 2017 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 or 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Brandon Schaefer <[e-mail]>
+ */
+
+#include <miral/window_info.h>
+
+#include "mir_test_framework/process.h"
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+using namespace miral;
+namespace mtf = mir_test_framework;
+
+namespace
+{
+static int a_successful_exit_function()
+{
+    return EXIT_SUCCESS;
+}
+}
+
+// Test for crash http://paste.ubuntu.com/25523431/
+TEST(WindowInfo, negative_window_size_does_not_divide_by_zero)
+{
+    auto p = mtf::fork_and_run_in_a_different_process(
+        [] {
+            Window window;
+            WindowSpecification params;
+
+            Point p{0, 0};
+            Size  s{-300, -300};
+
+            params.name()     = "";
+            params.top_left() = p;
+            params.size()     = s;
+
+            WindowInfo info(window, params);
+
+            info.min_width(s.width);
+            info.min_height(s.height);
+
+            info.constrain_resize(p, s);
+        }, a_successful_exit_function);
+
+    EXPECT_TRUE(p->wait_for_termination().succeeded());
+}