RFR(xxs): 8167650: NMT should check for invalid MEMFLAGS.
Thomas Stüfe
thomas.stuefe at gmail.com
Mon Nov 21 15:21:17 UTC 2016
Hi all,
this small issue got fc extension, so lets wrap this up. Thanks to all for
reviewing!
New webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.02/webrev/
No changes to the last one but the updated copyrights Chris did ask for.
Solution still follows Max' suggestion of checking the index right before
consumption.
Thanks, and Kind Regards, Thomas
On Mon, Nov 21, 2016 at 4:17 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:
> 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.co
>>>> m
>>>> <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