Hi David, > With your changes you no longer null out _prologue so the assertion would > now not fail and we'd proceed to access the deleted memory region!
On Linux, PerfMemory::delete_memory_region() does not call munmap() for PerfMemory. > I'm unclear why you no longer clear all the fields set during > initialization? PerfMemory.java in jdk.hotspot.agent needs these field values. `jhsdb jsnap --core` is failed if they are cleared. > But it seems to me that there are various checks of > _prologue that should really be checking is_initialized() and/or > is_destroyed() as a guard. Should I change all assertions for _prologue? Thanks, Yasumasa 2017-10-18 10:53 GMT+09:00 David Holmes <david.hol...@oracle.com>: > Hi Yasumasa, > > By chance we ran into this bug which I analysed yesterday: > > https://bugs.openjdk.java.net/browse/JDK-8189390 > > We hit the assertion: > > # Internal Error (/open/src/hotspot/share/runtime/perfMemory.cpp:216), > pid=17874, tid=17875 > # assert(_prologue != __null) failed: called before initialization > # > > which is misleading because it can fail if called before initialization, or > after PerfMemory::destroy has been called. > > With your changes you no longer null out _prologue so the assertion would > now not fail and we'd proceed to access the deleted memory region! > > I'm unclear why you no longer clear all the fields set during > initialization? But it seems to me that there are various checks of > _prologue that should really be checking is_initialized() and/or > is_destroyed() as a guard. > > Thanks, > David > > > On 16/10/2017 11:25 PM, Yasumasa Suenaga wrote: >> >> PING: >> >> Could you review it? >> >>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.05/ >> >> >> >> Thanks, >> >> Yasumasa >> >> >> On 2017/10/03 13:18, Yasumasa Suenaga wrote: >>> >>> Hi all, >>> >>> I added gtest unit test case for this change in new webrev: >>> >>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.05/ >>> >>> Could you review it? >>> >>> >>> Thanks, >>> >>> Yasumasa >>> >>> >>> >>> 2017-09-27 0:01 GMT+09:00 Yasumasa Suenaga <yasue...@gmail.com>: >>>> >>>> Hi all, >>>> >>>> I uploaded new webrev to be adapted to jdk10/hs: >>>> >>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.04/ >>>> >>>> >>>> Thanks, >>>> >>>> Yasumasa >>>> >>>> >>>> On 2017/09/21 7:45, Yasumasa Suenaga wrote: >>>>> >>>>> >>>>> PING: >>>>> >>>>> Have you checked this issue? >>>>> >>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.03/ >>>>> >>>>> >>>>> >>>>> >>>>> Yasumasa >>>>> >>>>> >>>>> On 2017/07/01 23:43, Yasumasa Suenaga wrote: >>>>>> >>>>>> >>>>>> PING: >>>>>> >>>>>> Have you checked this issue? >>>>>> >>>>>> >>>>>> Yasumasa >>>>>> >>>>>> >>>>>> On 2017/06/13 14:10, Yasumasa Suenaga wrote: >>>>>>> >>>>>>> >>>>>>> Hi all, >>>>>>> >>>>>>> I want to discuss about JDK-8151815: Could not parse core image with >>>>>>> JSnap. >>>>>>> >>>>>>> >>>>>>> In last year, I found JSnap cannot parse coredump and I've sent >>>>>>> review >>>>>>> request for it as JDK-8151815. However it has not been reviewed yet >>>>>>> [1]. >>>>>>> >>>>>>> We've discussed about safety implementation, but we could not get >>>>>>> consensus. >>>>>>> IMHO all SA tools should be handled java processes and core images, >>>>>>> and PerfCounter value is useful. So I fix this issue. >>>>>>> >>>>>>> I uploaded new webrev for this issue. I think this patch is safety >>>>>>> because new flag PerfMemory::_destroyed guards double free, and all >>>>>>> members in PerfMemory is accessible (they are not munmap'ed) >>>>>>> >>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.03/ >>>>>>> >>>>>>> >>>>>>> Can you cooperate? >>>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Yasumasa >>>>>>> >>>>>>> >>>>>>> [1] >>>>>>> >>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-April/019480.html >>>>>>> >>>> >