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

[Zhengyu Gu]

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