Request for review: 6761744 Hotspot crashes if process size limit is exceeded

Mon Apr 22 07:58:27 UTC 2013

Hi Tao,

On 4/19/13 7:27 PM, Tao Mao wrote:
> 1. check overflow routine is wrapped up.

I would prefer that the method 
CollectedHeap::add_to_total_reserved_and_check_overflow() did not return 
a bool. Since you do vm_exit_during_initialization() when you overflow 
there is no need to return different results. This would also simplify 
the code where you use it. No need to use if statements.

Also, I think you should remove the naming references to 
"total_reserved" and "reserved". This method is general enough to just 
add two size_t arguments together and check for overflow. Especially if 
it would take the message as a parameter. Instead of returning a bool 
you can return the result of the addition if there was no overflow.

I'm not sure we need round_up_total_reserved_and_check_overflow() since 
it is only being used in one place.

>
> 2. The reduction of code duplication is adopted.

Good.

>
> 3. Develop a way to intake 0~multiple external vm options in jtreg main().

Yes, this looks like it should work. However, based on John 
Cuthbertson's discussion with Leonid I think you need to do the same 
thing for test.java.opts too.

Bengt

> Hope it helps others implement similar functionality.
> webrev:
> http://cr.openjdk.java.net/~tamao/6761744/webrev.02/
>
> test:
> JTREG: passed.
> jtreg -jdk:/home/tamao/jdk1.7.0_04-i586 -vmoptions:"-tamao 
> <GC_OPTION>" 
> /home/tamao/src/6761744CrashIfProcessSizeLimitExceeded_hsx24/test/gc/init/TestHandleExceedingProcessSizeLimitIn32BitBuilds.java
>
> where GC_OPTION rotates in -XX:+UseParallelGC -XX:+UseG1GC 
> -XX:+UseSerialGC -XX:+UseParNewGC -XX:+UseConcMarkSweepGC
>
>
> Thanks.
> Tao
>
> On 4/15/13 1:37 AM, Bengt Rutisson wrote:
>>
>> Hi Tao,
>>
>> I agree with John. The changes look fine and I'm ok with them. But it 
>> would look nicer to add the 
>> CollectedHeap::add_and_verify_no_overflow() method.
>>
>> Also, regarding the test, a couple of minor things.
>>
>> First, with the new naming convention I think the test should be 
>> called something like:
>>
>> test/gc/init/TestHandleExceedingProcessSizeLimitOn32BitSystems.java
>>
>> Then I think you can reduce the code duplication a bit. You create 
>> the same ProcessBuilder and OutputAnalyzer in both 32 and 64 bit 
>> cases. So maybe it could look like this instead:
>>
>> public class TestHandleExceedingProcessSizeLimitOn32BitSystems {
>>   public static void main(String args[]) throws Exception {
>>     ProcessBuilder pb =
>> ProcessTools.createJavaProcessBuilder(System.getProperty("test.vm.opts"),
>> "-Xmx3072m",
>> "-XX:MaxPermSize=1024m",
>> "-version");
>>     OutputAnalyzer output = new OutputAnalyzer(pb.start());
>>
>>     String dataModel = System.getProperty("sun.arch.data.model");
>>     if (dataModel.equals("32")) {
>>       output.shouldContain("The size of the object heap + perm gen 
>> exceeds the maximum representable size");
>>       if (output.getExitValue() == 0) {
>>         throw new RuntimeException("Not expected to get exit value 0");
>>       }
>>     } else if (dataModel.equals("64")) {
>>       output.shouldHaveExitValue(0);
>>     }
>>   }
>> }
>>
>> There is also a small bug in the test. If you run JTreg without 
>> passing any vmoptions the test will fail. The reason is that 
>> System.getProperty("test.vm.opts") will evaluate to the empty string. 
>> This will be passed as the first argument to the Java process, which 
>> will assume that this is the class name. So, it fails to start 
>> because it can't load the class <empty string>.
>>
>> To reproduce the problem use this command line:
>>
>> java  -jar <path_to_jtreg>/lib/jtreg.jar 
>> test/gc/6761744/TestHandleExceedingProcessSizeLimitOn32BitSystems.java
>>
>> I guess the fix is to check that System.getProperty("test.vm.opts") 
>> is not empty before passing it on to createJavaProcessBuilder().
>>
>> Thanks,
>> Bengt
>>
>> On 4/13/13 3:15 AM, John Cuthbertson wrote:
>>> Hi Tao,
>>>
>>> This looks OK to me.
>>>
>>> I can see what Thomas is saying though. All of the checks involve 
>>> something like:
>>>
>>> total += some_value;
>>> if (total < some_value) {
>>>   // We must have overflowed
>>>   vm_exit(...);
>>> }
>>>
>>> So a function in CollectedHeap (the base class of SharedHeap and 
>>> ParallelScavengeHeap) might make sense. For example:
>>>
>>> total_reserved = 0;
>>> total_reserved = add_and_verify_no_overflow(total_reserved, 
>>> max_heap_size);
>>> total_reserved = add_and_verify_no_overflow(total_reserved, 
>>> max_perm_size);
>>>
>>> Where the function is:
>>>
>>> size_t add_and_verify_no_overflow(size_t x, size_t y) {
>>>   const char* msg = "...";
>>>   x += y;
>>>   if (x < y) {
>>>     vm_exit(msg);
>>>   }
>>>   return x;
>>> }
>>>
>>> But I can live with your current changes.
>>>
>>> JohnC
>>>
>>> On 4/10/2013 9:52 AM, Tao Mao wrote:
>>>> Hi Bengt,
>>>>
>>>> Thank you for reviewing. A new webrev is updated.
>>>> http://cr.openjdk.java.net/~tamao/6761744/webrev.01/
>>>>
>>>> Cheers,
>>>> Tao
>>>>
>>>> On 4/10/13 1:54 AM, Bengt Rutisson wrote:
>>>>>
>>>>> Hi Tao,
>>>>>
>>>>> This change looks good. Thanks for adding the JTReg test, it looks 
>>>>> good!
>>>>>
>>>>> One minor nit:
>>>>>
>>>>> In src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp I would 
>>>>> suggest to store "(size_t) align_size_up(pgs->max_size(), 
>>>>> HeapRegion::GrainBytes)" in a local variable rather than 
>>>>> duplicating the calculation.
>>>>>
>>>>> Thanks,
>>>>> Bengt
>>>>>
>>>>> On 4/8/13 7:22 AM, Tao Mao wrote:
>>>>>> 6761744 Hotspot crashes if process size limit is exceeded
>>>>>> https://jbs.oracle.com/bugs/browse/JDK-6761744
>>>>>>
>>>>>> webrev:
>>>>>> http://cr.openjdk.java.net/~tamao/6761744/webrev.00/
>>>>>>
>>>>>> changeset:
>>>>>> The fix only needs to go to hsx24 since there's no perm gen in 
>>>>>> hotspot-25. Thus, the webrev is based on hsx24 repo.
>>>>>>
>>>>>> It provides for 32-bit builds a preventive check of the size of 
>>>>>> "the object heap + perm gen" before reserving VM memory. The 
>>>>>> total size should not exceed 4096MB (i.e. 4GB) for 32-bit builds; 
>>>>>> otherwise, the total doesn't make sense and, what's worse, 
>>>>>> overflow occurs. It will consequentially trigger anther error of 
>>>>>> memory access violation, which was not protected.
>>>>>>
>>>>>> jtreg testing java code is also written, checking both 32-bit and 
>>>>>> (trivially) 64-bit builds.
>>>>>>
>>>>>> testing:
>>>>>> check jtreg tests with flags -XX:+UseParallelGC, -XX:+UseG1GC, 
>>>>>> -XX:+UseParNewGC, -XX:+UseConcMarkSweepGC, -XX:+UseSerialGC and 
>>>>>> builds of 32-bit and 64-bit. All passed.
>>>>>>
>>>>>> Needs JPRT test when pushing.
>>>>>
>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130422/98f65f34/attachment.htm>