RFR (S) 8211926: Catastrophic size_t underflow in BitMap::*_large methods

[Thomas Stüfe]

On Sat, Nov 10, 2018 at 2:10 PM Aleksey Shipilev <shade at redhat.com> wrote:
>
> On 11/10/2018 01:58 PM, Thomas Stüfe wrote:
> > http://cr.openjdk.java.net/~shade/8211926/webrev.05/src/hotspot/share/utilities/bitMap.inline.hpp.udiff.html
> >
> > +  assert(beg <= end, "underflow");
> >
> > I would feel slightly safer with a runtime check for product as well.
>
> I think current patch never enters the bad path on any combination of incoming arguments, so we can
> go with just the debug sanity check. And even without the assert, you would notice the breakage in
> product builds as well, when it stomps over most of VM memory, reaches the end of committed memory,
> and SEGVs :) Asserts only make it evident where it had gone downhill.
>

Okay, yes, chance for such an overwrite to go unnoticed are slim..

> > http://cr.openjdk.java.net/~shade/8211926/webrev.05/test/hotspot/gtest/utilities/test_bitMap_large.cpp.html
> >
> > The tests favor the left side (lower space) of the bitmap, since l
> > never grows beyond FUZZ_WINDOW. I wonder whether testing the right
> > side (small ranges near the bitmap end) would be useful. Maybe with a
> > gliding window, or just repeat the tests anchored at the right instead
> > of the left side.
>
> From gray-boxing perspective, it seems to me that what matters is the position of left/right
> boundaries with regards to word boundaries. This keeps testing time at bay as well. I would be in
> favor of adding the proper stress tests for internal code that would spend tens of minutes going
> through every possible combination, but not for this bug.

In that case you could have probably done with some less variants of
size_class too. But as I said I am fine with the original change.

Cheers, Thomas


>
> -Aleksey
>