Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "applyDefaultIcon" setting #22

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add "applyDefaultIcon" setting #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca6e594
Add "applyDefaultIcon" setting
Feb 13, 2024
25f182f
Handle integer overflow caused by bogus client data
Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions openbox/client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

guint64 size = w * h;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/* 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
            }

break;
} else if (w == 0 || h == 0) {
i += size;
continue;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

data[i+j] =
(((data[i+j] >> 24) & 0xff) << RrDefaultAlphaOffset) +
(((data[i+j] >> 16) & 0xff) << RrDefaultRedOffset) +
Expand All @@ -2262,7 +2267,7 @@ void client_update_icons(ObClient *self)
else
RrImageAddFromData(img, &data[i], w, h);

i += w*h;
i += size;
}

g_free(data);
Expand Down Expand Up @@ -2306,7 +2311,7 @@ void client_update_icons(ObClient *self)
/* if the client has no icon at all, then we set a default icon onto it.
but, if it has parents, then one of them will have an icon already
*/
if (!self->icon_set && !self->parents) {
if (!self->icon_set && !self->parents && config_apply_default_icon) {
RrPixel32 *icon = ob_rr_theme->def_win_icon;
gulong *ldata; /* use a long here to satisfy OBT_PROP_SETA32 */

Expand Down
4 changes: 4 additions & 0 deletions openbox/config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ gboolean config_theme_keepborder;
guint config_theme_window_list_icon_size;

gchar *config_title_layout;
gboolean config_apply_default_icon;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


gboolean config_animate_iconify;

Expand Down Expand Up @@ -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")))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then this can move down below config_theme_window_list_icon_size as well.

config_apply_default_icon = obt_xml_node_bool(n);
if ((n = obt_xml_find_node(node, "windowListIconSize"))) {
config_theme_window_list_icon_size = obt_xml_node_int(n);
if (config_theme_window_list_icon_size < 16)
Expand Down Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this is already in the right place then.


config_font_activewindow = NULL;
config_font_inactivewindow = NULL;
Expand Down
2 changes: 2 additions & 0 deletions openbox/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/*! The font for the active window's title */
extern RrFont *config_font_activewindow;
Expand Down