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

[John Cuthbertson]

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/20130412/bcac4a6f/attachment.htm>