On 23/09/24(Mon) 20:56, Ulf Brosziewski wrote:
> On 9/21/24 13:16, Martin Pieuchot wrote:
> > On 16/09/24(Mon) 23:12, Ulf Brosziewski wrote:
> >> This patch fixes a bug in the 'mousecfg' part of wsconsctl, which can
> >> lead to some display errors if wsconsctl commands are constructed in
> >> a somewhat unusual way.  When wsconsctl fields that are linked to more
> >> than one wsmouse parameter are being changed and subsequently read
> >> within a single command, stale values are read from the mousecfg buffer:
> >>
> >> $ wsconsctl mouse.tp.edges
> >> mouse.tp.edges=0.0,0.5,0.0,0.5
> >> $ wsconsctl mouse.tp.edges=1,2,3,4 mouse.tp.edges
> >> mouse.tp.edges -> 1.0,2.0,3.0,4.0
> >> mouse.tp.edges=0.0,0.5,0.0,4.0
> >> $ wsconsctl mouse.tp.edges
> >> mouse.tp.edges=1.0,2.0,3.0,4.0
> >>
> >> Obviously, there is no reason to chain WRITE and READ operations in
> >> this way, but it's possible.
> > 
> > Shouldn't we use the same idiom present in the other loops?
> > 
> > 	if ((n = index_of(field->params[i].key)) >= 0)
> > 		cfg_buffer[n].value = field->params[i].value;
> > 
> > Or are we sure the values cannot be changed to something invalid between
> > the two ioctls?
> 
> In its ioctl operations wsmouse(4) does not and must not change the 'key'
> fields, it would break the mechanism.  Invalid 'key' entries may only
> result from bad input in a 'mouse.param' argument.  However, the check
> is necessary everywhere else, we could add it here for stylistic reasons.

Thanks for the explanation.  ok mpi@ either way.

> >> Index: mousecfg.c
> >> ===================================================================
> >> RCS file: /cvs/src/sbin/wsconsctl/mousecfg.c,v
> >> retrieving revision 1.10
> >> diff -u -p -r1.10 mousecfg.c
> >> --- mousecfg.c	2 Jul 2023 21:44:04 -0000	1.10
> >> +++ mousecfg.c	16 Sep 2024 20:16:56 -0000
> >> @@ -222,8 +222,10 @@ mousecfg_put_field(int fd, struct wsmous
> >>  	    || (err = ioctl(fd, WSMOUSEIO_GETPARAMS, field)))
> >>  		return err;
> >>
> >> -	for (i = 0; i < field->nparams; i++)
> >> +	for (i = 0; i < field->nparams; i++) {
> >> +		n = index_of(field->params[i].key);
> >>  		cfg_buffer[n].value = field->params[i].value;
> >> +	}
> >>
> >>  	return (0);
> >>  }
> >>
> > 
>