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

[David Holmes]

On 18/10/2016 3:39 PM, Thomas Stüfe wrote:
> 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.

I don't see where Max suggested that?? It doesn't make sense to me to 
have all the callers of flag_to_index check what it returned instead of 
doing it inside flag_to_index.

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

Yes, afraid so.

Thanks,
David

> Kind Regards, Thomas
>
>
>
> On Fri, Oct 14, 2016 at 12:57 AM, David Holmes <david.holmes at oracle.com
> <mailto: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>
>         <mailto: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>
>                 <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_>
>
>         <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
>
>
>