Re: Fix overflow check in sys/kern/kern_time.c

[Todd C. Miller]

On Wed, 23 Mar 2016 10:58:42 -0400, Michael McConville wrote:

> I'm not sure whether avoiding incrementing here is an ideal move, but
> this diff definitely works toward a local optimum. Namely, that error
> check is technically meaningless because signed overflow is undefined.
> 
> ok? Or would people prefer a solution that's robust to changing
> *curpps's type?

Is there any reason for the counters to be signed?  In general I
don't think it makes much sense for counters to be signed.

 - todd

Re: Fix overflow check in sys/kern/kern_time.c

[Michael McConville]

Mark Kettenis wrote:
> > I'm not sure whether avoiding incrementing here is an ideal move, but
> > this diff definitely works toward a local optimum. Namely, that error
> > check is technically meaningless because signed overflow is undefined.
> 
> Within the kernel, signed overflow actually is well-defined.  We don't
> run on hypethetical one's complement machines (or machines that use
> some other weird scheme to encode signed integers) and we pass flags
> to the compiler that make it do the right thing.
> 
> Yes, the fact that the C standard doesn't want to make assumptions
> about the encoding of signed inttegers is the real reason why the
> standard leaves integer overflow undefined.  Unfortunately that has
> lead the language lwayer types in charge of compilers to implement
> misguided optimizations that break stuff.

I agree that it's unfortunate that signed overflow is undefined. I think
ignoring that fact is a long-term problem, though. Presumably, we're
eventually going to switch compilers, and that could silently break
things. In particular, the flags used for saner signed overflow behavior
are often poorly maintained or broken. For example, -ftrapv is
completely broken (no effect) in both the base and port GCC. If we
decide to depend on signed overflow, we restrict ourselves to compilers
implementing our non-standard dialect of C, and we rely on these
somewhat uncommonly used flags.

> > ok? Or would people prefer a solution that's robust to changing
> > *curpps's type?
> 
> I you absolutely want to "fix" this issue, I'd prefer that you just
> fixed the check and left the comment in place.  Then at least the code
> continues to document the corner case.  So simply:
> 
> > -   if (*curpps + 1 > *curpps)
> > +   if (*curpps != INT_MAX)
> 
> Of course that breaks if you start out with a negative number...

Can you explain? I don't understand.

> So I think I'd prefer if you simply left this alone.

Re: Fix overflow check in sys/kern/kern_time.c

[Mark Kettenis]

> Date: Wed, 23 Mar 2016 10:58:42 -0400
> From: Michael McConville 
> 
> I'm not sure whether avoiding incrementing here is an ideal move, but
> this diff definitely works toward a local optimum. Namely, that error
> check is technically meaningless because signed overflow is undefined.

Within the kernel, signed overflow actually is well-defined.  We don't
run on hypethetical one's complement machines (or machines that use
some other weird scheme to encode signed integers) and we pass flags
to the compiler that make it do the right thing.

Yes, the fact that the C standard doesn't want to make assumptions
about the encoding of signed inttegers is the real reason why the
standard leaves integer overflow undefined.  Unfortunately that has
lead the language lwayer types in charge of compilers to implement
misguided optimizations that break stuff.

> ok? Or would people prefer a solution that's robust to changing
> *curpps's type?

I you absolutely want to "fix" this issue, I'd prefer that you just
fixed the check and left the comment in place.  Then at least the code
continues to document the corner case.  So simply:

> - if (*curpps + 1 > *curpps)
> + if (*curpps != INT_MAX)

Of course that breaks if you start out with a negative number...

So I think I'd prefer if you simply left this alone.

Cheers,

Mark


> Index: sys/kern/kern_time.c
> ===
> RCS file: /cvs/src/sys/kern/kern_time.c,v
> retrieving revision 1.96
> diff -u -p -r1.96 kern_time.c
> --- sys/kern/kern_time.c  5 Dec 2015 10:11:53 -   1.96
> +++ sys/kern/kern_time.c  10 Feb 2016 05:25:35 -
> @@ -765,20 +765,8 @@ ppsratecheck(struct timeval *lasttime, i
>   else
>   rv = 0;
>  
> -#if 1 /*DIAGNOSTIC?*/
> - /* be careful about wrap-around */
> - if (*curpps + 1 > *curpps)
> - *curpps = *curpps + 1;
> -#else
> - /*
> -  * assume that there's not too many calls to this function.
> -  * not sure if the assumption holds, as it depends on *caller's*
> -  * behavior, not the behavior of this function.
> -  * IMHO it is wrong to make assumption on the caller's behavior,
> -  * so the above #if is #if 1, not #ifdef DIAGNOSTIC.
> -  */
> - *curpps = *curpps + 1;
> -#endif
> + if (*curpps != INT_MAX)
> + (*curpps)++;
>  
>   return (rv);
>  }
> 
>