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

[Daniel D. Daugherty]

On 7/10/13 8: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/

Thumbs up!

src/share/vm/memory/resourceArea.hpp
     No comments.

src/share/vm/runtime/thread.hpp
     line 592: ResourceMark* current_resource_mark() {return 
_current_resource_mark; }
        needs a space after '{'.

src/share/vm/runtime/thread.cpp
     No comments.

src/share/vm/utilities/ostream.hpp
     No comments.

src/share/vm/utilities/ostream.cpp
     No comments.

I presume your vm.quick test run did not reveal any firings of the new
assertion.

Dan


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