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.

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

colorful window icons not always passed to dom0, padlock showed instead #1495

Closed
marmarek opened this issue Dec 7, 2015 · 25 comments
Closed
Labels
C: gui-virtualization eol-4.0 Closed because Qubes 4.0 has reached end-of-life (EOL) P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. r4.0-dom0-stable ux User experience

Comments

@marmarek
Copy link
Member

marmarek commented Dec 7, 2015

It looks like sometimes colorful icon isn't set for a window. Most likely it is transferred before window in dom0 gets created, and simply discarded.
Dom0 part might monitor for new window events and have some (small) queue for such icons.

@marmarek marmarek added bug C: gui-virtualization P: minor Priority: minor. The lowest priority, below "default." labels Dec 7, 2015
@marmarek marmarek added this to the Release 3.1 milestone Dec 7, 2015
@marmarek
Copy link
Member Author

  1. When the icon is received but no matching window exists, it is
    discarded. That part of code is here:
    https://github.com/QubesOS/qubes-gui-daemon/blob/master/window-icon-updater/icon-receiver#L319-L324
  2. Instead of discarding it should be cached for a short time, and then
    when new window appears icon from cache should be applied (if there is
    one). This part require watching for "CreateNotifyEvent", which isn't
    currently done. Instead X11 events are handled only when searching for
    some window:
    https://github.com/QubesOS/qubes-gui-daemon/blob/master/window-icon-updater/icon-receiver#L235
    Watching for such events is implemented on the sender part here:
    https://github.com/QubesOS/qubes-gui-agent-linux/blob/master/window-icon-updater/icon-sender#L134-L143
  3. The icon cache should be done with security in mind, for example to
    not allow any VM use all dom0 memory. I think limiting its size to for
    example last 10 icons should be enough.

@v6ak
Copy link

v6ak commented Feb 18, 2016

Why not implement the logic in DomUs? For example, icon would be sent on window creation.

Pros:

  • No need for such DoS prevention mechanisms.
  • It should work in any load (compared to the “cached for a short time” best-effort mechanism)
  • A simpler code, I guess. At least in dom0.

Cons:

  • Maybe need to implement the logic in multiple agents. (But I am not sure if the issue also happens with QWT, maybe it is not needed there.)

@marmarek
Copy link
Member Author

On Thu, Feb 18, 2016 at 06:40:54AM -0800, Vít Šesták wrote:

Why not implement the logic in DomUs? For example, icon would be sent on window creation.

It is exactly what is currently done. In practice it looks like this:

  1. Application create a window
  2. Application set an icon
    3a. gui-agent notice new window and send it to dom0
    3b. icon-sender notice new window and send its icon to dom0
    4a. gui-daemon receive new window info and create it on dom0 X server
    4b. icon-receiver get new icon for a window and set the icon (if window
    exists)

The problem is that, we don't control order of 4a and 4b. To make things
even worse, it could be also another situation:
5. Application destroys the window
6. gui-agent send that event to dom0
7. gui-daemon destroys the window
and now 4b is happening.

VM have no direct confirmation when window is actually created in dom0.
So no simple point of synchronization...

Another idea (instead of caching) - add some feedback dom0->VM like "no
such window" info, so VM could retry some time later?

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

@najamelan
Copy link

najamelan commented Dec 28, 2017

I know this is somewhat old, but I feel this feature is not stabilized. I still see lock icons and so do others in Q4.

In terms of implementation described by @marmarek, something seems off. I would say that here the vm is the model and the gui agent in dom0 is the view. For that it should not be up to the model to send data to the view. It should be the view that requests it.

Example:
Model sends an event to dom0 (new window created, handle:xxx)
dom0 requests extra data for handle xxx: vm, please give me window title, position, icon...

This way the control over data that flows to dom0 is in the hands of dom0 and not the vm. The vm cannot send more icons than requested. This is better from a security perspective. All you have to do is sanitize and then validate the data you get back. Is the icon a valid image, not exceeding a certain size? Is the window title a valid utf-8 string, with min and max lengths, without forbidden characters, etc.

This also solves synchronization issues. dom0 will always be ready when it receives any data, because it has requested it.

It also means you don't need several different services. One gui manager in dom0 and one in each vm, no need for something separate for icons, or anything else that we should send, like mouse cursor? This should simplify the architecture.

I have not had a look at the implementation in Qubes, but obviously there is one area in which we have the vm initiate comms: Events. It is just important to sanitize and validate all data send by events, and formalize the format. It can then be used by stuff other than gui related as well. It should be a fixed transport protocol. It can help users to make safe extensions as well, when using something strictly typed.

@najamelan
Copy link

Not sure whether this helps, but in fedora-26 template, I get this in ~/.xsession-errors:

Traceback (most recent call last):
  File "/usr/lib/qubes/icon-sender", line 155, in <module>
    retriever.watch_and_send_icons()
  File "/usr/lib/qubes/icon-sender", line 136, in watch_and_send_icons
    [xproto.EventMask.SubstructureNotify])
  File "/usr/lib64/python2.7/site-packages/xcb/xproto.py", line 1400, in ChangeWindowAttributesChecked
    for elt in xcb.Iterator(value_list, 15, 'value_list', False):
xcb.Exception: Too few items in 'value_list' list (expect 15).

@marmarek
Copy link
Member Author

This helps a lot, thanks!

marmarek added a commit to marmarek/old-qubes-gui-agent-linux that referenced this issue Dec 31, 2017
xpyb in Fedora 26 (and probably others) have problems with parsing X
events, like this:

    Traceback (most recent call last):
      File "/usr/lib/qubes/icon-sender", line 155, in <module>
        retriever.watch_and_send_icons()
      File "/usr/lib/qubes/icon-sender", line 136, in watch_and_send_icons
        [xproto.EventMask.SubstructureNotify])
      File "/usr/lib64/python2.7/site-packages/xcb/xproto.py", line 1400, in ChangeWindowAttributesChecked
        for elt in xcb.Iterator(value_list, 15, 'value_list', False):
    xcb.Exception: Too few items in 'value_list' list (expect 15).

QubesOS/qubes-issues#1495
marmarek added a commit to QubesOS/qubes-gui-agent-linux that referenced this issue Feb 12, 2018
xpyb in Fedora 26 (and probably others) have problems with parsing X
events, like this:

    Traceback (most recent call last):
      File "/usr/lib/qubes/icon-sender", line 155, in <module>
        retriever.watch_and_send_icons()
      File "/usr/lib/qubes/icon-sender", line 136, in watch_and_send_icons
        [xproto.EventMask.SubstructureNotify])
      File "/usr/lib64/python2.7/site-packages/xcb/xproto.py", line 1400, in ChangeWindowAttributesChecked
        for elt in xcb.Iterator(value_list, 15, 'value_list', False):
    xcb.Exception: Too few items in 'value_list' list (expect 15).

QubesOS/qubes-issues#1495

(cherry picked from commit dd959a0)
@qubesos-bot
Copy link

Automated announcement from builder-github

The package qubes-gui-dom0-4.0.7-1.fc25 has been pushed to the r4.0 testing repository for dom0.
To test this update, please install it with the following command:

sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing

Changes included in this update

@t4777sd
Copy link

t4777sd commented Aug 1, 2018

In a VM that has been open for a while icons no longer get shown and instead only a padlock does. Looking at xsession_errors it looks like the following may be relevant:

Traceback (most recent call last):
  File "/usr/lib/qubes/icon-sender", line 155, in <module>
    retriever.watch_and_send_icons()
  File "/usr/lib/qubes/icon-sender", line 146, in watch_and_send_icons
    self.send_icon(ev.window)
  File "/usr/lib/qubes/icon-sender", line 121, in send_icon
    icons[chosen_size]]))
IOError: [Errno 32] Broken pipe

@t4777sd
Copy link

t4777sd commented Aug 1, 2018

Based on my testing of the icon displaying for the task bar these are the observations I have made:

  1. Nautilus icon is never sent. You can test this on a default fedora

  2. I have a desktop file with the following icon defined:
    Icon=/usr/share/icons/app.png
    This is the only icon available to this application. It works in the application menu. However, it does not work in the taskbar and displays a padlock.

  3. The application icon for http://searchmonkey.embeddediq.com/ (installed on Fedora via the RPM downloaded) does not display in the task bar, but it does in the application window. It's icon is defined as:
    Icon=searchmonkey

It creates the following icon paths:
/usr/share/icon/hicolor/48x48/apps/searchmonkey.png
/usr/share/icon/hicolor/16x16/apps/searchmonkey.png
/usr/share/icon/hicolor/22x22/apps/searchmonkey.png
/usr/share/icon/hicolor/32x32/apps/searchmonkey.png
/usr/share/icon/hicolor/24x24/apps/searchmonkey.png

@marmarek
Copy link
Member Author

There are two totally separate mechanisms for icons in the menu and for running applications (taskbar, application switcher etc). The former is set in .desktop file, the latter - by running application. Application is free to set different icon, and additionally in many cases there is no way to tell which .desktop file was used to start given application.

1. Nautilus icon is never sent. You can test this on a default fedora

Looks like Nautilus does not set _NET_WM_ICON window property. Not sure why. On the other hand, there are some GTK specific properties (retrieved with xprop tool):

_GTK_THEME_VARIANT(UTF8_STRING) = 
_GTK_APP_MENU_OBJECT_PATH(UTF8_STRING) = "/org/gnome/Nautilus/menus/appmenu"
_GTK_WINDOW_OBJECT_PATH(UTF8_STRING) = "/org/gnome/Nautilus/window/2"
_GTK_APPLICATION_OBJECT_PATH(UTF8_STRING) = "/org/gnome/Nautilus"
_GTK_UNIQUE_BUS_NAME(UTF8_STRING) = ":1.45"
_GTK_APPLICATION_ID(UTF8_STRING) = "org.gnome.Nautilus"

Maybe some of them can be used to extract the icon? Maybe _GTK_APPLICATION_ID to find desktop file and retrieve icon from there? I need to look at GTK docs how window icon is supposed to work.

@SaswatPadhi
Copy link

I noticed this issue for several other apps, including Spotify, Google Chrome, Microsoft Visual Studio Code ...

If there is any logs / debug info I can provide you with, let me know.

@ThomasKjaergaard
Copy link

ThomasKjaergaard commented Jan 31, 2020

The issue is also with gedit.
That is a padlock in the Qubes color is shown in the task bar instead of the expected icon.

It seems like marmarek are right about it's two separate mechanisms.

Quote:

There are two totally separate mechanisms for icons in the menu and for running applications (taskbar, application switcher etc). The former is set in .desktop file, the latter - by running application. Application is free to set different icon, and additionally in many cases there is no way to tell which .desktop file was used to start given application.

As trying to override it with:
Defining the icon by copying files to
/usr/share/icon/hicolor/16x16/apps/
/usr/share/icon/hicolor/32x32/apps/
/usr/share/icon/hicolor/48x48/apps/
etc up to 256 on the template vm.

Set the icon name in the .desktop file and then doing qvm-sync-appmenus fedora-30.

Does not get it to work.
The right icon is not shown in the task bar for other apps like Apache Netbeans.
Correct icons are shown for examples like: Firefox, Nautilys, Gnome Terminal, Thunderbird.

I'm using Quebes 4.0 with all updates.

@jamke
Copy link

jamke commented Dec 19, 2020

Please take a look at #6292.
It shows a simple way to 100% reproduce icon lock problem.

It's not actually a duplicate of this 2015 issue that is based on some bugs in logic or something but issue #6292 is almost certainly about icon sizes causing lock icon in 100% cases.

@DemiMarie
Copy link

DemiMarie commented Dec 19, 2020

@najamelan’s comment is right on the money, IMO.

@reprc
Copy link

reprc commented Mar 23, 2021

I have this issue when icon-sender stops running for some reason. ~/.xsession-errors contains this:

  File "/usr/lib/qubes/icon-sender", line 152, in <module>
    retriever.watch_and_send_icons()
  File "/usr/lib/qubes/icon-sender", line 143, in watch_and_send_icons
    self.send_icon(ev.window)
  File "/usr/lib/qubes/icon-sender", line 116, in send_icon
    sys.stdout.buffer.write(b''.join(
BrokenPipeError: [Errno 32] Broken pipe

Running /usr/bin/qrexec-client-vm dom0 qubes.WindowIconUpdater /usr/lib/qubes/icon-sender manually brings icons back

@marmarek
Copy link
Member Author

The above seems like a direct result of icon-receiver being terminated. So, the actual problem is somewhere on the icon-receiver side. Have you restarted your X11 session (in case of i3, I believe also reloading configuration may cause this)? Check in dom0 ~/.xsession-errors and maybe journalctl if you see any icon-receiver related error.

@marmarek
Copy link
Member Author

In terms of implementation described by @marmarek, something seems off. I would say that here the vm is the model and the gui agent in dom0 is the view. For that it should not be up to the model to send data to the view. It should be the view that requests it.

This may indeed look this way, but I think the above analogy doesn't work well here. Most of the gui daemon actions are triggered either by the user (click, key press etc), or by some in-vm application action (window content change, window title change etc). It is all event-driven. If you'd separate events from the actual data, then the protocol flow would complicate quite a bit, because for every event gui-daemon would need to:

  • make a request to gui-agent about whatever more data it needs
  • wait for the response (asynchronously, to not miss other events coming in the meantime)

If there are multiple events in the pipeline, then it would even need to keep track which are handled and which are still waiting for this subsequent data. Correlate which response is to which request (which event, which other responses if more than one requested etc). This is quite complex state machine and gives a great chance to introduce some logic bugs (leaving alone pure programming bugs that are also possible here, because of the C language).

Instead, we designed the GUI protocol the way, that events already carry all the needed auxiliary data. In your model you can simply treat all gui-agent messages as kind of rich events, that avoids that ping-pong to get the actual data.

Now to the icon receiver. The source of those synchronization issues is the icon handling being outside of gui-agent<->gui-daemon loop. And the sole reason for that is the programming language: I really don't want to do untrusted graphics processing in application written in C (gui-daemon). That's why it is a separate service written in Python.
Restructuring the protocol to be more "model-view" here and dom0 part request an icon wouldn't really solve this issue, because we still have two corner cases:

  • when window is created at dom0 side (by gui-daemon), the application may not set the icon yet (so, the request for icon would fail)
  • when VM sends an event that new icon is sent, the window at dom0 side may not exist yet (this is basically the case described initially here) - it doesn't really matter if this is "just" an event, or event with icon already attached

Both of the above cases would be solved by incorporating events processing into gui-agent<->gui-daemon, but I don't want to do that for the reasons explained above. It's better to sometime have a missing icon, than to risk complete system compromise.

@reprc
Copy link

reprc commented Mar 23, 2021

Have you restarted your X11 session (in case of i3, I believe also reloading configuration may cause this)?

No, I haven't

Check in dom0 ~/.xsession-errors and maybe journalctl if you see any icon-receiver related error.

~/.xsession-errors has no errors, journalctl has 2 different tracebacks, either one for each qube with this issue

Mar 19 18:15:52 dom0 qubes.WindowIconUpdater-sys-usb[13119]:

 Traceback (most recent call last):
   File "/usr/lib/python3.5/site-packages/xcffib/__init__.py", line 557, in wrapper
     return f(*args)
   File "/usr/lib/python3.5/site-packages/xcffib/__init__.py", line 596, in poll_for_event
     self.invalid()
   File "/usr/lib/python3.5/site-packages/xcffib/__init__.py", line 545, in invalid
     raise ConnectionException(err)
 xcffib.ConnectionException: xcb connection errors because of socket, pipe and other stream errors.
 
 During handling of the above exception, another exception occurred:
 
 Traceback (most recent call last):
   File "/usr/lib/qubes/icon-receiver", line 362, in <module>
     rcvd.handle_input()
   File "/usr/lib/qubes/icon-receiver", line 347, in handle_input
     self.handle_events()
   File "/usr/lib/qubes/icon-receiver", line 259, in handle_events
     for ev in iter(self.conn.poll_for_event, None):
   File "/usr/lib/python3.5/site-packages/xcffib/__init__.py", line 559, in wrapper
     self.invalid()
   File "/usr/lib/python3.5/site-packages/xcffib/__init__.py", line 545, in invalid
     raise ConnectionException(err)
 xcffib.ConnectionException: xcb connection errors because of socket, pipe and other stream errors.
Mar 21 15:58:48 dom0 qubes.WindowIconUpdater-personal[16498]:

Traceback (most recent call last):
   File "/usr/lib/qubes/icon-receiver", line 362, in <module>
     rcvd.handle_input()
   File "/usr/lib/qubes/icon-receiver", line 347, in handle_input
     self.handle_events()
   File "/usr/lib/qubes/icon-receiver", line 259, in handle_events
     for ev in iter(self.conn.poll_for_event, None):
   File "/usr/lib/python3.5/site-packages/xcffib/__init__.py", line 557, in wrapper
     return f(*args)
   File "/usr/lib/python3.5/site-packages/xcffib/__init__.py", line 598, in poll_for_event
     return self.hoist_event(e)
   File "/usr/lib/python3.5/site-packages/xcffib/__init__.py", line 668, in hoist_event
     return self._process_error(ffi.cast("xcb_generic_error_t *", e))
   File "/usr/lib/python3.5/site-packages/xcffib/__init__.py", line 634, in _process_error
     raise error(buf)
 xcffib.xproto.WindowError

@DemiMarie
Copy link

Now to the icon receiver. The source of those synchronization issues is the icon handling being outside of gui-agent<->gui-daemon loop. And the sole reason for that is the programming language: I really don't want to do untrusted graphics processing in application written in C (gui-daemon). That's why it is a separate service written in Python.

This appears to be a fantastic use-case for Rust. The C code would need to add support for a single new type of event, but all of the heavy lifting would be done in Rust. In the event that said heavy lifting was CPU-intensive, that work could even be done off of the main thread, thanks to Rust’s good support for multi-threading.

@andrewdavidwong andrewdavidwong added P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. P: major Priority: major. Between "default" and "critical" in severity. ux User experience and removed P: minor Priority: minor. The lowest priority, below "default." P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. labels May 30, 2021
@andrewdavidwong
Copy link
Member

I guess in light of #6652 it's not clear to me what the priority of this issue should be.

@andrewdavidwong andrewdavidwong added P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. and removed P: major Priority: major. Between "default" and "critical" in severity. labels May 30, 2021
@jamke
Copy link

jamke commented Jan 12, 2022

One way to reproduce:

  1. Lock xscreensaver
  2. Turn displays off and back on again.
    => Padlock icons are shown instead of proper icons for for all starting application (opening windows) for all appvms that ware running during this process.

The way to "fix" icons for appvm is only to shut it down completely, start again and reopen applications. Without restart nothing gets fixes, padlocks instead of proper icons.

@andrewdavidwong andrewdavidwong added the eol-4.0 Closed because Qubes 4.0 has reached end-of-life (EOL) label Aug 5, 2023
@github-actions
Copy link

github-actions bot commented Aug 5, 2023

This issue is being closed because:

If anyone believes that this issue should be reopened and reassigned to an active milestone, please leave a brief comment.
(For example, if a bug still affects Qubes OS 4.1, then the comment "Affects 4.1" will suffice.)

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 5, 2023
@jamke
Copy link

jamke commented Aug 8, 2023

#1495 (comment)

I just checked this way of reproducing.
Icons stay fine as expected. Probably solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: gui-virtualization eol-4.0 Closed because Qubes 4.0 has reached end-of-life (EOL) P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. r4.0-dom0-stable ux User experience
Projects
None yet
Development

No branches or pull requests