RFR(xxs): 8167650: NMT should check for invalid MEMFLAGS.
Zhengyu Gu
zgu at redhat.com
Wed Nov 23 13:58:48 UTC 2016
Sorry for participating late, I did not expect to be a lengthy discussion.
On 11/23/2016 06:20 AM, David Holmes wrote:
> Hi Thomas,
>
> On 23/11/2016 5:29 PM, Thomas Stüfe wrote:
>> Hi David,
>>
>> On Wed, Nov 23, 2016 at 3:08 AM, David Holmes <david.holmes at oracle.com
>> <mailto: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.
>
> Can't answer that - perhaps just for cleanliness of the type system?
> Perhaps he thought the enum was strongly-typed? Or perhaps it was to
> hide the actual mapping function to allow for a different scheme.
Yes, I was assuming the type system should ensure correct MEMFLAGS value here.
>
> If anything the & 0xff suggests that the input might be something
> other than a MEMFLAGS value - despite it being typed as MEMFLAGS -
> that needs to be brought back into range.
>
>> Note that I still do not understand the & 0xff part. Once MEMFLAG values
>> surpass 0xff, what is supposed to happen?
>
> I don't think there is any expectation that we will ever go that high:
>
> enum MemoryType {
> // Memory type by sub systems. It occupies lower byte.
>
> though why this was restricted to be a byte-size value I can't say. I
> also don't know why we then typedef MemoryType to MEMFLAGS.
NMTUtil::flag_to_index() converts flag to byte size for encoding the value into malloc header (services/mallocTracker.hpp).
At the time of implementation, we don't have some types, such as u1, etc. maybe change the return type to u1 (maybe refactor MallocHeader)
to make the code more explicit.
>
>> 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.
>>
>
> Fair enough, but Chris made it 2-1. :)
I am also with David and Chris. mt_number_of_types just for working around imperfect of enum type system,
I would rather it not to leak everywhere.
Thanks,
-Zhengyu
>
>> 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.
>
> Yes but I've now reached a point where I'd accept that if it is the
> only way to have the assertion inside the function. When I made that
> objection I did not expect this to take the path it did. Sorry.
>
>> 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.
>
> My preference, obviously, is an assert inside flag_to_index, on the
> value of index to be returned.
>
> I did not expect this to become so prolonged, and I'm sorry about
> that, but I really object to the callers of the function validating
> its output.
>
> Thanks,
> David
>
>> 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-should-check_MEMFLAGS/webrev.02/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 <mailto:thomas.stuefe at gmail.com>
>> <mailto: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>
>> <mailto: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>>
>> <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 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/
>> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.00/webrev/>
>>
>>
>> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.00/webrev/
>> <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-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>
>>
>>
>> <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>>
>>
>> <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>
>>
>>
>> <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>>
>> <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:
>>
>> 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>>>>
>> <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
>> <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>>>>
>>
>>
>> <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_>>>>
>>
>>
>>
>>
>> <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
>>
>>
>>
>>
>>
>>
>>
>>
>>
More information about the hotspot-runtime-dev
mailing list