[PATCH xf86-input-synaptics 2/2] Ensure history delta computations are signed

[Derek Foreman]

On 02/08/2012 04:41 PM, Chase Douglas wrote:
> On 02/08/2012 11:30 PM, Peter Hutterer wrote:
>> On Wed, Feb 08, 2012 at 09:10:51PM +0100, Chase Douglas wrote:
>>> On 02/08/2012 03:54 PM, Peter Hutterer wrote:
>>>> On Tue, Feb 07, 2012 at 01:07:08PM -0800, Chase Douglas wrote:
>>>>> Signed-off-by: Chase Douglas<chase.douglas at canonical.com>
>>>>> ---
>>>>>   src/synaptics.c |    2 +-
>>>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/src/synaptics.c b/src/synaptics.c
>>>>> index a4c1e5a..106b5ee 100644
>>>>> --- a/src/synaptics.c
>>>>> +++ b/src/synaptics.c
>>>>> @@ -1791,7 +1791,7 @@ HandleTapProcessing(SynapticsPrivate *priv, struct SynapticsHwState *hw,
>>>>>   }
>>>>>
>>>>>   #define HIST(a) (priv->move_hist[((priv->hist_index - (a) + SYNAPTICS_MOVE_HISTORY) % SYNAPTICS_MOVE_HISTORY)])
>>>>> -#define HIST_DELTA(a, b, e) ((HIST((a)).e) - (HIST((b)).e))
>>>>> +#define HIST_DELTA(a, b, e) ((int)((HIST((a)).e) - (HIST((b)).e)))
>>>>
>>>> this only casts the result to a signed int, the actual computation is still
>>>> undefined for b>  a. With the revert, HIST_DELTA is only used in one place
>>>> so we could either do it there or just write a helper function like this
>>>
>>> Why is it undefined? We can have a negative time difference, which is
>>> what the calculation was assuming, IIUC.
>>
>> yeah, but IIRC (unsigned int)a - (unsigned int)b is undefined for b>  a.
>
> I thought in C that was still defined. I think it's used in many places,
> including TCP timestamp wrap-around in the kernel. Maybe what I'm
> remembering is slightly different? I must admit I don't know where or
> how to find a definitive answer though.

I think whether it's defined or not, if b > a when using this macro to 
calculate times it's a bug - either the parameters have been passed 
backwards, or the history contains non-monotonic timestamps.

I suppose the cast could also be problematic if the result proved too 
large to fit into an int, but that would seem to indicate an invalid 
motion history as well.

There's only one usage of this macro left after reverting my motion 
estimator anyway.  Perhaps it should just die (and I'll resurrect it as 
an error checking function later...)