RFR(xxs): 8167650: NMT should check for invalid MEMFLAGS.
Chris Plummer
chris.plummer at oracle.com
Thu Nov 17 19:54:18 UTC 2016
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".
BTW, I'll sponsor this fix for you once it is finalized. Please update
the copyright dates first.
thanks,
Chris
>
> David
>
>> It is all academical and mostly a matter of taste.
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
>> <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.
>>
>>
>> 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