Add "applyDefaultIcon" setting #22

ahigerd · 2024-02-13T14:56:56Z

Some applications don't provide their own icons. By default, Openbox sets a generic icon for these applications. Other environments, such as GNOME, instead supply an icon from /usr/share/pixmaps or the icon theme. Openbox's behavior is appropriate when it is running on its own, but when it is being used as the window manager in a broader desktop environment, the presence of the default icon prevents the environment from noticing that it needs to provide an icon.

This change adds an <applyDefaultIcon> configuration option that defaults to yes to preserve the standard Openbox behavior. By setting this option to no, Openbox will cede responsibility for setting default icons to the desktop environment.

Resolves issue #21

Some applications don't provide their own icons. By default, Openbox sets a generic icon for these applications. Other environments, such as GNOME, instead supply an icon from /usr/share/pixmaps or the icon theme. Openbox's behavior is appropriate when it is running on its own, but when it being used as the window manager in a broader desktop environment, the presence of the default icon prevents the environment from noticing that it needs to provide an icon. This change adds an `<applyDefaultIcon>` configuration option that defaults to `yes` to preserve the standard Openbox behavior. By setting this option to `no`, Openbox will cede responsibility for setting default icons to the desktop environment. Resolves issue Mikachu#21

ahigerd · 2024-02-13T17:20:55Z

Hold this thought: This was working for me for several hours, but I just had a crash and I'm investigating if it's related to this change or not.

ahigerd · 2024-02-13T20:04:53Z

~~Doesn't look related, though it's kind of confusing. It crashed in client_calc_layer.~~

~~Okay, yeah, this PR should be good to go.~~

Recompiled with debug symbols and got a more useful crash log. It's crashing in an image resize operation.

danakj · 2024-02-13T21:23:20Z

I was going to say there hasn’t been a crash report in many years :) Thanks for digging in more

ahigerd · 2024-02-14T00:19:34Z

Ah. Okay, so there's a bug in my panel where I'm assigning the icon, and that's triggering an overflow error in client_update_icons -- it's checking i + w*h > num but w*h overflows, so the > comparison doesn't notice it.

To be fair, this is a ridiculous edge case and a WM shouldn't be expected to have flawless behavior in the face of misbehaving clients! But I'll go ahead and fix it while I'm in here.

ahigerd · 2024-02-14T04:43:57Z

(For the record, the bug was that I was forgetting to prefix the image data with the icon dimensions.)

ahigerd · 2024-02-14T06:29:19Z

Okay, having fixed my panel and exercised this PR and my own fallback icon code for a few hours, it's been working great. Ready for review!

danakj

Thanks, I finally got around to reviewing and left some feedback. It looks good to me then.

danakj · 2024-04-06T19:40:08Z

openbox/client.c

@@ -2241,15 +2241,20 @@ void client_update_icons(ObClient *self)
        while (i + 2 < num) { /* +2 is to make sure there is a w and h */
            w = data[i++];
            h = data[i++];
+            /* calculate the data size as guint64 to prevent integer
+               overflow due to invalid data */
+            guint64 size = w * h;


This did not actually calculate the size as a u64; it converts after multiplying. To do this, we need to cast at least one of w or h to u64 first.

size = (guint64)w * h;

The declaration of guint64 size should be up at the top of the function - openbox has followed old C rules in this regard.

danakj · 2024-04-06T19:41:16Z

openbox/client.c

            /* watch for the data being too small for the specified size,
               or for zero sized icons. */
-            if (i + w*h > num || w == 0 || h == 0) {
-                i += w*h;
+            if (i + size > num || size < w || size < h) {


With the above corrected, I believe we need to look for overflow here as well of i+size

/* watch for addition overflow, for the data being too small for the specified size, or for zero sized icons. */ if (i + size < i || i + size > num) { break; } else if (size == 0) { continue; }

danakj · 2024-04-06T19:45:27Z

openbox/client.c

@@ -2241,15 +2241,20 @@ void client_update_icons(ObClient *self)
        while (i + 2 < num) { /* +2 is to make sure there is a w and h */
            w = data[i++];
            h = data[i++];
+            /* calculate the data size as guint64 to prevent integer
+               overflow due to invalid data */


I am not sure that I would call it invalid data. the num here is guint which can be 64-bit. While unreasonably large, a w*h that is larger than 32 bits isn't invalid that I can see.

In fact, https://tronche.com/gui/x/xlib/window-information/XGetWindowProperty.html the value is an unsigned long which openbox converts incorrectly to guint, woops. But that's not your problem to solve here :) All this is just to say I think it's not invalid. We just need to ensure we don't go past whatever num we've got here.

So I think we can just drop "due to invalid data" from the comment.

danakj · 2024-04-06T19:53:24Z

openbox/client.c

                continue;
            }

            /* convert it to the right bit order for ObRender */
-            for (j = 0; j < w*h; ++j)
+            for (j = 0; j < size; ++j)


This will never finish for large size, as j will wrap around before it reaches size and will then go off writing all over memory, so we need to change more types.

as i and j are indexes into the data buffer, which we're comparing against a 64-bit size, we should make i and j also be guint64.

We may be prevented from this by num being a 32-bit value right now but that seems like a mistake anyway, so lets get these types right here at least.

danakj · 2024-04-06T20:07:02Z

openbox/config.c

@@ -50,6 +50,7 @@ gboolean config_theme_keepborder;
 guint    config_theme_window_list_icon_size;

 gchar   *config_title_layout;
+gboolean config_apply_default_icon;


could we have a consistent ordering across files? this can go right below config_theme_window_list_icon_size here, so move up a couple lines.

I was looking for a consistent place to put it but the rest of the ordering is inconsistent so I wasn't sure exactly where to place it. I'll go ahead and follow your suggestions because that's probably better than where I did put it.

danakj · 2024-04-06T20:07:22Z

openbox/config.c

@@ -712,6 +713,8 @@ static void parse_theme(xmlNodePtr node, gpointer d)
        config_theme_keepborder = obt_xml_node_bool(n);
    if ((n = obt_xml_find_node(node, "animateIconify")))
        config_animate_iconify = obt_xml_node_bool(n);
+    if ((n = obt_xml_find_node(node, "applyDefaultIcon")))


then this can move down below config_theme_window_list_icon_size as well.

danakj · 2024-04-06T20:07:32Z

openbox/config.c

@@ -1098,6 +1101,7 @@ void config_startup(ObtXmlInst *i)
    config_title_layout = g_strdup("NLIMC");
    config_theme_keepborder = TRUE;
    config_theme_window_list_icon_size = 36;
+    config_apply_default_icon = TRUE;


and this is already in the right place then.

danakj · 2024-04-06T20:12:45Z

openbox/config.h

@@ -152,6 +152,8 @@ extern gchar *config_title_layout;
 extern gboolean config_animate_iconify;
 /*! Size of icons in focus switching dialogs */
 extern guint config_theme_window_list_icon_size;
+/*! Set a default icon for windows that lack one */
+extern gboolean config_apply_default_icon;


The new option will need to be added to a few more places:

The default rc.xml: https://github.com/Mikachu/openbox/blob/53737a844d2bba4bb5892737b05a01d6d06fe84b/data/rc.xml

The rc.xsd: https://github.com/Mikachu/openbox/blob/53737a844d2bba4bb5892737b05a01d6d06fe84b/data/rc.xsd (it's a type ob:bool value)

And the rc-mouse-focus.xml example config: https://github.com/Mikachu/openbox/blob/53737a844d2bba4bb5892737b05a01d6d06fe84b/doc/rc-mouse-focus.xml

ahigerd mentioned this pull request Feb 13, 2024

Fallback icon doesn't play nice with third-party panels #21

Open

ahigerd force-pushed the alh/applyDefaultIcon branch from 1e3a682 to 25f182f Compare February 14, 2024 00:40

Handle integer overflow caused by bogus client data

25f182f

danakj reviewed Apr 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "applyDefaultIcon" setting #22

Add "applyDefaultIcon" setting #22

ahigerd commented Feb 13, 2024 •

edited

Loading

ahigerd commented Feb 13, 2024

ahigerd commented Feb 13, 2024 •

edited

Loading

danakj commented Feb 13, 2024

ahigerd commented Feb 14, 2024 •

edited

Loading

ahigerd commented Feb 14, 2024

ahigerd commented Feb 14, 2024

danakj left a comment

danakj Apr 6, 2024

danakj Apr 6, 2024

danakj Apr 6, 2024

danakj Apr 6, 2024

danakj Apr 6, 2024

ahigerd Apr 17, 2024

danakj Apr 6, 2024

danakj Apr 6, 2024

danakj Apr 6, 2024

Add "applyDefaultIcon" setting #22

Are you sure you want to change the base?

Add "applyDefaultIcon" setting #22

Conversation

ahigerd commented Feb 13, 2024 • edited Loading

ahigerd commented Feb 13, 2024

ahigerd commented Feb 13, 2024 • edited Loading

danakj commented Feb 13, 2024

ahigerd commented Feb 14, 2024 • edited Loading

ahigerd commented Feb 14, 2024

ahigerd commented Feb 14, 2024

danakj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahigerd commented Feb 13, 2024 •

edited

Loading

ahigerd commented Feb 13, 2024 •

edited

Loading

ahigerd commented Feb 14, 2024 •

edited

Loading