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

[frederic parain]

Thank you Vladimir,

Fred

On 10/07/2013 17:01, Vladimir Kozlov wrote:
> Looks good.
>
> Thanks,
> Vladimir
>
> On 7/10/13 7:37 AM, frederic parain wrote:
>> Hi Vladimir,
>>
>> Thank you for catching this.
>>
>> A new webrev is available:
>> http://cr.openjdk.java.net/~fparain/7143807/webrev.02/
>>
>> One liners are now using DEBUG_ONLY.
>> Bad code in ResourceMark(ResourceArea*) has been fixed.
>>
>> Thanks,
>>
>> Fred
>>
>> On 08/07/2013 23:59, Vladimir Kozlov wrote:
>>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>
>>

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