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;
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);
}
jobject create_RequestChannel(JNIEnv *env, apr_file_t *fd)
@@ -534,6 +541,24 @@ jobject create_ResponseChannel(JNIEnv *e
{
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 @@ OperationContext::openTunnel(svn_stream_
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);
@@ -625,7 +650,7 @@ OperationContext::openTunnel(svn_stream_
jobject jtunnelcb = jobject(tunnel_baton);
SVN_JNI_CATCH(
tc->jclosecb = env->CallObjectMethod(
- jtunnelcb, mid, jrequest, jresponse,
+ jtunnelcb, mid, tc->jrequest, tc->jresponse,
jtunnel_name, juser, jhostname, jint(port)),
SVN_ERR_BASE);
@@ -657,7 +682,12 @@ void
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;
JNIEnv *env = JNIUtil::getEnv();
@@ -666,4 +696,14 @@ OperationContext::closeTunnel(void *tunn
if (jclosecb)
callCloseTunnelCallback(env, jclosecb);
+
+ if (jrequest)
+ {
+ close_TunnelChannel(env, jrequest);
+ }
+
+ if (jresponse)
+ {
+ close_TunnelChannel(env, jresponse);
+ }
}
Modified:
subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/RequestChannel.java
URL:
http://svn.apache.org/viewvc/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/RequestChannel.java?rev=1882523&r1=1882522&r2=1882523&view=diff
==============================================================================
---
subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/RequestChannel.java
(original)
+++
subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/RequestChannel.java
Thu Oct 15 10:16:39 2020
@@ -42,15 +42,18 @@ class RequestChannel
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)
Modified:
subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/ResponseChannel.java
URL:
http://svn.apache.org/viewvc/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/ResponseChannel.java?rev=1882523&r1=1882522&r2=1882523&view=diff
==============================================================================
---
subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/ResponseChannel.java
(original)
+++
subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/ResponseChannel.java
Thu Oct 15 10:16:39 2020
@@ -42,15 +42,18 @@ class ResponseChannel
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)
Modified:
subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/TunnelChannel.java
URL:
http://svn.apache.org/viewvc/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/TunnelChannel.java?rev=1882523&r1=1882522&r2=1882523&view=diff
==============================================================================
---
subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/TunnelChannel.java
(original)
+++
subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/TunnelChannel.java
Thu Oct 15 10:16:39 2020
@@ -50,6 +50,39 @@ abstract class TunnelChannel implements
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;
