On Wed, 24 May 2006, Blair Zajac wrote:
> dlr@tigris.org wrote:
> >Author: dlr
> >Date: Wed May 24 11:04:16 2006
> >New Revision: 19803
> >
> >Modified:
> > trunk/subversion/bindings/java/javahl/native/SVNClient.cpp
> > trunk/subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientInterface.java
> >
> >Log:
> >Fix a hard JVM crash in JavaHL bindings due to a segfault in the C++
> >code which occurs when a null value is passed from Java-land into the
> >SVNClient.username() or SVNClient.password() method.
> >
> >Gory details: The JNI C code (the glue between Java and C++) creates a
> >JNIStringHolder object, which is implicitly cast to a const char *
> >when invoking the SVNClient::username() or password() method, which
> >implicitly invokes a std::string constructor which accepts a const
> >char * parameter during an "=" assignment, which ends up trying to do
> >a string length calculation on a NULL value. Boom!
>
> Would it be a good idea to put this comment and the knowledge into the code
> itself also, so that somebody who comes along and looks at it wonders why
> it's done the way it is?
I had thought about it before committing, and decided not to. The
code path is spread across a few source files, so there isn't a great
single place to document this (SVNClient.cpp seemed the best possible
compromise).
Personally, I think the code should be refactored to eliminate the
plethora of implicit method/constructor invocations, so that what the
code actually does becomes comprehensible by mere mortals (I peppered
rooneg with C++ questions on this one ;), and there's no need for
comments.
On a related note, here at CollabNet we were wondering if the
SVNClientInterface.username() and password() Java methods should even
accept null input, or should instead throw IllegalArgumentException
[1]. Setting a null username seems like a pretty bogus operation to
me, setting a null password perhaps less so. Thoughts?
If we do want to make this change, we should do it in 1.4, so it's
clearly a bug fix from a JVM crash to an exception (rather than in
1.5, where it would be a multi-release API behavior change).
--
Daniel Rall
[1] http://java.sun.com/j2se/1.4.2/docs/api/java/lang/IllegalArgumentException.html
- application/pgp-signature attachment: stored
Received on Wed May 24 22:20:05 2006