Index: subversion/bindings/javahl/native/OperationContext.cpp =================================================================== --- subversion/bindings/javahl/native/OperationContext.cpp (revision 1880751) +++ subversion/bindings/javahl/native/OperationContext.cpp (working copy) @@ -492,6 +492,8 @@ 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 @@ apr_file_t *response_in; apr_file_t *response_out; apr_status_t status; + jobject jrequest; + jobject jresponse; jobject jclosecb; }; @@ -523,7 +527,10 @@ jmethodID ctor = env->GetMethodID(cls, "", "(J)V"); if (JNIUtil::isJavaExceptionThrown()) return NULL; - return env->NewObject(cls, ctor, reinterpret_cast(fd)); + jobject channel = env->NewObject(cls, ctor, reinterpret_cast(fd)); + if (JNIUtil::isJavaExceptionThrown()) + return NULL; + return env->NewGlobalRef(channel); } jobject create_RequestChannel(JNIEnv *env, apr_file_t *fd) @@ -534,6 +541,24 @@ { return create_Channel(JAVAHL_CLASS("/util/ResponseChannel"), env, fd); } +void close_TunnelChannel(JNIEnv* env, jobject channel) +{ + // Usually after this function, the memory will be freed behind + // 'TunnelChannel.nativeChannel'. Ask Java side to forget it. This is the + // only way to avoid a JVM crash when 'TunnelAgent' tries to read/write, + // not knowing that 'TunnelChannel' is already closed in native side. + static jmethodID mid = 0; + if (0 == mid) + { + jclass cls; + SVN_JNI_CATCH_VOID( + cls = env->FindClass(JAVAHL_CLASS("/util/TunnelChannel"))); + SVN_JNI_CATCH_VOID(mid = env->GetMethodID(cls, "syncClose", "()V")); + } + + SVN_JNI_CATCH_VOID(env->CallVoidMethod(channel, mid)); + env->DeleteGlobalRef(channel); +} } // anonymous namespace svn_boolean_t @@ -590,10 +615,10 @@ JNIEnv *env = JNIUtil::getEnv(); - jobject jrequest = create_RequestChannel(env, tc->request_in); + tc->jrequest = create_RequestChannel(env, tc->request_in); SVN_JNI_CATCH(, SVN_ERR_BASE); - jobject jresponse = create_ResponseChannel(env, tc->response_out); + tc->jresponse = create_ResponseChannel(env, tc->response_out); SVN_JNI_CATCH(, SVN_ERR_BASE); jstring jtunnel_name = JNIUtil::makeJString(tunnel_name); @@ -623,12 +648,24 @@ } jobject jtunnelcb = jobject(tunnel_baton); - SVN_JNI_CATCH( - tc->jclosecb = env->CallObjectMethod( - jtunnelcb, mid, jrequest, jresponse, - jtunnel_name, juser, jhostname, jint(port)), - SVN_ERR_BASE); + tc->jclosecb = env->CallObjectMethod( + jtunnelcb, mid, tc->jrequest, tc->jresponse, + jtunnel_name, juser, jhostname, jint(port)); + svn_error_t* openTunnelError = JNIUtil::checkJavaException(SVN_ERR_BASE); + if (SVN_NO_ERROR != openTunnelError) + { + // Clear exception to let OperationContext::closeTunnel() work. + env->ExceptionClear(); + // OperationContext::closeTunnel() will never be called, clean up here. + // This also prevents a JVM native crash, see comment in + // close_TunnelChannel(). + *close_baton = 0; + tc->jclosecb = 0; + OperationContext::closeTunnel(tc, 0); + SVN_ERR(openTunnelError); + } + return SVN_NO_ERROR; } @@ -636,24 +673,35 @@ OperationContext::closeTunnel(void *tunnel_context, void *) { TunnelContext* tc = static_cast(tunnel_context); + jobject jrequest = tc->jrequest; + jobject jresponse = tc->jresponse; jobject jclosecb = tc->jclosecb; + + // Note that this closes other end of the pipe, which cancels and + // prevents further read/writes in 'TunnelAgent' delete tc; - if (!jclosecb) - return; - JNIEnv *env = JNIUtil::getEnv(); if (JNIUtil::isJavaExceptionThrown()) return; - static jmethodID mid = 0; - if (0 == mid) + if (jclosecb) { - jclass cls; - SVN_JNI_CATCH_VOID( - cls= env->FindClass( - JAVAHL_CLASS("/callback/TunnelAgent$CloseTunnelCallback"))); - SVN_JNI_CATCH_VOID(mid = env->GetMethodID(cls, "closeTunnel", "()V")); + static jmethodID mid = 0; + if (0 == mid) + { + jclass cls; + SVN_JNI_CATCH_VOID( + cls= env->FindClass( + JAVAHL_CLASS("/callback/TunnelAgent$CloseTunnelCallback"))); + SVN_JNI_CATCH_VOID(mid = env->GetMethodID(cls, "closeTunnel", "()V")); + } + SVN_JNI_CATCH_VOID(env->CallVoidMethod(jclosecb, mid)); } - SVN_JNI_CATCH_VOID(env->CallVoidMethod(jclosecb, mid)); + + if (jrequest) + close_TunnelChannel(env, jrequest); + + if (jresponse) + close_TunnelChannel(env, jresponse); } Index: subversion/bindings/javahl/src/org/apache/subversion/javahl/util/RequestChannel.java =================================================================== --- subversion/bindings/javahl/src/org/apache/subversion/javahl/util/RequestChannel.java (revision 1880751) +++ subversion/bindings/javahl/src/org/apache/subversion/javahl/util/RequestChannel.java (working copy) @@ -42,15 +42,18 @@ public int read(ByteBuffer dst) throws IOException { - long channel = nativeChannel.get(); - if (channel != 0) - try { - return nativeRead(channel, dst); - } catch (IOException ex) { - nativeChannel.set(0); // Close the channel - throw ex; - } - throw new ClosedChannelException(); + synchronized (nativeChannel) + { + long channel = nativeChannel.get(); + if (channel != 0) + try { + return nativeRead(channel, dst); + } catch (IOException ex) { + nativeChannel.set(0); // Close the channel + throw ex; + } + throw new ClosedChannelException(); + } } private static native int nativeRead(long nativeChannel, ByteBuffer dst) Index: subversion/bindings/javahl/src/org/apache/subversion/javahl/util/TunnelChannel.java =================================================================== --- subversion/bindings/javahl/src/org/apache/subversion/javahl/util/TunnelChannel.java (revision 1880751) +++ subversion/bindings/javahl/src/org/apache/subversion/javahl/util/TunnelChannel.java (working copy) @@ -50,6 +50,39 @@ nativeClose(channel); } + /** + * Wait for current read/write to complete, then close() channel. + * Compared to close(), it has the following differences: + *
    + *
  • + * Prevents race condition where read/write could use incorrect file : + *
      + *
    1. Writer thread extracts OS file descriptor from nativeChannel.
      2. + *
    2. Writer thread calls OS API to write to file, passing file descriptor.
      3. + *
    3. Writer thread is interrupted.
      4. + *
    4. Closer thread closes OS file descriptor. The file descriptor number is now free.
      5. + *
    5. Unrelated thread opens a new file. OS reuses the old file descriptor (currently free).
      6. + *
    6. Writer thread resumes inside OS API to write to file.
      7. + *
    7. Writer thread writes to unrelated file, corrupting it with unexpected data.
      8. + *
    + *
    • + *
  • + * It can no longer cancel a read/write operation already in progress. + * The native implementation closes the other end of the pipe, breaking the pipe, + * which prevents the risk of never-completing read/write. + *
    • + *
      + * + * @throws IOException + */ + public void syncClose() throws IOException + { + synchronized (nativeChannel) + { + close(); + } + } + private native static void nativeClose(long nativeChannel) throws IOException; Index: subversion/bindings/javahl/src/org/apache/subversion/javahl/util/ResponseChannel.java =================================================================== --- subversion/bindings/javahl/src/org/apache/subversion/javahl/util/ResponseChannel.java (revision 1880751) +++ subversion/bindings/javahl/src/org/apache/subversion/javahl/util/ResponseChannel.java (working copy) @@ -42,15 +42,18 @@ public int write(ByteBuffer src) throws IOException { - long channel = this.nativeChannel.get(); - if (channel != 0) - try { - return nativeWrite(channel, src); - } catch (IOException ex) { - nativeChannel.set(0); // Close the channel - throw ex; - } - throw new ClosedChannelException(); + synchronized (nativeChannel) + { + long channel = this.nativeChannel.get(); + if (channel != 0) + try { + return nativeWrite(channel, src); + } catch (IOException ex) { + nativeChannel.set(0); // Close the channel + throw ex; + } + throw new ClosedChannelException(); + } } private static native int nativeWrite(long nativeChannel, ByteBuffer src)