Bug#1016363: fix for fvwm

[Matthieu Herrb]

XCheckIfEvent() holds the X display lock and the predicate
function it calls is not allowed to call any Xlib function that
would re-enter the lock.
libX11 1.8.1 enables X display locks unconditionnaly (it was only
enabled by XInitThreads() when called explicitely before) and
thus exposes the issue.

So don't process events in the FCheckPeekIfEvent() predicate, but
instead use a separate handler that is called for the returned event
out of the lock.
---
 fvwm/events.c |  9 -
 libs/FEvent.c | 22 ++
 libs/FEvent.h |  8 
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/fvwm/events.c b/fvwm/events.c
index ae179d62..72d85707 100644
--- a/fvwm/events.c
+++ b/fvwm/events.c
@@ -267,6 +267,12 @@ static int _pred_weed_accumulate_expose(
return 1;
 }
 
+static int _pred_weed_is_expose(
+   Display *display, XEvent *event, XPointer arg)
+{
+   return (event->type == Expose);
+}
+
 static int _pred_weed_handle_expose(
Display *display, XEvent *event, XPointer arg)
 {
@@ -4546,7 +4552,8 @@ void handle_all_expose(void)
 
saved_event = fev_save_event();
FPending(dpy);
-   FWeedIfEvents(dpy, _pred_weed_handle_expose, NULL);
+   FWeedAndHandleIfEvents(dpy, _pred_weed_is_expose,
+  _pred_weed_handle_expose, NULL);
fev_restore_event(saved_event);
 
return;
diff --git a/libs/FEvent.c b/libs/FEvent.c
index f3bf54d3..7e062118 100644
--- a/libs/FEvent.c
+++ b/libs/FEvent.c
@@ -534,6 +534,28 @@ int FWeedIfEvents(
return weed_args.count;
 }
 
+int FWeedAndHandleIfEvents(
+   Display *display,
+   int (*weed_predicate) (Display *display, XEvent *event, XPointer arg),
+   int (*handler) (Display *display, XEvent *event, XPointer arg),
+   XPointer arg)
+{
+   _fev_weed_args weed_args;
+   XEvent e;
+
+   assert(fev_is_invalid_event_type_set);
+   memset(&weed_args, 0, sizeof(weed_args));
+   weed_args.weed_predicate = weed_predicate;
+   weed_args.arg = arg;
+   if (FCheckPeekIfEvent(display, &e, _fev_pred_weed_if,
+ (XPointer)&weed_args)) {
+   handler(display, &e, arg);
+   }
+   _fev_pred_weed_if_finish(&weed_args);
+
+   return weed_args.count;
+}
+
 int FWeedIfWindowEvents(
Display *display, Window window,
int (*weed_predicate) (
diff --git a/libs/FEvent.h b/libs/FEvent.h
index ecfb164c..f17ac5e9 100644
--- a/libs/FEvent.h
+++ b/libs/FEvent.h
@@ -114,6 +114,14 @@ int FWeedIfEvents(
Display *display, XEvent *current_event, XPointer arg),
XPointer arg);
 
+/* Same as FWeedIfEvents but with a second callback out of XLockDisplay()
+ * to handle events in a lock-safe manner */
+int FWeedAndHandleIfEvents(
+   Display *display,
+   int (*weed_predicate) (Display *display, XEvent *event, XPointer arg),
+   int (*handler) (Display *display, XEvent *event, XPointer arg),
+   XPointer arg);
+
 /* Same as FWeedIfEvents but weeds only events for the given window.  The
  * weed_predicate is only called for events with a matching window.  */
 int FWeedIfWindowEvents(
-- 
2.37.1

This is: https://github.com/fvwmorg/fvwm3/pull/683
-- 
Matthieu Herrb

Bug#968034: xidle: introduction of setsid() call introduces undocumented behaviour changes

[Matthieu Herrb]

On Fri, Aug 07, 2020 at 09:41:01PM +, Thorsten Glaser wrote:
> Marcel Partap dixit:
> 
> >in my user .tmux.conf, I have put following mechanism
> 
> This is a very complex setup; I cannot easily reproduce what exactly
> makes this fail for you.
> 
> >https://www.mail-archive.com/tech@openbsd.org/msg49105.html
> 
> As I said in my earlier mail, this patch combines multiple changes
> and is… tricky.
> 
> Matthieu, can you have a look at it as well as the change
> http://cvsweb.openbsd.org/cgi-bin/cvsweb/xenocara/app/xidle/xidle.c.diff?r1=1.3&r2=1.4
> possibly introducing the problem for our user?

Hi,

I'm currently on vacation. I will look at this when I come back later
next week.


> 
> I’m seeing multiple things here:
> 
> • closing stdout and stderr: I agree with that output should go
>   to .xsession-errors (if open) and would remove that, redirecting
>   only stdin from /dev/null
> 
> • using execvp: this makes me cringe, both the change and the
>   current code also. I’d propose introducing two new ways of
>   specifying the command to run. One would take one argument,
>   like -program does, but actually run sh -c  (with
>   shell interpolation, pipes, I/O redirection, etc. possible);
>   the other (probably a double dash) would collect all remaining
>   arguments and create an argument vector from them (whitespace-
>   safe), perhaps with an -argv0 option (like exec -a in some shells)
>   and retire -program entirely (or keep it for a while but document
>   it as deprecated, to avoid breaking users right now)
> 
> • calling setsid(2): is this deliberate? What are the effects of
>   doing so vs. not doing so for the programs run?
> 
>   If this is deliberate, perhaps we can introduce a -keepsession
>   flag that would omit the call to setsid(2) only, for use cases
>   like Marcel’s.
> 
> >noticed the introduction of a setsid() call, which seem to be the source of 
> >the
> 
> Are you sure about this? That is, if you locally recompile xidle with
> just the call to setsid removed, does your scenatio work?
> 
> Thanks,
> //mirabilos
> -- 
> (gnutls can also be used, but if you are compiling lynx for your own use,
> there is no reason to consider using that package)
>   -- Thomas E. Dickey on the Lynx mailing list, about OpenSSL

-- 
Matthieu Herrb