Committed in r947006. I tweaked the patch just a bit to ensure we catch any
exceptions generated. Please confirm that it still functions as expected.
Thanks!
-Hyrum
On Thu, May 20, 2010 at 5:13 PM, Byeongcheol Lee <lineonking_at_gmail.com>wrote:
> Dear Subversion Developers:
>
> I attach a patch for a local reference leak in Outter.cpp, and the log
> message follows here:
>
> [[[
> * subversion/bindings/javahl/native/Outputer.cpp
> (write): add a call to "DeleteLocalRef" where a leaking local
> reference is unreachable.
> ]]]
>
> To reproduce this bug, run your javaHL regression test under our
> dynamic bug detector, JInn [http://userweb.cs.utexas.edu/~bclee/jinn<http://userweb.cs.utexas.edu/%7Ebclee/jinn>
> ]:
>
> $env JAVA_TOOL_OPTIONS=-agentlib:jinn make check-javahl
> ...
> .Exception in thread "main" xtc.lang.blink.agent.JNIAssertionFailure:
> The number of local references exceeds the current capacity 16 in
> NewByteArray.
> at
> xtc.lang.blink.agent.JNIAssertionFailure.assertFail(JNIAssertionFailure.java:16)
> at org.apache.subversion.javahl.SVNAdmin.dump(Native Method)
> at org.apache.subversion.javahl.SVNAdmin.dump(SVNAdmin.java:128)
> at org.apache.subversion.javahl.SVNTests.setUp(SVNTests.java:241)
> at junit.framework.TestCase.runBare(TestCase.java:128)
> ...
> at org.apache.subversion.javahl.RunTests.main(RunTests.java:116)
> ...
>
> The local reference leak happens when a Java native method creates
> more than 16 local references without deleting unreachable references
> (DeleteLocalRef), adjusting capacity (EnsureLocalCapacity) or
> allocating a local frame (PushLocalFrame). The Subversion r946780
> violates this JNI programming rule:
>
> "The JNI specification dictates that the virtual machine automatically
> ensures that each native method can create at least 16 local
> references. Experience shows that this provides enough capacity for
> the majority of native methods that do not contain complex
> interactions with objects in the Java virtual machine. If, however,
> there is a need to create additional local references, a native method
> may issue an EnsureLocalCapacity call to make sure that space for a
> sufficient number of local references is available. For example, a
> slight variation of a previous example above reserves enough capacity
> for all local references created during the loop execution if
> sufficient memory is available:"
> [http://java.sun.com/docs/books/jni/html/refs.html#27592]
>
> I attached gdb to the JVM, and examined every JNI call that allocates
> a local reference, and I ended up the leaking allocation site at Line
> 99 in subversion/bindings/javahl/native/Outputer.cpp:
>
> ...
> 73 svn_error_t *Outputer::write(void *baton, const char *buffer,
> apr_size_t *len)
> 74 {
> ...
> 99 jbyteArray data = JNIUtil::makeJByteArray((const signed
> char*)buffer,
> 100 *len);
> 101 if (JNIUtil::isJavaExceptionThrown())
> 102 return SVN_NO_ERROR;
> 103
> 104 // write the data
> 105 jint written = env->CallIntMethod(that->m_jthis, mid, data);
> 106 if (JNIUtil::isJavaExceptionThrown())
> 107 return SVN_NO_ERROR;
> 108
> 109 // return the number of bytes written
> 110 *len = written;
> 111
> 112 return SVN_NO_ERROR;
> 113 }
>
> This "write" method does not delete the local reference in the
> variable "data" at Line 99 when it returns. In the test run, this
> "write" method was activated more than 16 times. The simplest fix is
> adding "env->DeleteLocalRef(data)" after Line 105 where the "data"
> variable is not used in the future. Even if Line 105 throws a Java
> exception, it's fine because "DeleteLocalRef" is one of 20
> exception-oblivious function:
>
> "After an exception has been raised, the native code must first clear
> the exception before making other JNI calls. When there is a pending
> exception, the JNI functions that are safe to call are:
> ....
> DeleteLocalRef()
> ...." [
> http://java.sun.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp770
> ]
>
> For your information, here are calling contexts that allocate 16
> unreachable local references:
>
> 1
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1072
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 2
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1075
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 3
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> write_revision_record at subversion/libsvn_repos/dump.c:985
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1101
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 4
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> write_revision_record at subversion/libsvn_repos/dump.c:988
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1101
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 5
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> write_revision_record at subversion/libsvn_repos/dump.c:995
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1101
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 6
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> write_revision_record at subversion/libsvn_repos/dump.c:1001
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1101
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 7
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> write_revision_record at subversion/libsvn_repos/dump.c:1004
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1101
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 8
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> repos_progress_handler at subversion/libsvn_repos/deprecated.c:454
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1159
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 9
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> write_revision_record at subversion/libsvn_repos/dump.c:985
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1119
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 10
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> write_revision_record at subversion/libsvn_repos/dump.c:988
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1119
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 11
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> write_revision_record at subversion/libsvn_repos/dump.c:995
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1119
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 12
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> write_revision_record at subversion/libsvn_repos/dump.c:1001
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1119
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 13
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> write_revision_record at subversion/libsvn_repos/dump.c:1004
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1119
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 14
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> dump_node at subversion/libsvn_repos/dump.c:330
> add_directory at subversion/libsvn_repos/dump.c:723
> path_driver_cb_func at subversion/libsvn_repos/replay.c:450
> svn_delta_path_driver at subversion/libsvn_delta/path_driver.c:254
> svn_repos_replay2 at subversion/libsvn_repos/replay.c:777
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1152
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 15
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> dump_node at subversion/libsvn_repos/dump.c:337
> add_directory at subversion/libsvn_repos/dump.c:723
> path_driver_cb_func at subversion/libsvn_repos/replay.c:450
> svn_delta_path_driver at subversion/libsvn_delta/path_driver.c:254
> svn_repos_replay2 at subversion/libsvn_repos/replay.c:777
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1152
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> 16
> Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101
> svn_stream_write at subversion/libsvn_subr/stream.c:153
> svn_stream_printf at subversion/libsvn_subr/stream.c:209
> dump_node at subversion/libsvn_repos/dump.c:417
> add_directory at subversion/libsvn_repos/dump.c:723
> path_driver_cb_func at subversion/libsvn_repos/replay.c:450
> svn_delta_path_driver at subversion/libsvn_delta/path_driver.c:254
> svn_repos_replay2 at subversion/libsvn_repos/replay.c:777
> svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1152
> svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480
> SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210
> Java_org_apache_subversion_javahl_SVNAdmin_dump at
> org_apache_subversion_javahl_SVNAdmin.cpp:165
>
> Regards,
> Byeong
>
Received on 2010-05-21 15:50:05 CEST