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

[Thomas Stüfe]

On Wed, Oct 19, 2016 at 3:21 AM, David Holmes <david.holmes at oracle.com>
wrote:

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


Max wrote: " I think the decision on whether to access a slot should occur
as close to memory accessing code as possible." and proceeded to suggest
fixing VirtualMemorySnapshot::by_type() as well.


> 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.
>
>
I disagree. Imho it makes sense to either check the Memflags enumeration
input argument in flag_to_index() or the returned index before consumption.
In both cases one knows the valid value range. Strictly speaking checking
the index in flag_to_index() cannot be done because it is a faceless int
type whose valid values are not yet known.

It is all academical and mostly a matter of taste.


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

The fix is trivial and I will try to get fc extension for this (now that
Goetz explained to me how to do this :). It seems this is done for many
other non-bug issues as well.

..Thomas


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