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

[Thomas Stüfe]

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.
>>
>
> 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.
>
> 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
>>
>>