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

[Coleen Phillimore]

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>
>>
>