RFR(xxs): 8167650: NMT should check for invalid MEMFLAGS.
Thomas Stüfe
thomas.stuefe at gmail.com
Mon Nov 21 15:17:48 UTC 2016
Hi Chris,
On Thu, Nov 17, 2016 at 8:54 PM, Chris Plummer <chris.plummer at oracle.com>
wrote:
> On 10/18/16 11:49 PM, David Holmes wrote:
>
>>
>>
>> On 19/10/2016 3:17 PM, Thomas Stüfe wrote:
>>
>>>
>>>
>>> On Wed, Oct 19, 2016 at 3:21 AM, David Holmes <david.holmes at oracle.com
>>> <mailto: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.
>>>
>>
>> I did not interpret that comment that way, and was puzzled by the
>> reference to by_type.
>>
>>>
>>> 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.
>>>
>>
>> The index has to fall in the range 0 <= index <= mt_number_of_types, and
>> I was suggesting that it makes more sense to verify this once in
>> flag_to_index() than in all the callers of flag_to_index.
>>
> Hi Thomas,
>
> Just catching up on this thread. This is the same conclusion I came to. I
> don't understand what you mean by "it is a faceless int type whose valid
> values are not yet known".
>
>
Thank you for looking into this!
"it is a faceless int type whose valid values are not yet known" was maybe
expressed sloppily:
My first patch checked the input enum "flag" argument inside
NMTUtil::flag_to_index() for correct enum values : http://cr.openjdk.java
.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.00/webrev/
David did not like that, he thought it strange to check an enum for correct
enum values. I can see his point. He would have preferred instead an assert
inside NMTUtil::flag_to_index() which before returning checks the
to-be-returned index for 0>=i>=mt_number_of_types. I did not like that
because strictly speaking inside of NMTUtil::flag_to_index() it is not
known that the returned integer will be used by the caller as an index into
an array of mt_number_of_types length.
It is all extreme nitpicking and in the end amounts to the same :)
The current patch does neither, but follows Max' suggestion of checking the
index right before it is consumed to access an array. I think this is a
good solution and very clear.
> BTW, I'll sponsor this fix for you once it is finalized. Please update the
> copyright dates first.
>
Thank you. I'll update the webrev and repost.
>
> thanks,
>
> Chris
>
>
Kind Regards, Thomas
>
>> David
>>
>> 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
>>> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-shou
>>> ld-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>
>>> <mailto: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>>
>>> <mailto: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>>
>>>
>>> <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_>>
>>>
>>>
>>> <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
>>>
>>>
>>>
>>>
>>>
>
>
More information about the hotspot-runtime-dev
mailing list