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

[David Holmes]

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-should-check_
>         <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-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
>
>