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

[frederic parain]

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