Re: JavaHL : EXCEPTION_ACCESS_VIOLATION in checkout when destPath equals empty string

On Mon, 18 Sep 2006, Mark Phippard wrote:

> james82@gmail.com wrote on 09/18/2006 07:06:05 PM:
>
> > On 9/15/06, Alexandre Pique <apique@isoft.fr> wrote:
> > > I have an EXCEPTION_ACCESS_VIOLATION when I call the checkout method
> of
> > SVNClientInterface with an empty destPath. I think it could be catch
> easily.
> >
> > Yes, I agree. We should definitely catch this case. If you'd like to
> > file an issue about this, please consider it buddied.
>
> I did a quick check. It looks like the code checks for NULL, but not an
> empty string.

We do check for an empty string in Path.cpp, in the code path we seem
to be passing through right before the JVM crash:

Path::init (const char * pi_path)
{
    if(*pi_path == 0)
    {
        m_path = "";
    }
    ...

SVNClient::checkout() creates a Path object, which in my test case --
edit BasicTest.testBasicCheckout() to pass "" for destPath -- is
producing an error with the following stack:

Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
C [libapr-1.so.0+0x14ca7] apr_pool_destroy+0x17
C [libapr-1.so.0+0x14cb8] apr_pool_destroy+0x28
C [libsvn_subr-1.so.0+0xe8fe] svn_error_clear+0x34
C [libsvnjavahl-1.so.0.0.0+0x1665b] _ZN7JNIUtil14handleSVNErrorEP11svn_error_t+0x263
C [libsvnjavahl-1.so.0.0.0+0x25058] _ZN9SVNClient8checkoutEPKcS1_R8RevisionS3_bb+0x11a
C [libsvnjavahl-1.so.0.0.0+0x33461] Java_org_tigris_subversion_javahl_SVNClient_checkout+0x1fd
j org.tigris.subversion.javahl.SVNClient.checkout(Ljava/lang/String;Ljava/lang/String;Lorg/tigris/subversion/javahl/Revision;Lorg/tigris/subversion/javahl/Revision;ZZ)J+0
...

The crash is caused when JNIUtil::handleSVNError() calls
svn_error_clear() on an svn_error_t which has a bogus pool parameter
(the culprit), a NULL "message" field, and an unknown "apr_err" field:

    ...
    jobject error = env->NewObject(clazz, mid, jmessage, jfile,
        static_cast<jint>(err->apr_err));
    printf("dying when we try to clear an svn_error_t\n");
    svn_error_clear(err);

It dies when svn_error_clear() calls apr_pool_destroy(err->pool)
internally. This feels like a bug in the C++ code of the JavaHL
bindings -- can anyone see what's producing this bogus error struct?

> I did not see anywhere in the code where it checks for an
> empty string on any API. Is the checkout API the only one that would have
> this problem? Do the other SVN API's do their own checks internally and
> this one does not?

Subversion core APIs don't typically check for NULL. The 'svn'
command-line program does the check which defaults "no destination
path specified" to mean "the current directory".

> It is kind of too bad that the code is not really setup so that these sort
> of validations do not just take place in the Java layer. I guess I feel
> that way because I know Java and not really C++ so would rather make the
> change there, but there is also the unnecesary JNI calls.
>
> It looks like this change would have to take place in the C++ code though.