On 27.01.2021 11:13, Johan Corveleyn wrote:
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).
It's a minor performance degradation in the case where there's no
callback, but it is in fact safer because it catches more Java
exceptions during tunnel destruction. The null check is still there, so
that's fine. IMO (and it's been a long time) this change is good.
-- Brane
