RFR: 8257228: G1: SIGFPE in G1ConcurrentRefine::create(int*) due to buffers_to_cards overflow

[Kim Barrett]

On Sat, 28 Nov 2020 08:26:57 GMT, Jie Fu <jiefu at openjdk.org> wrote:

> Hi all,
> 
> SIGFPE was observed by running:
> java -XX:G1ConcRefinementThresholdStep=16G -XX:G1UpdateBufferSize=1G -version
> 
> The reason is that buffers_to_cards [1] returns 0 for 'step' due to overflow.
> It would be better to add overflow check logic is it.
> 
> Testing:
>   - tier1 on Linux/x64
> 
> Thanks.
> Best regards,
> Jie
> 
> [1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp#L235

This is only a problem when using values for some command line options that
are far out of the "normal" range. That's just a general problem; we have
far too many options, and some of them interact in interesting ways, so that
it's pretty much impossible to fully test or check for problem cases. And we
don't want to set artificially low limit values for individual options
because it's hard to know what some application might find useful. In this
case, it does look like we can reasonably do more checking though.

src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 190:

> 188:   size_t res = value * G1UpdateBufferSize;
> 189: 
> 190:   if (res < value || res < G1UpdateBufferSize) { // Check overflow

That's not a correct multiplication overflow check.

src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 193:

> 191:     vm_exit_during_initialization("buffers_to_cards overflow!");
> 192:   }
> 193:   return res;

buffers_to_cards is only used with command line options as arguments. So
it's not inappropriate to use vm_exit_during_initialization, even though
this function isn't obviously about initialization. Maybe it should be
changed to

// Convert configuration values in units of buffers to number of cards.
static size_t configuration_buffers_to_cards(size_t value, const char* value_name) {
  ... use value_name in the error message
}

An alternative would be to move the checking to constraint functions for
the command line options. That puts the checking far from the code where
it matters though, but maybe a comment in the conversion function referring
to the option constraints would be sufficient. This might provide better
reporting?

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

Changes requested by kbarrett (Reviewer).

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