JDK-8219459 : oopDesc::is_valid() is broken
  • Type: Bug
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 13
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2019-02-20
  • Updated: 2019-08-15
  • Resolved: 2019-05-14
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 13
13 b21Fixed
Related Reports
Relates :  
JDK-8210754 - print_location is not reliable enough (printing register info)
Description
Somehow it doesn't recognize my oop during debugging when calling hsfind() which calls oopDesc::oop_or_null and ends up calling G1ContinguousSpace::block_start which gets a bad address for klass and crashes.

#8  0x00007ffff5c2d0b9 in G1ContiguousSpace::block_start (this=0x7ffff021e960, p=0x715d8cd78)
    at open/src/hotspot/share/gc/g1/heapRegion.inline.hpp:110
#9  0x00007ffff5b179c9 in G1CollectedHeap::block_start (this=0x7ffff0042a00, addr=0x715d8cd78)
    at open/src/hotspot/share/gc/g1/g1CollectedHeap.cpp:2250
#10 0x00007ffff60e05b3 in oopDesc::oop_or_null (addr=0x715d8cd78 "\005")
    at open/src/hotspot/share/oops/oop.cpp:206
#11 0x00007ffff61001b9 in os::print_location (st=0x7ffff0000930, x=30431300984, verbose=false)
    at open/src/hotspot/share/runtime/os.cpp:1052
#12 0x00007ffff59cf4d4 in hsfind (x=30431300984) at open/src/hotspot/share/utilities/debug.cpp:598

But it prints fine because it's an oop.

(gdb) call method_mirror->print()
java.lang.reflect.Method 
{0x0000000715d8cd78} - klass: 'java/lang/reflect/Method'
 - ---- fields (total size 11 words):
 - 'override' 'Z' @12  false
 - volatile 'accessCheckCache' 'Ljava/lang/Object;' @16  NULL
 (0)
...

It looks like is_valid() returns false when it should, but then things go downhill from there.
Comments
Summary: Use Metaspace::contains() to test address ranges. Tested with hs-tier1-3, and all tier1 on linux-x64-debug. Tested manually in gdb. open webrev at http://cr.openjdk.java.net/~coleenp/2019/8219459.01/webrev Out for review.
10-05-2019
The lock was added by JDK-8214522. I you want to keep it, can you change it to "MutexLocker cl(VMError::is_error_reported() ? NULL : MetaspaceExpand_lock, Mutex::_no_safepoint_check_flag);", please? I agree with that it's an overkill for the current use case. I can also live with only checking both ends individually if nobody plans to use it for further checks.
08-05-2019
I agree with getting rid of MetaspaceUtils::is_range_in_committed. klass.cpp can check both ends of k individually.
07-05-2019
It always had a lock (except when testing the CDS regions). -metaspace::VirtualSpaceNode* MetaspaceUtils::find_enclosing_virtual_space(const void* p) { - MutexLocker cl(MetaspaceExpand_lock, Mutex::_no_safepoint_check_flag); Okay, I can put back the function that I dislike and just fix the CDS code. Then it's trivial. I think the range check is way overkill, especially since it's checking is_range_committed(k, k+1); Metaspace is complicated enough and has enough code that we shouldn't have this function if we're not really using it. I would like to remove it. grep -r is_range_in_committed memory/metaspace.hpp: static bool is_range_in_committed(const void* from, const void* to); memory/metaspace.cpp.orig:bool MetaspaceUtils::is_range_in_committed(const void* from, const void* to) { memory/metaspace.cpp:bool MetaspaceUtils::is_range_in_committed(const void* from, const void* to) { oops/klass.cpp: if (!MetaspaceUtils::is_range_in_committed(k, k + 1)) return false;
07-05-2019
I think Coleen's version should be ok for the current usage, but MetaspaceUtils::is_range_in_committed may get used for further checks in the future. In that case, I have the same concern as Ioi. In addition, l think that acquiring a lock is a risk because the function is used during error reporting after a crash. If the lock is held, error reporting will hang.
07-05-2019
Can the (non-CDS) metaspace have many disjoint regions? If so, this seems a bit too loose to me: return Metaspace::contains(from) && Metaspace::contains(to);
07-05-2019
The nice thing about using contains is that I can remove the function that's a little like the others only different. It would be nice if metaspace has less knowledge than that of CDS. open webrev at http://cr.openjdk.java.net/~coleenp/2019/8219459.01/webrev
07-05-2019
I guess nobody would normally notice the difference between the stronger and the weaker check. But you never know what it will get used for in the future, so I'd prefer keeping it stronger. I agree, it should better get implemented independend of the region encodings. Would it make sense to iterate from 0 to num_core_spaces?
07-05-2019
Ok, I see. Does it need to be this strong? It appears only for is_valid. It would be better for it not to know the order of CDS archive regions.
07-05-2019
The existing check is stronger. It checks if from and to are in the same space. I think the space enum was rearranged (JDK-8072061), but the loop was implemented for the old enum.
07-05-2019
Yes, thank you for the diagnosis. I found the same. This seems to fix it. I don't know why this doesn't just call Metaspace::contains() though. diff --git a/src/hotspot/share/memory/metaspace.cpp b/src/hotspot/share/memory/metaspace.cpp --- a/src/hotspot/share/memory/metaspace.cpp +++ b/src/hotspot/share/memory/metaspace.cpp @@ -917,10 +917,8 @@ bool MetaspaceUtils::is_range_in_committed(const void* from, const void* to) { #if INCLUDE_CDS if (UseSharedSpaces) { - for (int idx = MetaspaceShared::ro; idx <= MetaspaceShared::mc; idx++) { - if (FileMapInfo::current_info()->is_in_shared_region(from, idx)) { - return FileMapInfo::current_info()->is_in_shared_region(to, idx); - } + if (MetaspaceShared::is_in_shared_metaspace(from)) { + return MetaspaceShared::is_in_shared_metaspace(to); } } #endif
07-05-2019
Are you using CDS? I think MetaspaceUtils::is_range_in_committed is incorrect for UseSharedSpaces. It doesn't iterate correctly over MetaspaceShared::ro, mc,... spaces.
02-04-2019