Page MenuHome GnuPG

gpg-agent: DISPLAY is not set when calling pinentry-qt
Closed, ResolvedPublic

Description

Related Qt report: https://bugreports.qt.io/browse/QTBUG-95549

Calling pinentry-qt without setting the DISPLAY variable can result in an inmediate crash, leading to gpg-agent not working if not connected to a TTY. This can be reproduced as follows:

$ DISPLAY= QT_QPA_PLATFORMTHEME=gtk3 pinentry-qt --display :0

(pinentry-qt:11296): Gtk-WARNING **: 10:51:29.093: cannot open display: 

This is due to libqgtk3.so relying on the DISPLAY variable instead of the --display flag. However, this will only happen on Xorg, as the wayland equivalent, WAYLAND_DISPLAY, is set properly. I propose setting DISPLAY as well.

Details

Version
2.2.28

Event Timeline

mid-kid created this object in space S1 Public.

Proposed patch:

--- gnupg-2.2.27.orig/agent/call-pinentry.c
+++ gnupg-2.2.27/agent/call-pinentry.c
@@ -202,13 +202,14 @@
 
   while ((name = session_env_list_stdenvnames (&iterator, &assname)))
     {
-      /* For all new envvars (!ASSNAME) and the two medium old ones
+      /* For all new envvars (!ASSNAME) and the few medium old ones
        * which do have an assuan name but are conveyed using
        * environment variables, update the environment of the forked
        * process.  */
       if (!assname
           || !strcmp (name, "XAUTHORITY")
-          || !strcmp (name, "PINENTRY_USER_DATA"))
+          || !strcmp (name, "PINENTRY_USER_DATA")
+          || (!opt.keep_display && !strcmp (name, "DISPLAY")))
         {
           value = session_env_getenv (ctrl->session_env, name);
           if (value)

I would prefer to see a fix/hack in pinentry-qt instead.

werner triaged this task as Normal priority.Aug 6 2021, 11:07 AM
werner added projects: pinentry, qt.

Not to be bothersome, but why? DISPLAY seems like the universal method of selecting a display to put things on, where a lot of applications don't support --display or equivalent, especially now there's no equivalent for wayland. It's especially confusing to me when the keep-display option will pass DISPLAY instead of --display. This would also prevent other such scenarios with 3rd party qt/gtk plugins or alternative pinentry implementations.

To minimize the risk of regressions.

I would prefer to see a fix/hack in pinentry-qt instead.

Like setting DISPLAY to the same value as --display, as with the following patch:

diff --git a/qt/main.cpp b/qt/main.cpp
index d2f0684..26f2036 100644
--- a/qt/main.cpp
+++ b/qt/main.cpp
@@ -398,6 +398,13 @@ main(int argc, char *argv[])
         }
         for (done = 0, p = *new_argv, i = 0; i < argc; i++)
             if (!done && !strcmp(argv[i], "--display")) {
+#if defined(Q_OS_UNIX) && !defined(Q_OS_DARWIN)
+                /* Force DISPLAY to the same value as --display, as
+                 * libqgtk3 relies on the environment variable. */
+                if (i < argc - 1) {
+                    qputenv("DISPLAY", argv[i+1]);
+                }
+#endif
                 new_argv[i] = strcpy(p, argv[i] + 1);
                 p += strlen(argv[i] + 1) + 1;
                 done = 1;

?

Yeah, that sounds good to me.

The Qt upstream bug report has just been rejected. I hope something can be done here...

Your proposed fix (in your first comment) has actually already been applied (commit 1305baf0994059f458b1d5ca28a355c12932fab3 in master, backported to the -2.2 branch in 455ba49071dea7588c9de11785b3092e45e4560b). It is part of gnupg-2.2.31 released today. :)

werner claimed this task.

Thanks for commenting. I close this bug then.

I see, I wasn't aware of this. Thanks for fixing!