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

[Vladimir Kozlov]

Hi Frederic,

You did not fix one liners to use DEBUG_ONLY().

In ResourceMark(ResourceArea) you need to do the same as in regular RM:

+    if (thread != NULL) {
+      _thread = thread;
+      _previous_resource_mark = thread->current_resource_mark();
+      thread->set_current_resource_mark(this);

setting it to 'this' is not correct.


Thanks,
Vladimir

On 7/8/13 2:16 AM, Frederic Parain wrote:
> 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
>>>
>>>
>>>
>