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

[Thomas Stüfe]

Hi David, Max,

I changed the asserts according to Max' suggestion. Instead of checking
inside flag_to_index, now I check before callers of this function use this
value to access memory.

http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html

As David correctly writes, this is technically not a bug, so I guess this
will have to wait until java 10.

Kind Regards, Thomas



On Fri, Oct 14, 2016 at 12:57 AM, David Holmes <david.holmes at oracle.com>
wrote:

> On 13/10/2016 10:53 PM, Thomas Stüfe wrote:
>
>> Hi David,
>>
>> On Thu, Oct 13, 2016 at 12:08 PM, David Holmes <david.holmes at oracle.com
>> <mailto: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
>>         <https://bugs.openjdk.java.net/browse/JDK-8167650>
>>         webrev:
>>         http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-shoul
>> d-check_
>>         <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-shou
>> ld-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.
>>
>
> Ah I see. Lack of default initialization can be annoying :)
>
> 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.
>>
>
> So I just realized that here:
>
>   62   // Map memory type to human readable name
>   63   static const char* flag_to_name(MEMFLAGS flag) {
>   64     assert(flag >= 0 && flag < mt_number_of_types, "Invalid flag
> value %d.", (int)flag);
>   65     return _memory_type_names[flag_to_index(flag)];
>   66   }
>
> we call flag_to_index, so the assert is redundant as it is already in
> flag_to_index. Then presumably we change flag_to_index to something like
> this:
>
>      static inline int flag_to_index(MEMFLAGS flag) {
>        int index = (flag & 0xff);
>        assert(index >= 0 && index < mt_number_of_types, "Invalid flag
> value %d.", (int)flag);
>        return index;
>      }
>
> so we're validating the index rather than the flag.
>
> Cheers,
> David
>
>
>
>>
>>     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
>>
>>
>>