You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by am...@apache.org on 2020/10/15 10:15:47 UTC
svn commit: r1882522 -
/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
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);
}
Re: svn commit: r1882522 -
/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
Posted by Branko Čibej <br...@apache.org>.
On 27.01.2021 11:13, Johan Corveleyn wrote:
> On Thu, Oct 15, 2020 at 12:16 PM <am...@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
Re: svn commit: r1882522 -
/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
Posted by Alexandr Miloslavskiy <al...@syntevo.com>.
On 27.01.2021 22:56, Johan Corveleyn wrote:
> Thanks for explaining. Hm, maybe it would be good to note this in some
> form in the commit message, yes. Just to make sure others scrolling
> through this in the future are not puzzled by it. Something like:
> "While here, remove the early exit on null jclosecb for $reasons (also
> see the followup commits where cleanup of ... is further improved, and
> everything needs to be cleaned up independently)". (this is just a
> quick off the cuff example, feel free to word it how you would best
> describe it)
Done!
Re: svn commit: r1882522 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
Posted by Johan Corveleyn <jc...@gmail.com>.
On Wed, Jan 27, 2021 at 10:32 PM Alexandr Miloslavskiy
<al...@syntevo.com> wrote:
>
> On 27.01.2021 11:13, Johan Corveleyn wrote:
> >> +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).
>
> Yes, this is intentional. In r1882523, I further extend the cleanup by
> also cleaning 'jrequest' and 'jresponse'. In this case, early exit is no
> good, because everything needs to be cleaned independently. For the
> purpose of reducing the amount of diffs, I removed the early exit one
> patch earlier.
>
> I believe that calling 'JNIUtil::getEnv()' and
> 'JNIUtil::isJavaExceptionThrown()' is merely a negligible performance
> change. I thought that it's not worthy to describe in commit message,
> but I could do that if you think otherwise.
Thanks for explaining. Hm, maybe it would be good to note this in some
form in the commit message, yes. Just to make sure others scrolling
through this in the future are not puzzled by it. Something like:
"While here, remove the early exit on null jclosecb for $reasons (also
see the followup commits where cleanup of ... is further improved, and
everything needs to be cleaned up independently)". (this is just a
quick off the cuff example, feel free to word it how you would best
describe it)
Brane also replied (on dev@) this morning, giving a cleanup-argument too:
"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."
So I'm entirely convinced :-).
--
Johan
Re: svn commit: r1882522 -
/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
Posted by Alexandr Miloslavskiy <al...@syntevo.com>.
On 27.01.2021 11:13, Johan Corveleyn wrote:
>> +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).
Yes, this is intentional. In r1882523, I further extend the cleanup by
also cleaning 'jrequest' and 'jresponse'. In this case, early exit is no
good, because everything needs to be cleaned independently. For the
purpose of reducing the amount of diffs, I removed the early exit one
patch earlier.
I believe that calling 'JNIUtil::getEnv()' and
'JNIUtil::isJavaExceptionThrown()' is merely a negligible performance
change. I thought that it's not worthy to describe in commit message,
but I could do that if you think otherwise.
Re: svn commit: r1882522 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
Posted by Johan Corveleyn <jc...@gmail.com>.
On Thu, Oct 15, 2020 at 12:16 PM <am...@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