Comment on attachment 182077 [details] [review]
Proposed patch

>+AC_PROG_CC_C99

GNOME still prefers to stick to C89 (although that's getting increasingly silly at this point...)

>+dnl *** Detect whether time_t is integer or float ***

Wow... that's totally insane. I'd never realized it wasn't required to be integral.

But anyway, I don't think we need all this anyway. For cookie purposes, dates beyond 2038 are essentially "never", and any date before now is just "expired". So we can truncate to that range.

>-SoupDate *soup_date_new_from_now    (int             offset_seconds);
>+SoupDate *soup_date_new_from_now    (long            offset_seconds);

This would change the ABI. You need to leave the methods (here and in soup-cookie) with their existing signatures.

>+	max_age = (expire_time - now <= LONG_MAX ? expire_time - now : LONG_MAX);

This part all looks good, though it would have to be INT_MAX now.

>+soup_date_new_from_now (long offset_seconds)
> {
>-	return soup_date_new_from_time_t (time (NULL) + offset_seconds);
>+	time_t now = time(NULL);
>+
>+	if (now > TIME_T_MAX - offset_seconds)
>+		return soup_date_new_from_time_t (TIME_T_MAX);
>+
>+	return soup_date_new_from_time_t (now + offset_seconds);
> }

So, offset_seconds has to go back to int, and we know that time_t is at least as big as int, so now + offset_seconds can't wrap around more than once. So this should work, I think?

    time_t then = now + offset_seconds;

    if (offset_seconds < 0 && then > now)
        return soup_date_new_from_time_t (0);
    else if (offset_seconds > 0 && then < now)
        return soup_date_new_from_time_t (INT_MAX);
    else
        return soup_date_new_from_time_t (then);

(In reply to comment #1)

> >+	max_age = (expire_time - now <= LONG_MAX ? expire_time - now : LONG_MAX);
> 
> This part all looks good, though it would have to be INT_MAX now.

Ok. With just a small note from my side: int may be 2 or 3 bytes long as well, in which case it won't be Y2038 issue anymore ;)
How about adding a sizeof(int) check in the configure script with an error message in case the size is less than 4 bytes?

> >+soup_date_new_from_now (long offset_seconds)
> > {
> >-	return soup_date_new_from_time_t (time (NULL) + offset_seconds);
> >+	time_t now = time(NULL);
> >+
> >+	if (now > TIME_T_MAX - offset_seconds)
> >+		return soup_date_new_from_time_t (TIME_T_MAX);
> >+
> >+	return soup_date_new_from_time_t (now + offset_seconds);
> > }
> 
> So, offset_seconds has to go back to int, and we know that time_t is at least
> as big as int, so now + offset_seconds can't wrap around more than once. So
> this should work, I think?

Do you have any spec reference which says that time_t is at least as big as int?
I don't see any such limitation here: http://www.opengroup.org/onlinepubs/000095399/basedefs/sys/types.h.html

>     time_t then = now + offset_seconds;

Not good. Will overflow. All the checks should be done before the addition:
    time_t now = time(NULL);

    if (offset_seconds < 0 && now < -offset_seconds)
        return soup_date_new_from_time_t (0);
    else if (offset_seconds > 0 && now > INT_MAX - offset_seconds)
        return soup_date_new_from_time_t (INT_MAX);
    else
        return soup_date_new_from_time_t (now + offset_seconds);

But I also don't like the following line:
>         return soup_date_new_from_time_t (INT_MAX);
because I'm not sure that time_t is at least as big as int. Therefore, I suppose that we may overflow here as well.

(In reply to comment #2)
> Ok. With just a small note from my side: int may be 2 or 3 bytes long as well,

glib does not build on any platform where int is smaller than 4 bytes

> Do you have any spec reference which says that time_t is at least as big as
> int?

If time_t was smaller than 4 bytes it wouldn't be able to represent any dates after July 1970.

> >     time_t then = now + offset_seconds;
> 
> Not good. Will overflow.

But as long as sizeof(int) <= sizeof(time_t), it is guaranteed that it can only wrap around once. Thus, if you're adding a positive offset_seconds, and it overflows, the result will be less than now, and if you're adding a negative and it overflows, the result will be more than now.


I've committed a patch based on yours. Thanks for the bug report and patch.

May I propose a small cleanup to the commit you've made?

In the libsoup/soup-date.c file it is probably better to use (G_MININT32) and (G_MAXINT32) instead of (-G_MAXINT) and (G_MAXINT) correspondingly.