Coredump when javahl SVNClient::diff() called with diff-cmd set

[BURT, Matthew J]

Hi,

We've got a number of developers using the javahl bindings from Eclipse
via subclipse.

One of them recently reported a coredump against 1.10.3. I've also
reproduced it with 1.9.7.

The problem occurs when SVNClient::diff() is called with a diff-cmd
defined in the user's ~/.subversion/config file.

SVNClient::diff() calls svn_client_diff6() with the errstream explicitly
set to NULL and the diff command in dwi->diff_cmd.

Further down in the code, if diff_content_changed() from
subversion/libsvn_client/diff.c is called, the NULL errstream is
de-referenced with a call to svn_stream__aprfile().

I've put together a simple patch to avoid the null dereference, but I'm
not 100% sure this is the correct solution. The code suggests to me that
the intention is to allow the errstream parameter for svn_client_diff6()
to be set to NULL, but maybe I'm misreading it.

Is this the correct approach, and should I submit this patch for
consideration?

Thanks

***************************************************************************
If you are not the intended recipient, please notify our Help Desk at Email ***@nats.co.uk
immediately. You should not copy or use this email or attachment(s) for any purpose nor disclose
their contents to any other person.

NATS computer systems may be monitored and communications carried on them recorded, to
secure the effective operation of the system.

Please note that neither NATS nor the sender accepts any responsibility for viruses or any losses
caused as a result of viruses and it is your responsibility to scan or otherwise check this email
and any attachments.

NATS means NATS (En Route) plc (company number: 4129273), NATS (Services) Ltd
(company number 4129270), NATSNAV Ltd (company number: 4164590)
or NATS Ltd (company number 3155567) or NATS Holdings Ltd (company number 4138218).
All companies are registered in England and their registered office is at 4000 Parkway,
Whiteley, Fareham, Hampshire, PO15 7FL.

***************************************************************************

[Branko Čibej]

Hi,
 
We’ve got a number of developers using the javahl bindings from Eclipse
via subclipse.
 
One of them recently reported a coredump against 1.10.3. I’ve also
reproduced it with 1.9.7.
 
The problem occurs when SVNClient::diff() is called with a diff-cmd
defined in the user’s ~/.subversion/config file.
 
SVNClient::diff() calls svn_client_diff6() with the errstream explicitly
set to NULL and the diff command in dwi->diff_cmd.
 
Further down in the code, if diff_content_changed() from
subversion/libsvn_client/diff.c is called, the NULL errstream is
de-referenced with a call to svn_stream__aprfile().
 
I’ve put together a simple patch to avoid the null dereference, but I’m
not 100% sure this is the correct solution. The code suggests to me that
the intention is to allow the errstream parameter for svn_client_diff6()
to be set to NULL, but maybe I’m misreading it.
 
Is this the correct approach, and should I submit this patch for
consideration?

The docstring for svn_client_diff7 (..._diff6 was deprecated on trunk)
doesn't say that outstream or errstream may be NULL; that's pretty
definitive. This should be fixed in the native JavaHL code; either an
actual stream should be used such that JavaHL users can read it (in this
case I suspect that the JavaHL API would have to be upgraded), or a
stream that ignores all writes should be used.

-- Brane

[m***@nats.co.uk]

Post by Branko Čibej

Hi,
 
We’ve got a number of developers using the javahl bindings from Eclipse
via subclipse.
 
One of them recently reported a coredump against 1.10.3. I’ve also
reproduced it with 1.9.7.
 
The problem occurs when SVNClient::diff() is called with a diff-cmd
defined in the user’s ~/.subversion/config file.
 
SVNClient::diff() calls svn_client_diff6() with the errstream explicitly
set to NULL and the diff command in dwi->diff_cmd.
 
Further down in the code, if diff_content_changed() from
subversion/libsvn_client/diff.c is called, the NULL errstream is
de-referenced with a call to svn_stream__aprfile().
 
I’ve put together a simple patch to avoid the null dereference, but I’m
not 100% sure this is the correct solution. The code suggests to me that
the intention is to allow the errstream parameter for svn_client_diff6()
to be set to NULL, but maybe I’m misreading it.
 
Is this the correct approach, and should I submit this patch for
consideration?

The docstring for svn_client_diff7 (..._diff6 was deprecated on trunk)
doesn't say that outstream or errstream may be NULL; that's pretty
definitive. This should be fixed in the native JavaHL code; either an
actual stream should be used such that JavaHL users can read it (in this
case I suspect that the JavaHL API would have to be upgraded), or a
stream that ignores all writes should be used.
-- Brane

OK - that's clear.

I've replaced both NULL values in the JavaHL code with an svn_stream_empty in the pool that's in-scope for the call. We've re-tested with Eclipse/subclipse and it seems to have solved the crash we're seeing. This is on 1.10.3.

I can submit a patch for trunk if you like, but I note that JavaHL on trunk is still using ..._diff6.

[Branko Čibej]

Post by m***@nats.co.uk

Post by Branko Čibej

Hi,
 
We’ve got a number of developers using the javahl bindings from Eclipse
via subclipse.
 
One of them recently reported a coredump against 1.10.3. I’ve also
reproduced it with 1.9.7.
 
The problem occurs when SVNClient::diff() is called with a diff-cmd
defined in the user’s ~/.subversion/config file.
 
SVNClient::diff() calls svn_client_diff6() with the errstream explicitly
set to NULL and the diff command in dwi->diff_cmd.
 
Further down in the code, if diff_content_changed() from
subversion/libsvn_client/diff.c is called, the NULL errstream is
de-referenced with a call to svn_stream__aprfile().
 
I’ve put together a simple patch to avoid the null dereference, but I’m
not 100% sure this is the correct solution. The code suggests to me that
the intention is to allow the errstream parameter for svn_client_diff6()
to be set to NULL, but maybe I’m misreading it.
 
Is this the correct approach, and should I submit this patch for
consideration?

The docstring for svn_client_diff7 (..._diff6 was deprecated on trunk)
doesn't say that outstream or errstream may be NULL; that's pretty
definitive. This should be fixed in the native JavaHL code; either an
actual stream should be used such that JavaHL users can read it (in this
case I suspect that the JavaHL API would have to be upgraded), or a
stream that ignores all writes should be used.
-- Brane

OK - that's clear.
I've replaced both NULL values in the JavaHL code with an svn_stream_empty in the pool that's in-scope for the call. We've re-tested with Eclipse/subclipse and it seems to have solved the crash we're seeing. This is on 1.10.3.
I can submit a patch for trunk if you like, but I note that JavaHL on trunk is still using ..._diff6.

Please do submit a patch against trunk. That JavaHL has not been updated
to ..._diff7 is not your problem. :)

-- Brane