RFR(S): 8155105: Enhance guardedMemory to detect accessing released memory

[Coleen Phillimore]

Hi Zhengyu, The changes look okay to me.  I'm going to run testing over 
the weekend on it.   I think it could use another careful reviewer though.

thanks,
Coleen


On 5/6/16 4:02 PM, Zhengyu Gu wrote:
> Hi Coleen & David,
>
> Thank you so much for reviewing. I updated the webrev based on your 
> comments and ran jtreg with "-Xcheck:jni"  on Linux 64 as Mr. Simms 
> suggested.
>
> I did hesitate to add another VM flag. However, I feel that there is 
> no choice. Obviously,  not releasing memory is definitely not an 
> option, but without the option, almost all the cases appear to be 
> wild-pointer. I could not get consistent results, even with two 
> consecutive free() calls.
>
> http://cr.openjdk.java.net/~zgu/8155105/webrev.01/ 
> <http://cr.openjdk.java.net/%7Ezgu/8155105/webrev.01/>
>
> Regards,
>
> -Zhengyu
>
>
> On 05/06/2016 08:22 AM, Coleen Phillimore wrote:
>>
>> Hi Zhengyu,  Thank you for doing this change.  I did a first pass 
>> through the review.
>>
>>
>> http://cr.openjdk.java.net/~zgu/8155105/webrev/src/share/vm/memory/guardedMemory.hpp.udiff.html 
>>
>>
>> I agree with Mr Simms that there is too much implementation code in 
>> guardedMemory.hpp, particularly the functions get_tail_guard, 
>> get_tail_guard_size, set_tracking_stack_and_user_bytes.  This header 
>> file shouldn't have to include nmtCommon.hpp.
>>
>> This function added
>>
>> + bool is_currupted() const {
>>
>>
>> is spelled wrong and doesn't seem to be called anywhere.
>>
>> http://cr.openjdk.java.net/~zgu/8155105/webrev/src/share/vm/runtime/globals.hpp.udiff.html 
>>
>>
>> Is there a way to do this without adding another flag?  We have too 
>> many flags (that lack testing!) and want to resist adding more.  I'd 
>> rather it go in as commented out code (but maybe others disagree with 
>> me).
>>
>> I offer to sponsor this and I'm sorry for not looking at it sooner.   
>> I haven't had time to look at the functionality (but thankfully Mr 
>> Simms has).   The timing is unfortunate because JDK9 feature complete 
>> for everything but jigsaw is Tuesday.  I don't know if this can make 
>> it by then and we might have to hold it until the repository opens up 
>> for the next release.
>>
>> Thanks,
>> Coleen
>>
>> On 5/6/16 4:01 AM, David Simms wrote:
>>> Hi,
>>>
>>> The change itself looks good, I assume you have made a quick check 
>>> for regressions when using "Checked JNI" ("java -version 
>>> -Xcheck:jni" at least) ?
>>>
>>> A few minor comments:
>>>
>>> guardedMemory.hpp:56 needs comment modified to describe the "release 
>>> call stack" + freeBlockPad
>>>
>>> guardedMemory.hpp:363 "alway" -> "always"
>>>
>>> guardedMemory.hpp: In general there's a border-line amount of 
>>> implementation code in the header file which could be moved to 
>>> guardedMemory.cpp, but that's subjective comment (feel free to ignore).
>>>
>>> Some malloc/free implementations are obviously better at debugging 
>>> this for you, looks like you are filling a gap for some :-)
>>>
>>> Thanks for doing this !
>>>
>>> /Mr. Simms
>>>
>>> On 29/04/2016 3:22 p.m., Zhengyu Gu wrote:
>>>> This is a debug-only change that is intended to detect accessing 
>>>> released memory, ex. double-release a malloc'd memory.
>>>>
>>>> The approach is to rewrite the memory guards with "released" 
>>>> pattern and write the calling stack into user data area during 
>>>> os::free() call.
>>>> As the result,  verify_memory() check will result a failure if it 
>>>> sees "released" guards.
>>>>
>>>> Double-free is not obvious usually, as the memory can be 
>>>> reallocated before the second free().  Most of time, it appears to 
>>>> be a wild-pointer, -XX:TraceMemoryDoubleFree flag is intended to 
>>>> help to identify such scenario, by only building released guards, 
>>>> but not actually free the memory, so it can only help when 
>>>> double-free is caught before runs out of memory.
>>>>
>>>> When double-free is caught, the two free() call stacks are provided:
>>>>
>>>> ## nof_mallocs = 56722, nof_frees = 9099
>>>> ## memory stomp:
>>>> GuardedMemory(0x00007f85a0a06c30) base_addr=0x00007f859af2f630 
>>>> tag=0x0000000000000000 user_size=17 user_data=0x00007f859af2f650
>>>>   Header guard @0x00007f859af2f630 is RELASED
>>>>   Trailer guard @0x00007f859af2f670 is RELASED
>>>>   User data appears to be releasing call stack
>>>> From:
>>>> [0x00007f859f150dc2] os::free(void*)+0x52
>>>> [0x00007f859ec79b5b] GuardedMemory::test_guarded_memory()+0x156b
>>>> [0x00007f859ed02e44] InternalVMTests::run()+0x1d4
>>>> [0x00007f859ed58c15] JNI_CreateJavaVM+0x3e5
>>>> Memory has been released from:
>>>> [0x00007f859ec79b53] GuardedMemory::test_guarded_memory()+0x1563
>>>> [0x00007f859ed02e44] InternalVMTests::run()+0x1d4
>>>> [0x00007f859ed58c15] JNI_CreateJavaVM+0x3e5
>>>> [0x00007f85a01ca753] JavaMain+0x83
>>>>
>>>> Also, updated guarded memory tests to test on "base" pointer 
>>>> (returned by ::malloc()) vs "user" pointer (os::malloc()) to 
>>>> reflect real runtime scenarios.
>>>>
>>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8155105 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8155105>
>>>> Webrev: http://cr.openjdk.java.net/~zgu/8155105/webrev/index.html 
>>>> <http://cr.openjdk.java.net/%7Ezgu/8155105/webrev/index.html>
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>> <http://cr.openjdk.java.net/%7Ezgu/8155105/webrev/index.html>
>>>>
>>>
>>
>