Bug 1643474 - Cleanup and validate input in button layout placement code. r=stransky,smurfd
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 05 Jun 2020 10:02:58 +0000
changeset 534109 7747e712a73d933c95dffcd55ee7fb5aa4daaa71
parent 534108 c8f9006e335613115da5e0853cb126e7981e2e21
child 534110 227899eeb3a021bf81aa126760c20d20e36217b6
push id37482
push usernbeleuzu@mozilla.com
push dateFri, 05 Jun 2020 14:35:19 +0000
treeherdermozilla-central@c835da226e6d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstransky, smurfd
bugs1643474
milestone79.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1643474 - Cleanup and validate input in button layout placement code. r=stransky,smurfd It is not clear whether this is the cause of the crash, but it seems plausible as it is both recent code and it clearly doesn't validate the length of the stack-allocated buffer, so a bad gconf could plausibly cause this. Clean stuff up a bit and validate the buffer length. Differential Revision: https://phabricator.services.mozilla.com/D78405
widget/gtk/WidgetStyleCache.cpp
widget/gtk/gtk3drawing.cpp
widget/gtk/gtkdrawing.h
widget/gtk/nsLookAndFeel.cpp
--- a/widget/gtk/WidgetStyleCache.cpp
+++ b/widget/gtk/WidgetStyleCache.cpp
@@ -602,19 +602,20 @@ static void CreateHeaderBarButton(GtkWid
   // We bypass GetWidget() here by explicit sWidgetStorage[] update so
   // invalidate the style as well as GetWidget() does.
   style = gtk_widget_get_style_context(image);
   gtk_style_context_invalidate(style);
 
   LoadWidgetIconPixbuf(image);
 }
 
-static bool IsToolbarButtonEnabled(ButtonLayout* aButtonLayout, int aButtonNums,
+static bool IsToolbarButtonEnabled(ButtonLayout* aButtonLayout,
+                                   size_t aButtonNums,
                                    WidgetNodeType aAppearance) {
-  for (int i = 0; i < aButtonNums; i++) {
+  for (size_t i = 0; i < aButtonNums; i++) {
     if (aButtonLayout[i].mType == aAppearance) {
       return true;
     }
   }
   return false;
 }
 
 static void CreateHeaderBarButtons() {
@@ -630,18 +631,18 @@ static void CreateHeaderBarButtons() {
   GtkWidget* buttonBox = gtk_box_new(GTK_ORIENTATION_HORIZONTAL, buttonSpacing);
   gtk_container_add(GTK_CONTAINER(GetWidget(MOZ_GTK_HEADER_BAR)), buttonBox);
   // We support only LTR headerbar layout for now.
   gtk_style_context_add_class(gtk_widget_get_style_context(buttonBox),
                               GTK_STYLE_CLASS_LEFT);
 
   ButtonLayout buttonLayout[TOOLBAR_BUTTONS];
 
-  int activeButtons =
-      GetGtkHeaderBarButtonLayout(buttonLayout, TOOLBAR_BUTTONS, nullptr);
+  size_t activeButtons =
+      GetGtkHeaderBarButtonLayout(MakeSpan(buttonLayout), nullptr);
 
   if (IsToolbarButtonEnabled(buttonLayout, activeButtons,
                              MOZ_GTK_HEADER_BAR_BUTTON_MINIMIZE)) {
     CreateHeaderBarButton(buttonBox, MOZ_GTK_HEADER_BAR_BUTTON_MINIMIZE);
   }
   if (IsToolbarButtonEnabled(buttonLayout, activeButtons,
                              MOZ_GTK_HEADER_BAR_BUTTON_MAXIMIZE)) {
     CreateHeaderBarButton(buttonBox, MOZ_GTK_HEADER_BAR_BUTTON_MAXIMIZE);
--- a/widget/gtk/gtk3drawing.cpp
+++ b/widget/gtk/gtk3drawing.cpp
@@ -8,18 +8,20 @@
  * Adapted from the gtkdrawing.c, and gtk+2.0 source.
  */
 
 #include <gtk/gtk.h>
 #include <gdk/gdkprivate.h>
 #include <string.h>
 #include "gtkdrawing.h"
 #include "mozilla/Assertions.h"
+#include "mozilla/ScopeExit.h"
 #include "prinrval.h"
 #include "WidgetStyleCache.h"
+#include "nsString.h"
 #include "nsDebug.h"
 
 #include <math.h>
 #include <dlfcn.h>
 
 static gboolean checkbox_check_state;
 static gboolean notebook_has_tab_gap;
 
@@ -28,16 +30,18 @@ static ScrollbarGTKMetrics sActiveScroll
 static ToggleGTKMetrics sCheckboxMetrics;
 static ToggleGTKMetrics sRadioMetrics;
 static ToggleGTKMetrics sMenuRadioMetrics;
 static ToggleGTKMetrics sMenuCheckboxMetrics;
 static ToolbarGTKMetrics sToolbarMetrics;
 static CSDWindowDecorationSize sToplevelWindowDecorationSize;
 static CSDWindowDecorationSize sPopupWindowDecorationSize;
 
+using mozilla::Span;
+
 #define ARROW_UP 0
 #define ARROW_DOWN G_PI
 #define ARROW_RIGHT G_PI_2
 #define ARROW_LEFT (G_PI + G_PI_2)
 
 #if !GTK_CHECK_VERSION(3, 14, 0)
 #  define GTK_STATE_FLAG_CHECKED (1 << 11)
 #endif
@@ -385,108 +389,94 @@ static void CalculateToolbarButtonSpacin
   }
 
   aMetrics->minSizeWithBorderMargin.width +=
       aMetrics->buttonMargin.right + aMetrics->buttonMargin.left;
   aMetrics->minSizeWithBorderMargin.height +=
       aMetrics->buttonMargin.top + aMetrics->buttonMargin.bottom;
 }
 
-int GetGtkHeaderBarButtonLayout(ButtonLayout* aButtonLayout, int aMaxButtonNums,
-                                bool* aReversedButtonsPlacement) {
-#if DEBUG
-  if (aButtonLayout) {
-    NS_ASSERTION(
-        aMaxButtonNums >= TOOLBAR_BUTTONS,
-        "Requested number of buttons is higher than storage capacity!");
-  }
-#endif
-
+size_t GetGtkHeaderBarButtonLayout(Span<ButtonLayout> aButtonLayout,
+                                   bool* aReversedButtonsPlacement) {
   gchar* decorationLayoutSetting = nullptr;
   GtkSettings* settings = gtk_settings_get_for_screen(gdk_screen_get_default());
   g_object_get(settings, "gtk-decoration-layout", &decorationLayoutSetting,
                nullptr);
+  auto free = mozilla::MakeScopeExit([&] { g_free(decorationLayoutSetting); });
 
   // Use a default layout
   const gchar* decorationLayout = "menu:minimize,maximize,close";
   if (decorationLayoutSetting) {
     decorationLayout = decorationLayoutSetting;
   }
 
   // "minimize,maximize,close:" layout means buttons are on the opposite
   // titlebar side. close button is always there.
-  bool reversedButtonsPlacement = false;
-  const char* closeButton = strstr(decorationLayout, "close");
-  const char* separator = strchr(decorationLayout, ':');
-  if (closeButton != nullptr && separator != nullptr) {
-    reversedButtonsPlacement = closeButton < separator;
+  if (aReversedButtonsPlacement) {
+    const char* closeButton = strstr(decorationLayout, "close");
+    const char* separator = strchr(decorationLayout, ':');
+    *aReversedButtonsPlacement =
+        closeButton && separator && closeButton < separator;
   }
 
   // We check what position a button string is stored in decorationLayout.
   //
   // decorationLayout gets its value from the GNOME preference:
   // org.gnome.desktop.vm.preferences.button-layout via the
   // gtk-decoration-layout property.
   //
   // Documentation of the gtk-decoration-layout property can be found here:
   // https://developer.gnome.org/gtk3/stable/GtkSettings.html#GtkSettings--gtk-decoration-layout
-  //
-  int activeButtonNums = 0;
+  if (aButtonLayout.IsEmpty()) {
+    return 0;
+  }
+
+  nsDependentCSubstring layout(decorationLayout, strlen(decorationLayout));
+
   bool right = false;
-  if (aButtonLayout) {
-    nsDependentCSubstring layout(decorationLayout, strlen(decorationLayout));
-    // aButtonLayout used to be like this if all three buttons were represented:
-    // [MOZ_GTK_HEADER_BAR_BUTTON_CLOSE, MOZ_GTK_HEADER_BAR_BUTTON_MINIMIZE,
-    // MOZ_GTK_HEADER_BAR_BUTTON_MAXIMIZE].
-    // Now it is a struct with both a WidgetNodeType and a bool, the later
-    // represents if a button is on the right side of the tab bar.
-    //
-    for (const auto& part : layout.Split(':')) {
-      for (const auto& button : part.Split(',')) {
-        if (button.EqualsLiteral("close")) {
-          aButtonLayout[activeButtonNums++] = {MOZ_GTK_HEADER_BAR_BUTTON_CLOSE,
-                                               right};
-        } else if (button.EqualsLiteral("minimize")) {
-          aButtonLayout[activeButtonNums++] = {
-              MOZ_GTK_HEADER_BAR_BUTTON_MINIMIZE, right};
-        } else if (button.EqualsLiteral("maximize")) {
-          aButtonLayout[activeButtonNums++] = {
-              MOZ_GTK_HEADER_BAR_BUTTON_MAXIMIZE, right};
-        }
+  size_t activeButtons = 0;
+  for (const auto& part : layout.Split(':')) {
+    for (const auto& button : part.Split(',')) {
+      if (button.EqualsLiteral("close")) {
+        aButtonLayout[activeButtons++] = {MOZ_GTK_HEADER_BAR_BUTTON_CLOSE,
+                                          right};
+      } else if (button.EqualsLiteral("minimize")) {
+        aButtonLayout[activeButtons++] = {MOZ_GTK_HEADER_BAR_BUTTON_MINIMIZE,
+                                          right};
+      } else if (button.EqualsLiteral("maximize")) {
+        aButtonLayout[activeButtons++] = {MOZ_GTK_HEADER_BAR_BUTTON_MAXIMIZE,
+                                          right};
       }
-      right = true;
+      if (activeButtons == aButtonLayout.Length()) {
+        return activeButtons;
+      }
     }
+    right = true;
   }
-
-  if (aReversedButtonsPlacement) {
-    *aReversedButtonsPlacement = reversedButtonsPlacement;
-  }
-  g_free(decorationLayoutSetting);
-
-  return activeButtonNums;
+  return activeButtons;
 }
 
 static void EnsureToolbarMetrics(void) {
   if (!sToolbarMetrics.initialized) {
     // Make sure we have clean cache after theme reset, etc.
     memset(&sToolbarMetrics, 0, sizeof(sToolbarMetrics));
 
     // We're running on old Gtk+ version. Leave the cache empty
     // which means all buttons are disabled.
     if (gtk_check_version(3, 10, 0) != nullptr) {
       sToolbarMetrics.initialized = true;
       return;
     }
 
     // Calculate titlebar button visibility and positions.
     ButtonLayout aButtonLayout[TOOLBAR_BUTTONS];
-    int activeButtonNums =
-        GetGtkHeaderBarButtonLayout(aButtonLayout, TOOLBAR_BUTTONS, nullptr);
-
-    for (int i = 0; i < activeButtonNums; i++) {
+    size_t activeButtonNums =
+        GetGtkHeaderBarButtonLayout(mozilla::MakeSpan(aButtonLayout), nullptr);
+
+    for (size_t i = 0; i < activeButtonNums; i++) {
       int buttonIndex =
           (aButtonLayout[i].mType - MOZ_GTK_HEADER_BAR_BUTTON_CLOSE);
       ToolbarButtonGTKMetrics* metrics = sToolbarMetrics.button + buttonIndex;
       metrics->visible = true;
       // Mark first button
       if (!i) {
         metrics->firstButton = true;
       }
@@ -2439,18 +2429,18 @@ gint moz_gtk_get_widget_border(WidgetNod
       moz_gtk_add_margin_border_padding(labelStyle, left, top, right, bottom);
 
       return MOZ_GTK_SUCCESS;
     }
     case MOZ_GTK_HEADER_BAR_BUTTON_BOX: {
       style = GetStyleContext(MOZ_GTK_HEADER_BAR);
       moz_gtk_add_border_padding(style, left, top, right, bottom);
       *top = *bottom = 0;
-      bool leftButtonsPlacement;
-      GetGtkHeaderBarButtonLayout(nullptr, 0, &leftButtonsPlacement);
+      bool leftButtonsPlacement = false;
+      GetGtkHeaderBarButtonLayout({}, &leftButtonsPlacement);
       if (direction == GTK_TEXT_DIR_RTL) {
         leftButtonsPlacement = !leftButtonsPlacement;
       }
       if (leftButtonsPlacement) {
         *right = 0;
       } else {
         *left = 0;
       }
--- a/widget/gtk/gtkdrawing.h
+++ b/widget/gtk/gtkdrawing.h
@@ -13,16 +13,17 @@
  */
 
 #ifndef _GTK_DRAWING_H_
 #define _GTK_DRAWING_H_
 
 #include <gdk/gdk.h>
 #include <gtk/gtk.h>
 #include <algorithm>
+#include "mozilla/Span.h"
 
 /*** type definitions ***/
 typedef struct {
   guint8 active;
   guint8 focused;
   guint8 selected;
   guint8 inHover;
   guint8 disabled;
@@ -603,28 +604,26 @@ gint moz_gtk_get_tab_thickness(WidgetNod
 /**
  * Get ToolbarButtonGTKMetrics for recent theme.
  */
 const ToolbarButtonGTKMetrics* GetToolbarButtonMetrics(
     WidgetNodeType aAppearance);
 
 /**
  * Get toolbar button layout.
- * aButtonLayout:  [IN][OUT] An array which will be filled by WidgetNodeType
- *                           references to visible titlebar buttons.
- *                           Must contains at least TOOLBAR_BUTTONS entries.
- * aMaxButtonNums: [IN] Allocated aButtonLayout entries. Must be at least
- *                      TOOLBAR_BUTTONS wide.
+ * aButtonLayout:  [OUT] An array which will be filled by ButtonLayout
+ *                       references to visible titlebar buttons. Must contain at
+ *                       least TOOLBAR_BUTTONS entries if non-empty.
  * aReversedButtonsPlacement: [OUT] True if the buttons are placed in opposite
  *                                  titlebar corner.
  *
  * returns:    Number of returned entries at aButtonLayout.
  */
-int GetGtkHeaderBarButtonLayout(ButtonLayout* aButtonLayout, int aMaxButtonNums,
-                                bool* aReversedButtonsPlacement);
+size_t GetGtkHeaderBarButtonLayout(mozilla::Span<ButtonLayout>,
+                                   bool* aReversedButtonsPlacement);
 
 /**
  * Get size of CSD window extents.
  *
  * aIsPopup: [IN] Get decoration size for popup or toplevel window.
  *
  * returns: Calculated (or estimated) decoration size of given aGtkWindow.
  */
--- a/widget/gtk/nsLookAndFeel.cpp
+++ b/widget/gtk/nsLookAndFeel.cpp
@@ -1280,50 +1280,47 @@ void nsLookAndFeel::EnsureInit() {
   mCSDCloseButtonPosition = 0;
   mCSDMinimizeButtonPosition = 0;
   mCSDMaximizeButtonPosition = 0;
 
   // We need to initialize whole CSD config explicitly because it's queried
   // as -moz-gtk* media features.
   ButtonLayout buttonLayout[TOOLBAR_BUTTONS];
 
-  int activeButtons = GetGtkHeaderBarButtonLayout(buttonLayout, TOOLBAR_BUTTONS,
-                                                  &mCSDReversedPlacement);
-  for (int i = 0; i < activeButtons; i++) {
+  size_t activeButtons = GetGtkHeaderBarButtonLayout(MakeSpan(buttonLayout),
+                                                     &mCSDReversedPlacement);
+  for (size_t i = 0; i < activeButtons; i++) {
     // We check if a button is represented on the right side of the tabbar.
     // Then we assign it a value from 3 to 5, instead of 0 to 2 when it is on
     // the left side.
-    switch (buttonLayout[i].mType) {
+    const ButtonLayout& layout = buttonLayout[i];
+    int32_t* pos = nullptr;
+    switch (layout.mType) {
       case MOZ_GTK_HEADER_BAR_BUTTON_MINIMIZE:
         mCSDMinimizeButton = true;
-        if (buttonLayout[i].mAtRight) {
-          mCSDMinimizeButtonPosition = i + TOOLBAR_BUTTONS;
-        } else {
-          mCSDMinimizeButtonPosition = i;
-        }
+        pos = &mCSDMinimizeButtonPosition;
         break;
       case MOZ_GTK_HEADER_BAR_BUTTON_MAXIMIZE:
         mCSDMaximizeButton = true;
-        if (buttonLayout[i].mAtRight) {
-          mCSDMaximizeButtonPosition = i + TOOLBAR_BUTTONS;
-        } else {
-          mCSDMaximizeButtonPosition = i;
-        }
+        pos = &mCSDMaximizeButtonPosition;
         break;
       case MOZ_GTK_HEADER_BAR_BUTTON_CLOSE:
         mCSDCloseButton = true;
-        if (buttonLayout[i].mAtRight) {
-          mCSDCloseButtonPosition = i + TOOLBAR_BUTTONS;
-        } else {
-          mCSDCloseButtonPosition = i;
-        }
+        pos = &mCSDCloseButtonPosition;
         break;
       default:
         break;
     }
+
+    if (pos) {
+      *pos = i;
+      if (layout.mAtRight) {
+        *pos += TOOLBAR_BUTTONS;
+      }
+    }
   }
 
   RecordTelemetry();
 }
 
 // virtual
 char16_t nsLookAndFeel::GetPasswordCharacterImpl() {
   EnsureInit();