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

j4-dmenu-desktop --no-exec improperly handles terminal applications #166

Closed
benjamb opened this issue Jun 5, 2024 · 9 comments
Closed
Milestone

Comments

@benjamb
Copy link

benjamb commented Jun 5, 2024

Version 2.18 would output/launch the application using $term -e <script>, however this now just prints the plain command, which when piped to something like xargs swaymsg exec -- fails to run any terminal commands such as aerc.

Example desktop file:

[Desktop Entry]
Version=1.0
Name=aerc

GenericName=Mail Client
GenericName[de]=Email Client
Comment=Launches the aerc email client
Comment[de]=Startet den aerc Email-Client
Keywords=Email,Mail,IMAP,SMTP
Categories=Office;Network;Email;ConsoleOnly

Type=Application
Icon=utilities-terminal
Terminal=true
Exec=aerc %u
MimeType=x-scheme-handler/mailto

Previous output:

% j4-dmenu-desktop --no-exec --dmenu bemenu --term=foot
Read 85 .desktop files, found 55 apps.
User input is: aerc 
foot -e "/tmp/j4-dmenu-g3vqLO"

New output:

% j4-dmenu-desktop --no-exec --dmenu bemenu --term=foot
Read 85 .desktop files, found 55 apps.
User input is: aerc
aerc

This is a breaking change not announced in the changelogs.

@benjamb benjamb changed the title j4-dmenu-desktop can no longer handle terminal applications j4-dmenu-desktop --no-exec improperly handles terminal applications Jun 5, 2024
@meator
Copy link
Collaborator

meator commented Jun 6, 2024

This is a breaking change not announced in the changelogs.

This is a bug & not an intended change. I will work on a fix. Thanks for reporting this issue!

[...] when piped to something like xargs swaymsg exec -- fails to run any terminal commands such as aerc.

j4-dmenu-desktop r3.0 adds support for i3 IPC. Sway IPC appears to be compatible with i3 IPC, so the -I (or --i3-ipc) flag should work the same as your xargs solution while providing tighter and faster integration with j4-dmenu-desktop + it handles quoting well when forwarding desktop file executables.

I will document Sway support in the Examples section. I am not a Sway user though, so I'll have to test this further. Would you mind trying it out if you have spare time? I assume that you are a Sway user because you make mention of it. I'd like to know whether the following command:

j4-dmenu-desktop -I

works as expected on Sway. If it doesn't, I'd like to know whether the following change

I3SOCK="$(sway --get-socketpath)" j4-dmenu-desktop -I

fixes it (if the first command doesn't work).

Both of these commands will require the i3 executable to be present. j4-dmenu-desktop calls i3 --get-socketpath internally to get the socket path. i3 isn't used otherwise. I will fix this in the next release (this is a very simple fix).


The r3.0 release was made recently. I would like to fix these issues (and any other issues that should occur) as soon as possible, but I don't want to spam releases. I was considering making release r3.1 around next month.

@meator
Copy link
Collaborator

meator commented Jun 6, 2024

A workaround that would remove the requirement for i3 executable would be to create the following script:

#!/bin/sh
exec sway --get-socketpath

name it i3, make it executable and execute j4-dmenu-desktop with altered $PATH so that the script gets executed when j4-dmenu-desktop requests i3 --get-socketpath.

This workaround should work well in the r3.0 release. I haven't tested it though.

@meator meator added this to the r3.1 milestone Jun 6, 2024
@meator meator mentioned this issue Jun 6, 2024
meator added a commit that referenced this issue Jun 6, 2024
Overriding i3 socket path was fully supported before (as documented in
the manpage). The mechanism relied on propagating I3SOCK environmental
variable to i3 executable which would then acknowledge it in
--get-socketpath. This can be troublesome when using other window
managers compatible with i3 IPC (like Sway). It introduces an arbitrary
requirement on i3 executable which is unnecessary because it's only used
for `i3 --get-socketpath`.

j4-dmenu-desktop now looks for I3SOCK and it handles it itself. When
$I3SOCK is set, i3 (nor sway) is executed whatsoever.

related to #166
@meator
Copy link
Collaborator

meator commented Jun 6, 2024

j4-dmenu-desktop from develop branch should now recognize Sway by simply calling

j4-dmenu-desktop -I

I will have to test it further.


j4-dmenu-desktop r2.18 used a temporary file to handle terminal applications. This approach was overly complicated and largely unnecessary. From r3.0 onwards, j4-dmenu-desktop executes programs itself without any wrapper scripts. This means that j4-dmenu-desktop doesn't have to worry about managing and removing temporary files.

It also removes the dependency on the /tmp directory. It doesn't have to exist or be writable (but it usually is).

These changes complicate quoting though. There can be up to two levels of indirection when executing commands. Different shells have different quoting rules (j4-dmenu-desktop strictly uses /bin/sh for executing stuff when appropriate which makes quoting reliable, but "external executors" parsing the output of --no-exec like xargs can have special quoting rules which can complicate things).

The --i3-ipc flag should be preferred, because it gives j4-dmenu-desktop full control of quoting.

@benjamb
Copy link
Author

benjamb commented Jun 6, 2024

I've just given the latest commit a go with:

j4-dmenu-desktop -I

Note that I3SOCK already appears to be set (I presume by sway) on the environment, along with SWAYSOCK, if that's of any note:

I3SOCK=/run/user/1000/sway-ipc.1000.2544.sock
SWAYSOCK=/run/user/1000/sway-ipc.1000.2544.sock

j4-dmenu-desktop now successfully launches both graphical and terminal applications, though it seems to expect some kind of response from sway via IPC that may not match up with what it receives, and subsequently aborts and core dumps:

$ j4-dmenu-desktop -I --no-generic --term=foot --dmenu='bemenu -i'
Read 28 .desktop files, found 17 apps.
User input is: aerc
[2024-06-06 16:54:52.129] [error] [I3Exec.cc:308] A parsing error occurred while reading i3's response (executing command 'foot -e /bin/sh -c 'exec aerc'')!
Aborted (core dumped)
User input is: Thunderbird
[2024-06-06 16:56:41.554] [error] [I3Exec.cc:308] A parsing error occurred while reading i3's response (executing command 'thunderbird --name thunderbird')!
Aborted (core dumped)

@benjamb
Copy link
Author

benjamb commented Jun 6, 2024

@meator Thanks for jumping on this so quickly!

@benjamb
Copy link
Author

benjamb commented Jun 6, 2024

With debug logging enabled:

[2024-06-06 18:03:12.956] [info] [main.cc:463] User input is: aerc
[2024-06-06 18:03:12.962] [debug] [I3Exec.cc:277] I3 IPC response: [ { "success": true } ]
[2024-06-06 18:03:12.962] [error] [I3Exec.cc:308] A parsing error occurred while reading i3's response (executing command '/nix/store/ybsgi7i42x27p9hjbj1wrn2q608gbsdv-foot-1.17.2/bin/foot -e /bin/sh -c 'exec aerc'')!
Aborted (core dumped)

@benjamb
Copy link
Author

benjamb commented Jun 6, 2024

From a quick glance at the code, looks like the rudimentary JSON parsing is falling over due to differences in whitespace in the sway IPC response.

meator added a commit that referenced this issue Jun 7, 2024
wrap_command_in_wrapper() is yet to be implemented.

related to #166
meator added a commit that referenced this issue Jun 7, 2024
This solution isn't ideal. But it works, and that's the important part.

Related to #166
@meator
Copy link
Collaborator

meator commented Jun 7, 2024

All problems should be fixed now. It was pretty easy to fix i3 IPC, but the --no-exec problem required more work. My changes broke the --wrapper flag. I haven't reimplemented it yet.

--no-exec is now more paranoid when quoting things. You should be able to see that single quotes are used a lot now. j4-dmenu-desktop doesn't use them when it executes things itself (unless i3 IPC mode is enabled), but they are used in --no-exec mode. This should mean that you should be able to pass output of --no-exec to any shell without worrying about escape sequences causing syntax errors or shell injections being possible.

@benjamb Would you be willing to test develop again & close this issue if everything's working?

@benjamb
Copy link
Author

benjamb commented Jun 9, 2024

@meator Everything appears to be working as expected, thanks!

@benjamb benjamb closed this as completed Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants