Please find test snippet and patch attached.
[[[
Fix JavaHL crash in RequestChannel.nativeRead
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.
One other problem is that when 'TunnelAgent.openTunnel()' throws an
exception, 'OperationContext::closeTunnel()' was not called at all.
[in subversion/bindings/javahl]
* native/OperationContext.cpp
(close_TunnelChannel): New function to inform Java side.
(openTunnel): Keep references to Java tunnel objects.
(openTunnel): In case of exception, clean up properly.
(closeTunnel): Inform Java side when tunnel is closed.
* src/org/apache/subversion/javahl/util/RequestChannel.java
Add 'synchronized' to avoid error described in 'syncClose()'
* 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.
]]]
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, "<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);
}
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<TunnelContext*>(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:
+ * <ul>
+ * <li>
+ * Prevents race condition where read/write could use incorrect file :
+ * <ol>
+ * <li>Writer thread extracts OS file descriptor from
nativeChannel.</li>
+ * <li>Writer thread calls OS API to write to file, passing file
descriptor.</li>
+ * <li>Writer thread is interrupted.</li>
+ * <li>Closer thread closes OS file descriptor. The file descriptor
number is now free.</li>
+ * <li>Unrelated thread opens a new file. OS reuses the old file
descriptor (currently free).</li>
+ * <li>Writer thread resumes inside OS API to write to file.</li>
+ * <li>Writer thread writes to unrelated file, corrupting it with
unexpected data.</li>
+ * </ol>
+ * </li>
+ * <li>
+ * 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.
+ * </li>
+ * <ul/>
+ *
+ * @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)
import java.io.*;
import java.lang.reflect.Field;
import java.nio.*;
import java.nio.channels.*;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicLong;
import org.apache.subversion.javahl.*;
import org.apache.subversion.javahl.callback.*;
public class JavaHL_Crash_RequestChannel_nativeRead {
public static void main(String[] args) {
final SVNClient svnClient = new SVNClient();
final CountDownLatch exceptionReceived = new CountDownLatch(1);
svnClient.setTunnelAgent(new TunnelAgent() {
@Override
public boolean checkTunnel(String name) {
return true;
}
@Override
public CloseTunnelCallback
openTunnel(ReadableByteChannel request, WritableByteChannel response, String
name, String user, String hostname, int port) throws Throwable {
new Thread(() -> {
try {
dump_RequestChannel_nativeChannel(request);
exceptionReceived.await();
request.read(ByteBuffer.allocate(1024));
System.out.println();
System.out.println("request.read() succeed, kind of...");
} catch (IOException e) {
System.out.println();
System.out.println("IOException
was caught.");
} catch (Exception e) {
e.printStackTrace(System.out);
} finally {
System.out.println("This
happens when already-freed apr_file_t was lucky to have reasonable fields to
avoid the crash.");
System.out.println("In order to
reproduce the crash, do one of:");
System.out.println("a) Try
again. Usually it reproduces once in 10 times or so.");
System.out.println("b) Use
Application Verifier with Basics/Heaps for java.exe");
}
}).start();
// Exception causes 'svn_ra_open5()' to call
'svn_pool_destroy(sesspool)'
// which also destroys
'TunnelChannel.nativeClose'.
throw ClientException.fromException(new
RuntimeException("Test exception"));
}
});
try {
svnClient.openRemoteSession("svn+ssh://localhost/test");
} catch (SubversionException e) {
exceptionReceived.countDown();
}
}
// For debugging purposes, not needed to reproduce the crash.
static void dump_RequestChannel_nativeChannel(ReadableByteChannel
channel) throws Exception {
Class class_TunnelChannel =
Class.forName("org.apache.subversion.javahl.util.TunnelChannel");
Field field_nativeChannel =
class_TunnelChannel.getDeclaredField("nativeChannel");
field_nativeChannel.setAccessible(true);
AtomicLong nativeChannel =
(AtomicLong)field_nativeChannel.get(channel);
System.out.format("%X = TunnelChannel.nativeChannel%n",
nativeChannel.get());
System.out.flush();
}
}
