[PATCH] Xext: "xauth generate" with large timeout crashes Xorg #27134 .
Arvind Umrao
Arvind.Umrao at Sun.COM
Wed Apr 14 22:02:53 PDT 2010
Your fixes are better than mine. I have tested your fixes in SPARC and
x86 machine with all the possible range of timeout values.
Thanks and Regards
-Arvind
On 04/14/10 08:08, Keith Packard wrote:
> On Wed, 14 Apr 2010 07:22:24 +0530, Arvind Umrao<Arvind.Umrao at Sun.COM> wrote:
>
>> 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.
>>
> Go look at TimerSet again. For relative timers, it does the following:
>
> timer->delta = millis;
> millis += now;
>
> if ((int) (millis - now)<= 0) {
> ...
>
> All you have to do is ensure that 'millis' is less than MAXINT as
> TimerSet adds 'now' and then subtracts it back off before comparing with
> 0.
>
> The only change needed here was to change the ~0 to MAXINT, plus the
> removal of the assert.
>
> I think this patch will suffice (it compiles, but I haven't run it to check):
>
> diff --git a/Xext/security.c b/Xext/security.c
> index af8d205..e81e560 100644
> --- a/Xext/security.c
> +++ b/Xext/security.c
> @@ -279,10 +279,10 @@ SecurityComputeAuthorizationTimeout(
> /* maxSecs is the number of full seconds that can be expressed in
> * 32 bits worth of milliseconds
> */
> - CARD32 maxSecs = (CARD32)(~0) / (CARD32)MILLI_PER_SECOND;
> + CARD32 maxSecs = (CARD32)(MAXINT) / (CARD32)MILLI_PER_SECOND;
>
> if (seconds> maxSecs)
> - { /* only come here if we want to wait more than 49 days */
> + { /* only come here if we want to wait more than 24 days */
> pAuth->secondsRemaining = seconds - maxSecs;
> return maxSecs * MILLI_PER_SECOND;
> }
> @@ -320,8 +320,6 @@ SecurityAuthorizationExpired(
> {
> SecurityAuthorizationPtr pAuth = (SecurityAuthorizationPtr)pval;
>
> - assert(pAuth->timer == timer);
> -
> if (pAuth->secondsRemaining)
> {
> return SecurityComputeAuthorizationTimeout(pAuth,
>
>
More information about the xorg-devel
mailing list