RFR: 8266787: Potential overflow of pointer arithmetic in G1ArchiveAllocator

[Kim Barrett]

On Mon, 10 May 2021 09:36:03 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1Allocator.cpp line 477:
>> 
>>> 475:     if (_max != _allocation_region->end()) {
>>> 476:       // Shift to the next chunk
>>> 477:       old_top = _bottom = _max;
>> 
>> Why is this line being moved?  It seems otherwise unrelated to the CR change.  With this change, if `alloc_new_region` fails then `_bottom` is no longer updated; is that okay?  I spent a little time investigating, but haven't yet found anything convincing either way.
>
>> Why is this line being moved?
> 
> Two reasons:
> 
> 1. To make it more symmetric in updating `old_top`, `_bottom` and `_max`.
> 
> 
> if (_max != _allocation_region->end()) {
>   ... // updating the triplet in one way
> } else {
>   ... // updating them in another way
> }
> 
> 
> 2. In general, the range of `[_bottom, _top]` marks the allocatable space, and `_top` fits in the middle. In the original code, when `alloc_new_region` fails, `_bottom == _max`, which leaves zero allocatable space. With this PR, `[_bottom, _top]` is still a valid allocatable space, but full.
> 
>> is that okay?
> 
> When `alloc_new_region` fails, `G1ArchiveAllocator::archive_mem_allocate` returns NULL. Callers seem to expect this method to *not* return NULL in the normal case; they either terminate the VM (`HeapShared::archive_heap_object`) or directly treat the return value as non-null (`HeapShared::copy_roots()` and `G1ArchiveAllocator::complete_archive`). Therefore, both the existing code and the PR are fine.
> 
> BTW, it seems that `_bottom` is only used for verification purpose; maybe remove it or make this more explicit.

Thanks for the explanation.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3936