Possibility of integer overflow in JfrThreadSampler::run()

[Severin Gehwolf]

Hi Yasumasa,

On Fri, 2019-01-11 at 12:02 +0900, Yasumasa Suenaga wrote:
> Hi Severin,
> 
> I checked JfrThreadSampler::run() again, and I'm sure that (last_ms -
> now_ms) will be negative value in any case.
> Thus next_j and next_n will not be overflown.
> 
> So I uploaded a new webrev. It works fine with jdk_jfr jtreg tests.
> 
>   http://cr.openjdk.java.net/~ysuenaga/jfr-integer-overflow/webrev.3/
> 
> I will send review request if you are ok.

This looks reasonable. It needs a comment as to why parenthesis are
needed. Suggestion:

/*
 * Let I be java_interval or native_interval.
 * Let L be last_java_ms or last_native_ms.
 * Let N be now_ms.
 *
 * Interval, I, might be max_jlong so the addition
 * could potentially overflow without parenthesis (UB). Also note that
 * L - N < 0. Avoid UB, by adding parenthesis.
 */

Thanks,
Severin

> 
> Thanks,
> 
> Yasumasa
> 
> 
> 2019年1月11日(金) 1:11 Severin Gehwolf <sgehwolf at redhat.com>:
> > 
> > Hi Yasumasa,
> > 
> > On Thu, 2019-01-10 at 23:08 +0900, Yasumasa Suenaga wrote:
> > > Hi Severin,
> > > 
> > > > This fix works for me:
> > > > 
http://cr.openjdk.java.net/~sgehwolf/webrevs/ub-jfr-sampler-fix/webrev/
> > > > 
> > > > Feel free to use it.
> > > 
> > > Thanks!
> > > I modified your patch as below. It works fine on my environment.
> > > 
> > >    
> > > http://cr.openjdk.java.net/~ysuenaga/jfr-integer-overflow/webrev.2/
> > 
> > Could you explain a little why you've modified it the way you did?
> > It
> > seems overly convoluted for some rather theoretical overflow cases.
> > Perhaps somebody could shed some light on what a setting of:
> > 
> > _interval_java == 0
> > 
> > is supposed to mean for the JfrThreadSampler. It looks to me it's
> > max_jlong setting is the problematic case triggering this issue.
> > 
> > > I filed this issue to JBS:
> > > 
> > >    https://bugs.openjdk.java.net/browse/JDK-8216486
> > 
> > Thanks! Since you've got this bug now, please send it for official
> > RFR
> > with a proper subject.
> > 
> > Cheers,
> > Severin
> > 
> > > Yasumasa
> > > 
> > > 
> > > On 2019/01/10 20:06, Severin Gehwolf wrote:
> > > > Hi Yasumasa,
> > > > 
> > > > On Wed, 2019-01-09 at 20:27 +0100, Severin Gehwolf wrote:
> > > > > Hi,
> > > > > 
> > > > > On Wed, 2019-01-09 at 13:23 +0900, Yasumasa Suenaga wrote:
> > > > > > Hi all,
> > > > > > 
> > > > > > I posted this issue in May 2018 [1].
> > > > > > It still occurs in current jdk/jdk when it was build by GCC
> > > > > > 8.
> > > > > > 
> > > > > > We can avoid it with -fno-strict-overflow GCC option. So I
> > > > > > propose to add it
> > > > > > to JvmOverrideFiles.gmk:
> > > > > > 
> > > > > >    
> > > > > > http://cr.openjdk.java.net/~ysuenaga/jfr-integer-overflow/webrev.1/
> > > > > > 
> > > > > > JDK-8147466 has been fixed with similar change for jdk7u
> > > > > > [2].
> > > > > > 
> > > > > > What do you think?
> > > > > 
> > > > > This fix is masking undefined behavior (signed integer
> > > > > overflow) in
> > > > > code. We should fix the code instead. The previous fix looked
> > > > > better,
> > > > > but would still overflow if:
> > > > > 
> > > > > max(_interval_java, 10) + last_java_ms > max_jlong
> > > > 
> > > > This fix works for me:
> > > > 
http://cr.openjdk.java.net/~sgehwolf/webrevs/ub-jfr-sampler-fix/webrev/
> > > > 
> > > > Feel free to use it.
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > > Note that the assertion triggers on a fastdebug build with
> > > > > GCC 8.2 and
> > > > > running :jdk_jfr test group for me (18 tests fail). Failures
> > > > > go away
> > > > > with -fno-strict-overflow.
> > > > > 
> > > > > > I will file it to JBS if it is accepted.
> > > > > 
> > > > > Please file a bug.
> > > > > 
> > > > > Thanks,
> > > > > Severin
> > > > > 
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > Yasumasa
> > > > > > 
> > > > > > [1] 
> > > > > > https://mail.openjdk.java.net/pipermail/hotspot-jfr-dev/2018-May/000047.html
> > > > > > [2] 
> > > > > > http://hg.openjdk.java.net/jdk7u/jdk7u/jdk/rev/35dcc0db31dc