[PATCH] Xext: "xauth generate" with large timeout crashes Xorg #27134 .
Arvind Umrao
Arvind.Umrao at Sun.COM
Tue Apr 13 18:52:24 PDT 2010
Keith Packard wrote:
> On Tue, 13 Apr 2010 16:50:30 +0530, Arvind Umrao <Arvind.Umrao at Sun.COM> wrote:
>
>> On 04/13/10 10:57, Keith Packard wrote:
>>
>>> On Tue, 13 Apr 2010 10:16:20 +0530, Arvind Umrao<Arvind.Umrao at Sun.COM> wrote:
>>>
>>>
>>>
>>>> CARD32 maxSecs = (CARD32)(~0) / (CARD32)MILLI_PER_SECOND;
>>>> + CARD32 nowSec = GetTimeInMillis()/ (CARD32)MILLI_PER_SECOND;
>>>>
>>>> - if (seconds> maxSecs)
>>>> - { /* only come here if we want to wait more than 49 days */
>>>> - pAuth->secondsRemaining = seconds - maxSecs;
>>>> - return maxSecs * MILLI_PER_SECOND;
>>>> + CARD32 maxPossibleSec = maxSecs - nowSec;
>>>>
>>>>
>>> What happens when GetTimeInMillis() returns (CARD32)(~0)?
>>>
>>>
>>>
>> Yes I agree you found problem in my fix. When GetTimeInMillis() returns
>> (CARD32)(~0) , execution should go to overflow timeout, not in regular
>> timeout. I have made the correction please review it and give your
>> comments again.
>>
>
> The actual requirement for the milliseconds value is that it not
> fail the comparison below:
>
> if ((int) (millis - now) <= 0)
>
> This means keeping the milliseconds value within 31 bits of the now
> value. There should be no reason to ever use GetTimeInMillis() to range
> check the incoming value.
>
Please again, reconfirm should I remove GetTimeInMillis(). If I do not
keep the milliseconds value within 31 bits of 'now' value, Security
Authorization will get expired immediately with setting timeout to zero,
when user supply large value of timeout.
> In addition, I'd suggest that the assert in SecurityAuthorizationExpired
> be removed; that's a bogus test with pAuth->timer is NULL, as it will be
> when TimerSet is first invoked.
>
Yes I do agree with you. Assert in SecurityAuthorizationExpired should
be removed and it is bogus. I will recreate patch and sent it for review.
More information about the xorg-devel
mailing list