RFR(xxs): 8167650: NMT should check for invalid MEMFLAGS.
David Holmes
david.holmes at oracle.com
Wed Oct 19 06:49:35 UTC 2016
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.
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