On Thu, Oct 15, 2020 at 12:16 PM <amiloslavs...@apache.org> wrote: > > Author: amiloslavskiy > Date: Thu Oct 15 10:15:47 2020 > New Revision: 1882522 > > URL: http://svn.apache.org/viewvc?rev=1882522&view=rev > Log: > JavaHL: Fix crash in TunnelAgent.CloseTunnelCallback after GC > > When jobject reference is kept across different JNI calls, a new global > reference must be requested with NewGlobalRef(). Otherwise, GC is free > to remove the object. Even if Java code keeps a reference to the object, > GC can still move the object around, invalidating the kept jobject, > which results in a native crash when trying to access it. > > This crash is demonstrated by the following JavaHL test: > 'testCrash_RemoteSession_nativeDispose' > > [in subversion/bindings/javahl] > * native/OperationContext.cpp > (callCloseTunnelCallback): Extract function to facilitate changes in > further commits. > (openTunnel): Add NewGlobalRef() for kept jobject. > (callCloseTunnelCallback): Add a matching DeleteGlobalRef(). > > Modified: > > subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp > > Modified: > subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp > URL: > http://svn.apache.org/viewvc/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp?rev=1882522&r1=1882521&r2=1882522&view=diff > ============================================================================== > --- > subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp > (original) > +++ > subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp > Thu Oct 15 10:15:47 2020 > @@ -629,23 +629,17 @@ OperationContext::openTunnel(svn_stream_ > jtunnel_name, juser, jhostname, jint(port)), > SVN_ERR_BASE); > > + if (tc->jclosecb) > + { > + tc->jclosecb = env->NewGlobalRef(tc->jclosecb); > + SVN_JNI_CATCH(, SVN_ERR_BASE); > + } > + > return SVN_NO_ERROR; > } > > -void > -OperationContext::closeTunnel(void *tunnel_context, void *) > +void callCloseTunnelCallback(JNIEnv* env, jobject jclosecb) > { > - TunnelContext* tc = static_cast<TunnelContext*>(tunnel_context); > - jobject jclosecb = tc->jclosecb; > - delete tc; > - > - if (!jclosecb) > - return; > - > - JNIEnv *env = JNIUtil::getEnv(); > - if (JNIUtil::isJavaExceptionThrown()) > - return; > - > static jmethodID mid = 0; > if (0 == mid) > { > @@ -656,4 +650,20 @@ OperationContext::closeTunnel(void *tunn > SVN_JNI_CATCH_VOID(mid = env->GetMethodID(cls, "closeTunnel", "()V")); > } > SVN_JNI_CATCH_VOID(env->CallVoidMethod(jclosecb, mid)); > + env->DeleteGlobalRef(jclosecb); > +} > + > +void > +OperationContext::closeTunnel(void *tunnel_context, void *) > +{ > + TunnelContext* tc = static_cast<TunnelContext*>(tunnel_context); > + jobject jclosecb = tc->jclosecb; > + delete tc; > + > + JNIEnv *env = JNIUtil::getEnv(); > + if (JNIUtil::isJavaExceptionThrown()) > + return; > + > + if (jclosecb) > + callCloseTunnelCallback(env, jclosecb); > } > >
Here above in closeTunnel(), you dropped the early exit on if (!jclosecb) (before the call to JNIUtil::getEnv()). Was this intentional? Previously, in the case of a null jclosecb, the unnecessary calls to JNIUtil::getEnv() and JNIUtil::isJavaExceptionThrown() were also avoided. In the new version they aren't (not sure if that's important, but it's a small behavioral difference). -- Johan
