RFR(xxs): 8167650: NMT should check for invalid MEMFLAGS.
Zhengyu Gu
zgu at redhat.com
Wed Nov 23 14:40:25 UTC 2016
Looks good to me.
Thanks,
-Zhengyu
On 11/23/2016 09:15 AM, Thomas Stüfe wrote:
> Hi Zhengyu,
>
> thanks for looking at this. See my answer to David, I did the last rev
> as you guys requested. See:
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.03/webrev/
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.03/webrev/>
>
> Kind Regards, Thomas
>
> On Wed, Nov 23, 2016 at 2:58 PM, Zhengyu Gu <zgu at redhat.com
> <mailto:zgu at redhat.com>> wrote:
>
> 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>
> <mailto: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/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.02/webrev/>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.02/webrev/
> <http://cr.openjdk.java.net/%7Estuefe/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>>
> <mailto: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>>
> <mailto: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>>>
> <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 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/%7Estuefe/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/%7Estuefe/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/%7Estuefe/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/%7Estuefe/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/%7Estuefe/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/%7Estuefe/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/%7Estuefe/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/%7Estuefe/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/%7Estuefe/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/%7Estuefe/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/%7Estuefe/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/%7Estuefe/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>>>>
> <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:
>
> 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>>>>>
> <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
> <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>>>>>
>
>
> <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/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>
>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>>
>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>>>
>
>
>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>>
>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
>
>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> <http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> <http://cr.openjdk.java.net/%7Estuefe/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