MirAL

Merge lp:~alan-griffiths/miral/fix-1717061 into lp:miral

fix-1717061
Merge into trunk

Proposed by Alan Griffiths on 2017-09-15

Status:

Merged

Approved by:

Alan Griffiths on 2017-09-20

Approved revision:

580

Merged at revision:

580

Proposed branch:

lp:~alan-griffiths/miral/fix-1717061

Merge into:

lp:miral

Diff against target:

350 lines (+174/-39)

7 files modified

miral/CMakeLists.txt (+1/-0)
miral/set_window_management_policy.cpp (+16/-16)
miral/window_info.cpp (+49/-15)
miral/window_info_defaults.h (+36/-0)
miral/window_management_trace.cpp (+10/-8)
test/CMakeLists.txt (+1/-0)
test/window_info.cpp (+61/-0)

To merge this branch:

bzr merge lp:~alan-griffiths/miral/fix-1717061

Critical

Fix Committed

Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Approve on 2017-09-20
Gerry Boland (community)		2017-09-15	Approve on 2017-09-18
Brandon Schaefer (community)			Approve on 2017-09-15
Review via email: mp+330832@code.launchpad.net

Commit message

Define default window settings in one place and clamp the actual values to avoid ldiv0. (LP: #1717061)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-09-15:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-09-15:

FAILED: Continuous integration, rev:580
https://mir-jenkins.ubuntu.com/job/miral-ci/77/
Executed test runs:
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-miral/123/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5243
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5231
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5231
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5231

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-09-15:

+unsigned clamp_dim(unsigned dim)
+{
+ return std::min<unsigned long>(std::numeric_limits<long>::max(), dim);
+}

There's a mix of types here: unsigned int, unsigned long and signed long, which is causing me problems understanding. I know sizeof(unsigned int) <= sizeof(long), which implies to me that

dim <= std::numeric_limits<unsigned>::max()
<= std::numeric_limits<long>::max()

so the min() will always choose "dim". So I don't understand what it is clamping to.

+// For our convenience when doing calculations we clamp the dimensions of the aspect ratio
+// so they will fit into a long without overflow.
What "long"? AspectRatio is a pair of unsigned ints.

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2017-09-15:

> +unsigned clamp_dim(unsigned dim)
> +{
> + return std::min<unsigned long>(std::numeric_limits<long>::max(), dim);
> +}
>
> There's a mix of types here: unsigned int, unsigned long and signed long,
> which is causing me problems understanding. I know sizeof(unsigned int) <=
> sizeof(long), which implies to me that
>
> dim <= std::numeric_limits<unsigned>::max()
> <= std::numeric_limits<long>::max()

when, as Brandon encountered on armhf, long and int are the same size.

std::numeric_limits<long>::max() <= std::numeric_limits<unsigned>::max()

> so the min() will always choose "dim". So I don't understand what it is
> clamping to.

In the above case it is clamping to std::numeric_limits<long>::max()

> +// For our convenience when doing calculations we clamp the dimensions of the
> aspect ratio
> +// so they will fit into a long without overflow.
> What "long"? AspectRatio is a pair of unsigned ints.

If std::numeric_limits<long>::max() <= dim <= std::numeric_limits<unsigned>::max() then dim doesn't fit in a long.

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2017-09-15:

Yeah this seems a bit more robust then what I was doing in my branch.

I was just figuring since sizeof(long) >= sizeof(int) we could just cheat and assign the aspect to int::max() vs unsigned::max() but you do end up losing a byte there if really wanted on amd64 machines.

Though wont it all be going through the values that are depended in the RPC? Should we at some point look at moving to proper int sizes (u)int8/16/32/64 to know we wont have changing int values depending on the architecture.

Also that test is sort of ... different then this overflow issue but still nice to test. Wasnt sure if we actually had more checking higher up the stack when we come from the RPC.

LGTM

review: Approve

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-09-18:

lgtm

review: Approve

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-09-18:

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-09-20:

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-09-20:

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) on 2017-09-20:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alan Griffiths

Mir development team

 === modified file 'miral/CMakeLists.txt'
 --- miral/CMakeLists.txt	2017-07-26 10:44:19 +0000
 +++ miral/CMakeLists.txt	2017-09-15 13:33:35 +0000
@@ -64,6 +64,7 @@
                                          ${miral_include}/mir/client/connection.h
                                          ${miral_include}/mir/client/display_config.h
                                          ${miral_include}/mir/client/window.h
++                                        window_info_defaults.h
+ )
  target_include_directories(mirclientcpp
 === modified file 'miral/set_window_management_policy.cpp'
 --- miral/set_window_management_policy.cpp	2017-08-10 16:59:22 +0000
 +++ miral/set_window_management_policy.cpp	2017-09-15 13:33:35 +0000
@@ -32,22 +32,6 @@
  char const* const trace_option = "window-management-trace";
+ }
--MIRAL_FAKE_OLD_SYMBOL(
--    _ZN5miral24SetWindowManagmentPolicyC1ERKSt8functionIFSt10unique_ptrINS_22WindowManagementPolicyESt14default_deleteIS3_EERKNS_18WindowManagerToolsEEE,
--    _ZN5miral25SetWindowManagementPolicyC1ERKSt8functionIFSt10unique_ptrINS_22WindowManagementPolicyESt14default_deleteIS3_EERKNS_18WindowManagerToolsEEE)
--
--MIRAL_FAKE_OLD_SYMBOL(
--    _ZNK5miral24SetWindowManagmentPolicyclERN3mir6ServerE,
--    _ZNK5miral25SetWindowManagementPolicyclERN3mir6ServerE)
--
--MIRAL_FAKE_OLD_SYMBOL(
--    _ZN5miral24SetWindowManagmentPolicyD1Ev,
--    _ZN5miral25SetWindowManagementPolicyD1Ev)
--
--MIRAL_FAKE_OLD_SYMBOL(
--    _ZN5miral24SetWindowManagmentPolicyD2Ev,
--    _ZN5miral25SetWindowManagementPolicyD2Ev)
--
  miral::SetWindowManagementPolicy::SetWindowManagementPolicy(WindowManagementPolicyBuilder const& builder) :
      builder{builder}
+ {
@@ -89,3 +73,19 @@
                  builder);
          });
+ }
++
++MIRAL_FAKE_OLD_SYMBOL(
++    _ZN5miral24SetWindowManagmentPolicyC1ERKSt8functionIFSt10unique_ptrINS_22WindowManagementPolicyESt14default_deleteIS3_EERKNS_18WindowManagerToolsEEE,
++    _ZN5miral25SetWindowManagementPolicyC1ERKSt8functionIFSt10unique_ptrINS_22WindowManagementPolicyESt14default_deleteIS3_EERKNS_18WindowManagerToolsEEE)
++
++MIRAL_FAKE_OLD_SYMBOL(
++    _ZNK5miral24SetWindowManagmentPolicyclERN3mir6ServerE,
++    _ZNK5miral25SetWindowManagementPolicyclERN3mir6ServerE)
++
++MIRAL_FAKE_OLD_SYMBOL(
++    _ZN5miral24SetWindowManagmentPolicyD1Ev,
++    _ZN5miral25SetWindowManagementPolicyD1Ev)
++
++MIRAL_FAKE_OLD_SYMBOL(
++    _ZN5miral24SetWindowManagmentPolicyD2Ev,
++    _ZN5miral25SetWindowManagementPolicyD2Ev)
 === modified file 'miral/window_info.cpp'
 --- miral/window_info.cpp	2017-08-10 16:59:22 +0000
 +++ miral/window_info.cpp	2017-09-15 13:33:35 +0000
@@ -17,6 +17,7 @@
   */
  #include "miral/window_info.h"
++#include "window_info_defaults.h"
  #include "both_versions.h"
@@ -35,7 +36,40 @@
+ {
      return optional_value.is_set() ? optional_value.value() : default_;
+ }
--}
++
++unsigned clamp_dim(unsigned dim)
++{
++    return std::min<unsigned long>(std::numeric_limits<long>::max(), dim);
++}
++
++// For our convenience when doing calculations we clamp the dimensions of the aspect ratio
++// so they will fit into a long without overflow.
++miral::WindowInfo::AspectRatio clamp(miral::WindowInfo::AspectRatio const& source)
++{
++    return {clamp_dim(source.width), clamp_dim(source.height)};
++}
++
++miral::Width clamp(miral::Width const& source)
++{
++    return std::min(miral::default_max_width, std::max(miral::default_min_width, source));
++}
++
++miral::Height clamp(miral::Height const& source)
++{
++    return std::min(miral::default_max_height, std::max(miral::default_min_height, source));
++}
++}
++
++miral::Width  const miral::default_min_width{0};
++miral::Height const miral::default_min_height{0};
++miral::Width  const miral::default_max_width{std::numeric_limits<int>::max()};
++miral::Height const miral::default_max_height{std::numeric_limits<int>::max()};
++miral::DeltaX const miral::default_width_inc{1};
++miral::DeltaY const miral::default_height_inc{1};
++miral::WindowInfo::AspectRatio const miral::default_min_aspect_ratio{
++    clamp(WindowInfo::AspectRatio{0U, std::numeric_limits<unsigned>::max()})};
++miral::WindowInfo::AspectRatio const miral::default_max_aspect_ratio{
++    clamp(WindowInfo::AspectRatio{std::numeric_limits<unsigned>::max(), 0U})};
  struct miral::WindowInfo::Self
+ {
@@ -71,16 +105,16 @@
      type{optional_value_or_default(params.type(), mir_window_type_normal)},
      state{optional_value_or_default(params.state(), mir_window_state_restored)},
      restore_rect{params.top_left().value(), params.size().value()},
--    min_width{optional_value_or_default(params.min_width())},
--    min_height{optional_value_or_default(params.min_height())},
--    max_width{optional_value_or_default(params.max_width(), Width{std::numeric_limits<int>::max()})},
--    max_height{optional_value_or_default(params.max_height(), Height{std::numeric_limits<int>::max()})},
++    min_width{optional_value_or_default(params.min_width(), miral::default_min_width)},
++    min_height{optional_value_or_default(params.min_height(), miral::default_min_height)},
++    max_width{optional_value_or_default(params.max_width(), miral::default_max_width)},
++    max_height{optional_value_or_default(params.max_height(), miral::default_max_height)},
      preferred_orientation{optional_value_or_default(params.preferred_orientation(), mir_orientation_mode_any)},
      confine_pointer(optional_value_or_default(params.confine_pointer(), mir_pointer_unconfined)),
--    width_inc{optional_value_or_default(params.width_inc(), DeltaX{1})},
--    height_inc{optional_value_or_default(params.height_inc(), DeltaY{1})},
--    min_aspect(optional_value_or_default(params.min_aspect(), AspectRatio{0U, std::numeric_limits<unsigned>::max()})),
--    max_aspect(optional_value_or_default(params.max_aspect(), AspectRatio{std::numeric_limits<unsigned>::max(), 0U})),
++    width_inc{optional_value_or_default(params.width_inc(), default_width_inc)},
++    height_inc{optional_value_or_default(params.height_inc(), default_height_inc)},
++    min_aspect(optional_value_or_default(params.min_aspect(), default_min_aspect_ratio)),
++    max_aspect(optional_value_or_default(params.max_aspect(), default_max_aspect_ratio)),
      shell_chrome(optional_value_or_default(params.shell_chrome(), mir_shell_chrome_normal))
+ {
      if (params.output_id().is_set())
@@ -450,7 +484,7 @@
  void miral::WindowInfo::min_width(mir::geometry::Width min_width)
+ {
--    self->min_width = min_width;
++    self->min_width = clamp(min_width);
+ }
  auto miral::WindowInfo::min_height() const -> mir::geometry::Height
@@ -460,7 +494,7 @@
  void miral::WindowInfo::min_height(mir::geometry::Height min_height)
+ {
--    self->min_height = min_height;
++    self->min_height = clamp(min_height);
+ }
  auto miral::WindowInfo::max_width() const -> mir::geometry::Width
@@ -470,7 +504,7 @@
  void miral::WindowInfo::max_width(mir::geometry::Width max_width)
+ {
--    self->max_width = max_width;
++    self->max_width = clamp(max_width);
+ }
  auto miral::WindowInfo::max_height() const -> mir::geometry::Height
@@ -480,7 +514,7 @@
  void miral::WindowInfo::max_height(mir::geometry::Height max_height)
+ {
--    self->max_height = max_height;
++    self->max_height = clamp(max_height);
+ }
  auto miral::WindowInfo::userdata() const -> std::shared_ptr<void>
@@ -520,7 +554,7 @@
  void miral::WindowInfo::min_aspect(AspectRatio min_aspect)
+ {
--    self->min_aspect = min_aspect;
++    self->min_aspect = clamp(min_aspect);
+ }
  auto miral::WindowInfo::max_aspect() const -> AspectRatio
@@ -530,7 +564,7 @@
  void miral::WindowInfo::max_aspect(AspectRatio max_aspect)
+ {
--    self->max_aspect = max_aspect;
++    self->max_aspect = clamp(max_aspect);
+ }
  bool miral::WindowInfo::has_output_id() const
 === added file 'miral/window_info_defaults.h'
 --- miral/window_info_defaults.h	1970-01-01 00:00:00 +0000
 +++ miral/window_info_defaults.h	2017-09-15 13:33:35 +0000
@@ -0,0 +1,36 @@
++/*
++ * 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: Alan Griffiths <alan@octopull.co.uk>
++ */
++
++#ifndef MIR_WINDOW_INFO_DEFAULTS_H
++#define MIR_WINDOW_INFO_DEFAULTS_H
++
++#include "miral/window_info.h"
++
++namespace miral
++{
++Width  extern const default_min_width;
++Height extern const default_min_height;
++Width  extern const default_max_width;
++Height extern const default_max_height;
++DeltaX extern const default_width_inc;
++DeltaY extern const default_height_inc;
++WindowInfo::AspectRatio extern const default_min_aspect_ratio;
++WindowInfo::AspectRatio extern const default_max_aspect_ratio;
++}
++
++#endif //MIR_WINDOW_INFO_DEFAULTS_H
 === modified file 'miral/window_management_trace.cpp'
 --- miral/window_management_trace.cpp	2017-08-10 16:59:22 +0000
 +++ miral/window_management_trace.cpp	2017-09-15 13:33:35 +0000
@@ -17,6 +17,7 @@
   */
  #include "window_management_trace.h"
++#include "window_info_defaults.h"
  #include <miral/application_info.h>
  #include <miral/window_info.h>
@@ -133,18 +134,19 @@
          APPEND(name);
          APPEND(type);
          APPEND(state);
++        bout.append("size", info.window().size());
          if (info.state() != mir_window_state_restored) APPEND(restore_rect);
          if (std::shared_ptr<mir::scene::Surface> parent = info.parent())
              bout.append("parent", parent->name());
          bout.append("children", dump_of(info.children()));
--        if (info.min_width()  != Width{0}) APPEND(min_width);
--        if (info.min_height() != Height{0}) APPEND(min_height);
--        if (info.max_width()  != Width{std::numeric_limits<int>::max()}) APPEND(max_width);
--        if (info.max_height() != Height{std::numeric_limits<int>::max()}) APPEND(max_height);
--        if (info.width_inc()  != DeltaX{1}) APPEND(width_inc);
--        if (info.height_inc() != DeltaY{1}) APPEND(height_inc);
--        if (info.min_aspect() != miral::WindowInfo::AspectRatio{0U, std::numeric_limits<unsigned>::max()}) APPEND(min_aspect);
--        if (info.max_aspect() != miral::WindowInfo::AspectRatio{std::numeric_limits<unsigned>::max(), 0U}) APPEND(max_aspect);
++        if (info.min_width()  != miral::default_min_width)  APPEND(min_width);
++        if (info.min_height() != miral::default_min_height) APPEND(min_height);
++        if (info.max_width()  != miral::default_max_width)  APPEND(max_width);
++        if (info.max_height() != miral::default_max_height) APPEND(max_height);
++        if (info.width_inc()  != miral::default_width_inc)  APPEND(width_inc);
++        if (info.height_inc() != miral::default_height_inc) APPEND(height_inc);
++        if (info.min_aspect() != miral::default_min_aspect_ratio) APPEND(min_aspect);
++        if (info.max_aspect() != miral::default_max_aspect_ratio) APPEND(max_aspect);
          APPEND(preferred_orientation);
          APPEND(confine_pointer);
 === modified file 'test/CMakeLists.txt'
 --- test/CMakeLists.txt	2017-04-11 11:17:34 +0000
 +++ test/CMakeLists.txt	2017-09-15 13:33:35 +0000
@@ -55,6 +55,7 @@
      window_id.cpp
      runner.cpp
      select_active_window.cpp
++    window_info.cpp
      window_placement.cpp
      window_placement_anchors_to_parent.cpp
      window_placement_client_api.cpp
 === added file 'test/window_info.cpp'
 --- test/window_info.cpp	1970-01-01 00:00:00 +0000
 +++ test/window_info.cpp	2017-09-15 13:33:35 +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 <brandon.schaefer@canonical.com>
++ */
++
++#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());
++}

MirAL

Merge lp:~alan-griffiths/miral/fix-1717061 into lp:miral

Commit message

Description of the change

Preview Diff

Subscribers