RFR(xxs): 8167650: NMT should check for invalid MEMFLAGS.

[Max Ockner]

Hi Thomas,

(Comments below. )

Max

On 10/13/2016 8:53 AM, Thomas Stüfe wrote:
> Hi David,
>
> On Thu, Oct 13, 2016 at 12:08 PM, David Holmes <david.holmes at oracle.com>
> wrote:
>
>> Hi Thomas,
>>
>> On 13/10/2016 3:49 PM, Thomas Stüfe wrote:
>>
>>> Hi all,
>>>
>>> may I have plase a review for this tiny change? It just adds some assert
>>> to
>>> NMT.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8167650
>>> webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-shoul
>>> d-check_
>>> MEMFLAGS/webrev.00/webrev/
>>>
>>> We had an ugly memory overwrite caused by this - ultimately our fault,
>>> because we fed an invalid memory flag to NMT - but it was difficult to
>>> find. An assert would have saved some time.
It is alarming that a bug in NMT could cause a problem in memory 
management, since it was my understanding that memory allocation 
decisions are not informed by the NMT state.
>> I'm a little perplexed with asserting that something of MEMFLAGS type must
>> be an actual MEMFLAGS value - it implies the caller is coercing plain int
>> to MEMFLAGS, and I don't have much sympathy if they mess that up. Can't
>> help wondering if there is some clever C++ trick to flag bad conversions at
>> compile-time?
>>
>>
> The error was caused by an uninitialized variable of type MEMFLAGS. This
> was our fault, we have heavily modified allocation.hpp and introduced an
> error then merging changes from upstream. Due to a merging error this lead
> to a case where Arena::_flags was not initialized and contained a very
> large value.
>
> I admit it looks funny. If it bothers you, I could instead check the
> returned index to be in the range for the size of the _malloc array in
> MallocMemorySnapshot::by_type(). Technically, it would mean the same.
>
>
>
>> The function that takes the index should validate the index, so that is
>> fine.
I agree with this. I think the decision on whether to access a slot 
should occur as close to memory accessing code as possible.

Another note - If you are validating the index immediately before 
consumption, then it looks like there is a second place where you need 
to add an assert. In virtualMemoryTracker.hpp we have:

   inline VirtualMemory* by_type(MEMFLAGS flag) {
     int index = NMTUtil::flag_to_index(flag);
     return &_virtual_memory[index];
   }

>> Which one were you actually passing the bad value to? :)
>>
>> This isn't a strong objection just musing if we can do better. And as the
>> hs repos are still closed, and likely to remain so till early next week, we
>> have some slack time :)
> :) Sure.
>
> Kind Regards, Thomas
>
>
>> Cheers,
>> David
>>
>> Thank you!
>>> Thomas
>>>
>>>