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

[Vladimir Kozlov]

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