From: Alexander Arkhipov <aa@manpager.org>
Subject: Re: xenocara/app/xidle corner locking and debug code fixes
To: Matthieu Herrb <matthieu@openbsd.org>
Cc: tech@openbsd.org
Date: Wed, 24 Jan 2024 20:38:04 +0400

Matthieu Herrb <matthieu@openbsd.org> wrote:
> On Sun, Jan 14, 2024 at 09:50:08AM +0300, Alexander Arkhipov wrote:
> > Hello, tech@,
> > 
> > I noticed a bug in xidle: if you use the program with corner locking,
> > and then put some window over that corner, then moving the pointer there
> > doesn't actually activate xidle.
> > 
> > The bug is a little weird. There is code in xidle to handle
> > VisibilityNotify, but it doesn't get executed because:
> > 
> > 1. VisibilityChangeMask is hidden behind an "#if 0"
> > 2. The window is created as InputOnly instead of InputOutput
> 
> Hi,

Hi, Matthieu!

> This was explicitely disabled in 2005/10 by fgsch@, the original
> author of xidle(1) with the comment :
> 
>   disable visibilitychangemask.. it has some hmm, side effects.
> 
> So I'd like to take a bit more time to understand what are those sides
> effects or if they have been fixed by further changes since then.

Ah, I was originally looking for a comment like that, but couldn't find
anything. I wonder if the bug can be adequately fixed if we use
PointerMotionMask instead of VisibilityChangeMask. I'll try to find out
later.

> > The first diff below fixes both issues. I've been running this fixed
> > version for several days now and noticed no problems, and there are no
> > comments, so I suspect this was left in as a result of mistake.
> > 
> > Another issue I noticed while dealing with the first is that some of the
> > debug printing doesn't work because at one point std{in,out,err} get
> > redirected to /dev/null even if DEBUG is defined. The second diff fixes
> > that. I've also had an idea of instead turning everything hidden behind
> > "#if*def DEBUG"s into a run-time option (perhaps -v or -debug), but I'd
> > like to see some feedback before I do that.
> 
> I prefer to leave the debug code as a compile-time option. It's not
> something regular users (who do not plan to change the code) need
> everyday. Less code -> less attack surface.

OK, but in this case I think that at the very least line 352:

	dup2(fd, STDOUT_FILENO);

should be rewritten like so:

#ifndef DEBUG
	dup2(fd, STDOUT_FILENO);
#endif

(or my second diff should be applied wholly.)

> > Lastly, I'm not sure if the two diffs will apply on top of each other,
> > but both are trivial enough to be typed in by hand.
> > 
> > These are my very first diffs to tech@, so I'd appreciate some feedback.
> > Thank you.
> > 
> > Index: xidle.c
> > ===================================================================
> > RCS file: /var/cvs/xenocara/app/xidle/xidle.c,v
> > diff -u -p -r1.10 xidle.c
> > --- xidle.c	25 Oct 2021 09:30:33 -0000	1.10
> > +++ xidle.c	14 Jan 2024 06:15:14 -0000
> > @@ -129,15 +129,12 @@ init_x(struct xinfo *xi, int position, i
> >  
> >  	attr.override_redirect = True;
> >  	xi->win = XCreateWindow(dpy, DefaultRootWindow(dpy),
> > -	    xi->coord_x, xi->coord_y, area, area, 0, 0, InputOnly,
> > +	    xi->coord_x, xi->coord_y, area, area, 0, 0, InputOutput,
> >  	    CopyFromParent, CWOverrideRedirect, &attr);
> >  
> >  	if (position != none) {
> >  		XSelectInput(dpy, xi->win, EnterWindowMask|StructureNotifyMask
> > -#if 0
> > -			       |VisibilityChangeMask
> > -#endif
> > -		);
> > +		    |VisibilityChangeMask);
> >  		XMapWindow(dpy, xi->win);
> >  	}
> >  
> > 
> > And the second diff:
> > 
> > 
> > Index: xidle.c
> > ===================================================================
> > RCS file: /var/cvs/xenocara/app/xidle/xidle.c,v
> > diff -u -p -r1.10 xidle.c
> > --- xidle.c	25 Oct 2021 09:30:33 -0000	1.10
> > +++ xidle.c	14 Jan 2024 06:16:37 -0000
> > @@ -327,7 +327,9 @@ main(int argc, char **argv)
> >  	char *args[10];
> >  	int area = 2, delay = 2, timeout = 0;
> >  	int position = north|west;
> > +#ifndef DEBUG
> >  	int fd;
> > +#endif
> >  	u_long last_serial = 0;
> >  
> >  	parse_opts(argc, argv, &x.dpy, &area, &delay, &timeout,
> > @@ -345,6 +347,7 @@ main(int argc, char **argv)
> >  	signal(SIGTERM, handler);
> >  	signal(SIGUSR1, handler);
> >  
> > +#ifndef DEBUG
> >  	fd = open(_PATH_DEVNULL, O_RDWR);
> >  	if (fd < 0)
> >  		err(1, _PATH_DEVNULL);
> > @@ -353,6 +356,7 @@ main(int argc, char **argv)
> >  	dup2(fd, STDERR_FILENO);
> >  	if (fd > 2)
> >  		close(fd);
> > +#endif
> >  
> >  	if (pledge("stdio proc exec", NULL) == -1)
> >  		err(1, "pledge");
> > 
> > -- 
> > Alexander
> > 


-- 
Alexander