Request for Review/feedback: JDK-7143807 ResourceMark nesting problem in stringStream

[Frederic Parain]

Hi Vladimir,

Thank you for the review.
Here's an updated webrev:
http://cr.openjdk.java.net/~fparain/7143807/webrev.01/

The thread is now stored in the ResourceMark (in debug mode
only). It still have to use ThreadLocalStorage::thread()
because of a corner case exists with JVMTI where a ResourceMark
is created but there's no Thread instance for the current
thread (see SafeResourceMark class).
The ResourceMark(ResourceArea) is also fixed, but is tightly
linked to the corner case cited above.

Thanks,

Fred

On 02/07/2013 21:02, Vladimir Kozlov wrote:
> Hi Frederic,
>
> You don't need to guard assert() and you can use DEBUG_ONLY( ) macro to
> guard one line.
>
> Why you used ThreadLocalStorage::thread() instead of Thread::current()
> in destructor. Can you preserve _thread in ResourceMark constructor to
> avoid it?
>
> I think you need to set _previous_resource_mark also in
> ResourceMark(ResourceArea *r) constructor otherwise you will loose
> inforamtion if you have nesting sequence:
>
> { ResourceMark rm1;          // _previous_resource_mark = NULL;
>                               // thread->_current_resource_mark = rm1
>    stringStream st(80);
>    { ResourceMark rm2(arena); // _previous_resource_mark = NULL;
>                               // thread->_current_resource_mark = NULL
>      { ResourceMark rm3;      // _previous_resource_mark = NULL;
>                               // thread->_current_resource_mark = rm3
>      } // thread->_current_resource_mark = NULL
>    }   // thread->_current_resource_mark = NULL
>
>    st.write("abcd", 4); // you will hit stringStream::write() assert here.
> }
>
> thanks,
> Vladimir
>
> On 7/2/13 10:53 AM, frederic parain wrote:
>> Hi,
>>
>> This request for review is related to the following bug:
>>
>> JDK-7143807 ResourceMark nesting problem in stringStream
>> http://bugs.sun.com/view_bug.do?bug_id=7143807
>> https://jbs.oracle.com/bugs/browse/JDK-7143807
>>
>> The buggy code initially found in method MethodHandlePrinter
>> doesn't exist anymore after its removal when the fix for
>> 7023639: "JSR 292 method handle invocation needs a fast path for
>> compiled code" has been pushed.
>>
>> However, there's still a risk that the bad pattern (ResourceMark
>> nesting while using stringStream) shows up again in the future.
>> Because this bad pattern is causing memory corruptions, which are
>> usually hard to track when they occur, I'm proposing to use this
>> bugID to add assertions in the code to detect the problem before
>> the memory get corrupted.
>>
>> The fix is quite simple. When a stringStream is allocated
>> the address of the current ResourceMark used by the thread
>> is stored in the stringStream instance. It doesn't matter if
>> the stringStream is allocated in a ResourceArea, on the stack
>> or in the C heap, the buffer is always allocated in the
>> current ResourceArea for dynamically sized stringStreams
>> (stringStream with static buffer are not concerned by this bug).
>> If later the stringStream instance tries to re-allocated a new
>> buffer, the code checks that the current ResourceMark is the
>> same as the one used during the creation of the stringStream.
>>
>> Because this code only performs verifications, it is conditioned
>> with the ASSERT variable and is only included in debug/fastdebug
>> builds, not in product builds.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~fparain/7143807/webrev.00/
>>
>> Testing:
>>     * vm.quick.testlist and vm.runtime.testlist with fastdebug build
>>       (to have asserts enabled)
>>     * JPRT job on the hotspot repository
>>     * manual testing with bad pattern inserted in VM code to verify
>>       that the assert detects it correctly.
>>
>> Thanks,
>>
>> Fred
>>
>>
>>

-- 
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at oracle.com