On Thu, Oct 15, 2020 at 12:16 PM <amiloslavs...@apache.org> wrote: > > Author: amiloslavskiy > Date: Thu Oct 15 10:16:39 2020 > New Revision: 1882523 > > URL: http://svn.apache.org/viewvc?rev=1882523&view=rev > Log: > JavaHL: Fix crash when TunnelAgent continues reading after being closed > > The problem here is that when 'RemoteSession' is destroyed, it also > does 'apr_pool_destroy()' which frees memory behind 'apr_file_t', which > is represented by 'TunnelChannel.nativeChannel' in Java. > 'TunnelChannel' runs on a different thread and is unaware that > 'apr_file_t' pointer is now invalid. > > Fix this by informing 'TunnelChannel' before 'apr_file_t' is freed. > > This crash is demonstrated by the following JavaHL tests: > * testCrash_RequestChannel_nativeRead_AfterException > * testCrash_RequestChannel_nativeRead_AfterSvnError > > This commit alone does not fix all problems in these tests, see > subsequent commits as well. > > [in subversion/bindings/javahl] > * native/OperationContext.cpp > (close_TunnelChannel): New function to inform Java side. > (openTunnel): Keep references to Java tunnel objects. > (closeTunnel): Inform Java side when tunnel is closed. > * src/org/apache/subversion/javahl/util/RequestChannel.java > * src/org/apache/subversion/javahl/util/ResponseChannel.java > Add 'synchronized' to avoid error described in 'syncClose()' > * src/org/apache/subversion/javahl/util/Tunnel.java > A new function to clean up. I decided not to change old '.close()' > because the new function can lead to a deadlock if used incorrectly. > > Modified: > > subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp > > subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/RequestChannel.java > > subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/ResponseChannel.java > > subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/TunnelChannel.java > > 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=1882523&r1=1882522&r2=1882523&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:16:39 2020 > @@ -492,6 +492,8 @@ public: > request_out(NULL), > response_in(NULL), > response_out(NULL), > + jrequest(NULL), > + jresponse(NULL), > jclosecb(NULL) > { > status = apr_file_pipe_create_ex(&request_in, &request_out, > @@ -512,6 +514,8 @@ public: > apr_file_t *response_in; > apr_file_t *response_out; > apr_status_t status; > + jobject jrequest; > + jobject jresponse;
These new fields are not mentioned in the log message. > jobject jclosecb; > }; > > @@ -523,7 +527,10 @@ jobject create_Channel(const char *class > jmethodID ctor = env->GetMethodID(cls, "<init>", "(J)V"); > if (JNIUtil::isJavaExceptionThrown()) > return NULL; > - return env->NewObject(cls, ctor, reinterpret_cast<jlong>(fd)); > + jobject channel = env->NewObject(cls, ctor, reinterpret_cast<jlong>(fd)); > + if (JNIUtil::isJavaExceptionThrown()) > + return NULL; > + return env->NewGlobalRef(channel); > } ^^ also not mentioned in log message. (rest looks fine, so if you could amend the commit message with the missing details, that'd be great) -- Johan
