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

Wed Nov 23 07:29:34 UTC 2016

Hi David,

On Wed, Nov 23, 2016 at 3:08 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> 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.
>>
>
> Sorry but how is NMTUtil::flag_to_index anything but a mapping from 0 to
> mt_number_of_types ??? MEMFLAGS aka MemoryType is an enum that spans zero
> to mt_number_of_types! We're validating the post condition for the method.
>

Sorry, this may be clear for you but was not clear for me. There is no
comment at NMTUtil::flag_to_index explaining the range of the output value,
and the callers are spread over multiple files. The fact that the original
author found it necessary to add this conversion function and to mask the
upper bits out suggested to me that the returned index is something
different than MEMFLAGS, or may be different in the future. Otherwise he
could have just used the MEMFLAGS enum itself as index.

Note that I still do not understand the & 0xff part. Once MEMFLAG values
surpass 0xff, what is supposed to happen?

>
> As I said on Oct 19, and which Chris indicated he agreed with, it makes no
> sense to me to put the same assert in all the callers of flag_to_index
> instead of doing the validation inside flag_to_index.

I understand you. I understood Max' comment otherwise. Faced with two
opinions, and as a outsider having no way to weight them against each
other, I have to choose one.

> Whether you choose to validate the flag or the index I don't care.

Then we could have just gone with my very first webrev and I could have
saved work. In your very first answer to my original webrev you wrote: "I'm
a little perplexed with asserting that something of MEMFLAGS type must be
an actual MEMFLAGS value". I understood this as objection.

I am fine with either solution. I am also fine with someone else fixing
this issue. My only preference would be to some form of assert in this
code, because I spent quite some time to find the memory overwriter this
caused.

If we were in a decent language the bug would have been impossible to begin
> with.
>
> David
> -----
>
>
Thanks, Thomas

> On 22/11/2016 1:21 AM, Thomas Stüfe wrote:
>
>> 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-shoul
>> d-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 <mailto:thomas.stuefe at gmail.com>> wrote:
>>
>> Hi Chris,
>>
>> On Thu, Nov 17, 2016 at 8:54 PM, Chris Plummer
>> <chris.plummer at oracle.com <mailto: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>
>> <mailto: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-shoul
>> d-check_MEMFLAGS/webrev.00/webrev/
>>
>>
>> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-shou
> ld-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>
>
>> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-shou
>> ld-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>>
>> <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:
>>
>> 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>>>
>> <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 <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>>>
>>
>>
>> <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_>>>
>
>>
>>
>> <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
>>
>>
>>
>>
>>
>>
>>
>>
>>