RFR: 8186502: Assert when range testing G1RefProcDrainInterval on 64-bit systems

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8186502: Assert when range testing G1RefProcDrainInterval on 64-bit systems

Leo Korinth
Hi,

Fix range checking when using -XX:G1RefProcDrainInterval=n

On 64 bit machines, when issuing extremly large integer arguments
to -XX:G1RefProcDrainInterval=n, a 32 bit integer will overflow and
cause a crash.

First I have improved the tests by adding the flag
-XX:+ExplicitGCInvokesConcurrent when testing the flag
-XX:G1RefProcDrainInterval=n to trigger the fault in our
test suit.

Then I have fixed the fault by limiting the arguments on 64 bit
systems (using an int type instead of intx type and a range up to
only INT_MAX for argument parsing).

Bug:
https://bugs.openjdk.java.net/browse/JDK-8186502

Webrev:
http://cr.openjdk.java.net/~lkorinth/8186502/00/

Testing:
- JPRT

Thanks,
Leo
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186502: Assert when range testing G1RefProcDrainInterval on 64-bit systems

sangheon.kim@oracle.com
Hi Leo,

On 11/03/2017 02:24 AM, Leo Korinth wrote:

> Hi,
>
> Fix range checking when using -XX:G1RefProcDrainInterval=n
>
> On 64 bit machines, when issuing extremly large integer arguments
> to -XX:G1RefProcDrainInterval=n, a 32 bit integer will overflow and
> cause a crash.
>
> First I have improved the tests by adding the flag
> -XX:+ExplicitGCInvokesConcurrent when testing the flag
> -XX:G1RefProcDrainInterval=n to trigger the fault in our
> test suit.
>
> Then I have fixed the fault by limiting the arguments on 64 bit
> systems (using an int type instead of intx type and a range up to
> only INT_MAX for argument parsing).
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8186502
>
> Webrev:
> http://cr.openjdk.java.net/~lkorinth/8186502/00/
Looks good, thank you for fixing for this.
Just minor nit: you can update copyright year at g1_globals.hpp.

BTW, did you run TestOptionsWithRanges.java manually?
Probably this test is still excluded by default when you run JPRT.

Thanks,
Sangheon


>
> Testing:
> - JPRT
>
> Thanks,
> Leo

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186502: Assert when range testing G1RefProcDrainInterval on 64-bit systems

Leo Korinth
Hi Sangheon.

On 04/11/17 00:36, sangheon.kim wrote:
 > Hi Leo,
 >
 > On 11/03/2017 02:24 AM, Leo Korinth wrote:
 >> Hi,
 >>
 >> Fix range checking when using -XX:G1RefProcDrainInterval=n
 >>
 >> On 64 bit machines, when issuing extremly large integer arguments
 >> to -XX:G1RefProcDrainInterval=n, a 32 bit integer will overflow and
 >> cause a crash.
 >>
 >> First I have improved the tests by adding the flag
 >> -XX:+ExplicitGCInvokesConcurrent when testing the flag
 >> -XX:G1RefProcDrainInterval=n to trigger the fault in our
 >> test suit.
 >>
 >> Then I have fixed the fault by limiting the arguments on 64 bit
 >> systems (using an int type instead of intx type and a range up to
 >> only INT_MAX for argument parsing).
 >>
 >> Bug:
 >> https://bugs.openjdk.java.net/browse/JDK-8186502
 >>
 >> Webrev:
 >> http://cr.openjdk.java.net/~lkorinth/8186502/00/
 > Looks good, thank you for fixing for this.
 > Just minor nit: you can update copyright year at g1_globals.hpp.

Thanks, copyright year fixed in:
http://cr.openjdk.java.net/~lkorinth/8186502/01/

 > BTW, did you run TestOptionsWithRanges.java manually?
 > Probably this test is still excluded by default when you run JPRT.

Yes, first I updated the test so that it triggered the bug (and ran the
test manually to verify that it indeed triggered the bug). Then I fixed
the bug. After that I ran the test again (and the test passed).

I am unsure if the test is enabled when you run JPRT though, but if it
is, it will pass.

/Leo

 > Thanks,
 > Sangheon
 >
 >
 >>
 >> Testing:
 >> - JPRT
 >>
 >> Thanks,
 >> Leo
 >
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186502: Assert when range testing G1RefProcDrainInterval on 64-bit systems

Stefan Johansson
Thanks for fixing this Leo,

Change looks good and I can do the push for you,
StefanJ

On 2017-11-06 10:55, Leo Korinth wrote:

> Hi Sangheon.
>
> On 04/11/17 00:36, sangheon.kim wrote:
> > Hi Leo,
> >
> > On 11/03/2017 02:24 AM, Leo Korinth wrote:
> >> Hi,
> >>
> >> Fix range checking when using -XX:G1RefProcDrainInterval=n
> >>
> >> On 64 bit machines, when issuing extremly large integer arguments
> >> to -XX:G1RefProcDrainInterval=n, a 32 bit integer will overflow and
> >> cause a crash.
> >>
> >> First I have improved the tests by adding the flag
> >> -XX:+ExplicitGCInvokesConcurrent when testing the flag
> >> -XX:G1RefProcDrainInterval=n to trigger the fault in our
> >> test suit.
> >>
> >> Then I have fixed the fault by limiting the arguments on 64 bit
> >> systems (using an int type instead of intx type and a range up to
> >> only INT_MAX for argument parsing).
> >>
> >> Bug:
> >> https://bugs.openjdk.java.net/browse/JDK-8186502
> >>
> >> Webrev:
> >> http://cr.openjdk.java.net/~lkorinth/8186502/00/
> > Looks good, thank you for fixing for this.
> > Just minor nit: you can update copyright year at g1_globals.hpp.
>
> Thanks, copyright year fixed in:
> http://cr.openjdk.java.net/~lkorinth/8186502/01/
>
> > BTW, did you run TestOptionsWithRanges.java manually?
> > Probably this test is still excluded by default when you run JPRT.
>
> Yes, first I updated the test so that it triggered the bug (and ran
> the test manually to verify that it indeed triggered the bug). Then I
> fixed the bug. After that I ran the test again (and the test passed).
>
> I am unsure if the test is enabled when you run JPRT though, but if it
> is, it will pass.
>
> /Leo
>
> > Thanks,
> > Sangheon
> >
> >
> >>
> >> Testing:
> >> - JPRT
> >>
> >> Thanks,
> >> Leo
> >

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186502: Assert when range testing G1RefProcDrainInterval on 64-bit systems

sangheon.kim@oracle.com
In reply to this post by Leo Korinth


On 11/06/2017 01:55 AM, Leo Korinth wrote:

> Hi Sangheon.
>
> On 04/11/17 00:36, sangheon.kim wrote:
> > Hi Leo,
> >
> > On 11/03/2017 02:24 AM, Leo Korinth wrote:
> >> Hi,
> >>
> >> Fix range checking when using -XX:G1RefProcDrainInterval=n
> >>
> >> On 64 bit machines, when issuing extremly large integer arguments
> >> to -XX:G1RefProcDrainInterval=n, a 32 bit integer will overflow and
> >> cause a crash.
> >>
> >> First I have improved the tests by adding the flag
> >> -XX:+ExplicitGCInvokesConcurrent when testing the flag
> >> -XX:G1RefProcDrainInterval=n to trigger the fault in our
> >> test suit.
> >>
> >> Then I have fixed the fault by limiting the arguments on 64 bit
> >> systems (using an int type instead of intx type and a range up to
> >> only INT_MAX for argument parsing).
> >>
> >> Bug:
> >> https://bugs.openjdk.java.net/browse/JDK-8186502
> >>
> >> Webrev:
> >> http://cr.openjdk.java.net/~lkorinth/8186502/00/
> > Looks good, thank you for fixing for this.
> > Just minor nit: you can update copyright year at g1_globals.hpp.
>
> Thanks, copyright year fixed in:
> http://cr.openjdk.java.net/~lkorinth/8186502/01/
Looks good.

>
> > BTW, did you run TestOptionsWithRanges.java manually?
> > Probably this test is still excluded by default when you run JPRT.
>
> Yes, first I updated the test so that it triggered the bug (and ran
> the test manually to verify that it indeed triggered the bug). Then I
> fixed the bug. After that I ran the test again (and the test passed).
>
> I am unsure if the test is enabled when you run JPRT though, but if it
> is, it will pass.
I agree with you that the test will pass.

FYI, test/hotspot/jtreg/TEST.groups : line 153
-runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java \
: means this test will be excluded from 'hotspot_tier1_runtime' which is
one of lists run via JPRT.
So to run via JPRT, you should remove that line.

Thanks,
Sangheon


>
> /Leo
>
> > Thanks,
> > Sangheon
> >
> >
> >>
> >> Testing:
> >> - JPRT
> >>
> >> Thanks,
> >> Leo
> >