[PATCH weston] input: Don't use uninitialized variables in default_grab_pointer_focus()

[Derek Foreman]

On 13/04/15 01:51 AM, Pekka Paalanen wrote:
> On Fri, 10 Apr 2015 11:13:22 -0500
> Derek Foreman <derekf at osg.samsung.com> wrote:
> 
>> On 10/04/15 09:41 AM, Derek Foreman wrote:
>>> On 10/04/15 02:16 AM, Pekka Paalanen wrote:
>>>> On Wed,  8 Apr 2015 15:18:41 -0500
>>>> Derek Foreman <derekf at osg.samsung.com> wrote:
>>>>
>>>>> If we have no pointer focus and weston_compositor_pick_view() returns NULL
>>>>> default_grab_pointer_focus() will test the unset sx, sy output parameters
>>>>> from weston_compositor_pick_view().
>>>>>
>>>>> Instead, assume that since both pointers are NULL focus hasn't changed and
>>>>> we don't need to update pointer focus.
>>>>>
>>>>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>>>>> ---
>>>>>
>>>>> This fixes an error reported by valgrind.  I think the patch is correct but
>>>>> it does stop pointer->focus_signal from being emitted when focus switches
>>>>> from NULL to NULL.  I think that shouldn't have happened anyway?
>>>>
>>>> If focus switches from NULL to NULL, weston_pointer_set_focus() would
>>>> never be called because of the pointer->focus != view check, right?
>>>>
>>>> So the case you saw in Valgrind must be a case of !NULL -> NULL switch,
>>>> which has to be processed but does not have valid sx, sy by definition.
>>>> Therefore...
>>>>
>>>>> Initializing sx, sy to wl_fixed_from_int(0) would silence the warning too,
>>>>> but I think this way is more correct...
>>>>
>>>> ...isn't initializing the sx, sy the right thing to do, and this patch
>>>> actually fixes nothing?
>>>>
>>>> Did I miss something?
>>>
>>> As did I.
>>>
>>> As discussed on irc, the null to null case doesn't short circuit the ||
>>> so the patch does "something"...
> 
> Ahaha, yeah, silly me, can't read logic operators right... sorry.
> 
>>> However, we probably shouldn't be getting here for a null to null switch
>>> anyway, so I need to investigate why that's happened in the first place.
>>>
>>> Initializing sx and sy would really have the same result, so I'm going
>>> to avoid that for now as well.
>>
>> On further investigation this happens in two places:
>> From udev_input_init() which is called very early, so device_added()
>> gets called before the view list contains anything at all.  This could
>> be hidden by setting input->suspended for the duration of
>> udev_input_init().  However, if the user is moving the mouse around
>> during startup, it'll still happen.
> 
> Sounds like that special-casing is not worth it, then.
> 
>> During the first repaint when only the shell's fade surface exists.
>> This can be hidden by giving the shell's fade surface an input region, I
>> guess.  This'll also hide the mouse cursor during fade in, which is a
>> little weird/wrong.
> 
> Yeah, not good.
> 
>> I think we need to deal with this case with either what I've done, or
>> initializing sx, sy.  exposay appears to set a NULL focus with non-zero
>> co-ordinates, so there may actually be a functional difference between
>> these, though I'm having a hard time figuring out what it is, and if
>> it's even intentional.
> 
> Right...
> 
> Let's take a step back. If weston_compositor_pick_view() returns NULL,
> then there is no surface having input at that point. As there is no
> surface, no value for sx,sy makes sense, because you don't even have a
> coordinate system to begin with (a surface would define the coordinate
> system). Therefore, sx,sy should not be used at all, they are invalid
> in any case.
> 
> Comparing invalid sx,sy to previous sx,sy makes no sense either. It
> just should not be done. Comparing previous sx,sy to new sx,sy only
> makes sense if the coordinate systems are the same, that means, the
> previous and current picked surface are the same and not NULL.
> 
> This brings us to what default_grab_pointer_focus() should do. Sx,sy
> should be compared only if the previous and current surface are the
> same non-NULL. In all other cases, the comparison is illegal.
> 
> If the picked surface is different than the previous one, then we
> obviously need to call weston_pointer_set_focus(). If previous and
> current are NULL, then it should just do nothing, because there are no
> sx,sy and going from no-target to no-target is not a change.

Ok, that's all I've handled in the previous patch.

> This leaves the problem when previous surface was non-NULL, but the new
> pick is NULL, because weston_pointer_set_focus() needs some sx,sy
> arguments. In this case, I'd suggest we pick some arbitrary sx,sy that
> is not easily confused with proper coordinates. Maybe something like
> wl_fixed_from_int(-1000000).

Ah, yes.  I don't know if that ever happens right now, but we should
prevent it.

I'd like to be a little more aggressive in stamping this out - how about
if view is NULL we expect 0s to be passed along with it to
weston_pointer_set_focus() (and assert otherwise).  Then internally in
weston_pointer_set_focus() we set sx, sy to wl_fixed_from_int(-1000000).

Can it be a normal integer 0 instead of a wl_fixed_from_int(0) since
it's being ignored anyway? ;)

exposay is the only caller that tries to use non 0 with a NULL view, and
I haven't been able to figure out a reason why that's useful - we've
just declared that case to be invalid.  I'm planning to just make that 0
too.

> If anything ends up actually computing with that value while the pick
> is NULL, that's another bug to fix.
> 
> Would this make sense?

Hopefully anything doing a computation with -1000000 will stand out as
wrong immediately.  Valgrind's nice in that it can give me a line
number, instead of 'something looks funny' some time further on...

> The strange thing here I do not understand is why would you need to
> call weston_pointer_set_focus() if the view stays the same but only
> coordinates change? Is that something that shouldn't happen? Is that
> about bumping the input serial? It would cause leave/enter, and... ooh!
> 
> I know! We use an arbitrary leave/enter pair to signal pointer location
> changes to clients when there is no physical input. That is, the
> pointer device doesn't move, but the surface or view moves. We cannot
> fake a wl_pointer.motion event, because it's not a user's physical
> action. It happens when the surface/view moves under the pointer for any
> reason.

Ah yes, I can confirm that with two mice on separate seats.  If I drag a
window under the stationary pointer it generates events.

> Thanks,
> pq
> 
>>>>>  src/input.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/src/input.c b/src/input.c
>>>>> index 142c670..18d1262 100644
>>>>> --- a/src/input.c
>>>>> +++ b/src/input.c
>>>>> @@ -157,6 +157,9 @@ default_grab_pointer_focus(struct weston_pointer_grab *grab)
>>>>>  					   pointer->x, pointer->y,
>>>>>  					   &sx, &sy);
>>>>>  
>>>>> +	if (!pointer->focus && !view)
>>>>> +		return;
>>>>> +
>>>>>  	if (pointer->focus != view || pointer->sx != sx || pointer->sy != sy)
>>>>>  		weston_pointer_set_focus(pointer, view, sx, sy);
>>>>>  }
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>