Possibility of integer overflow in JfrThreadSampler::run()

[Yasumasa Suenaga]

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.


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
>