[PATCH] Fix JavaHL crash in RequestChannel.nativeRead

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix JavaHL crash in RequestChannel.nativeRead

Alexandr Miloslavskiy
Please find test snippet and patch attached.

[[[
Fix JavaHL crash in RequestChannel.nativeRead

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.

One other problem is that when 'TunnelAgent.openTunnel()' throws an
exception, 'OperationContext::closeTunnel()' was not called at all.

[in subversion/bindings/javahl]
* native/OperationContext.cpp
   (close_TunnelChannel): New function to inform Java side.
   (openTunnel): Keep references to Java tunnel objects.
   (openTunnel): In case of exception, clean up properly.
   (closeTunnel): Inform Java side when tunnel is closed.
* src/org/apache/subversion/javahl/util/RequestChannel.java
   Add 'synchronized' to avoid error described in 'syncClose()'
* 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.
]]]

patch.txt (12K) Download Attachment
JavaHL_Crash_RequestChannel_nativeRead.java (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix JavaHL crash in RequestChannel.nativeRead

Nathan Hartman
On Mon, Aug 10, 2020 at 3:44 PM Alexandr Miloslavskiy
<[hidden email]> wrote:

>
> Please find test snippet and patch attached.
>
> [[[
> Fix JavaHL crash in RequestChannel.nativeRead
>
> 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.
>
> One other problem is that when 'TunnelAgent.openTunnel()' throws an
> exception, 'OperationContext::closeTunnel()' was not called at all.
>
> [in subversion/bindings/javahl]
> * native/OperationContext.cpp
>    (close_TunnelChannel): New function to inform Java side.
>    (openTunnel): Keep references to Java tunnel objects.
>    (openTunnel): In case of exception, clean up properly.
>    (closeTunnel): Inform Java side when tunnel is closed.
> * src/org/apache/subversion/javahl/util/RequestChannel.java
>    Add 'synchronized' to avoid error described in 'syncClose()'
> * 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.
> ]]]

Ping...

Just want to make sure this patch isn't lost. It looks reasonable to
me but I'm not an expert in this area. Could someone review it please?

Also there's one other patch I'll ping in a moment...

Thanks,
Nathan
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix JavaHL crash in RequestChannel.nativeRead

Thomas Singer (SyntEvo)
In reply to this post by Alexandr Miloslavskiy
Has this patch been merged yet? If not, what input is needed to get it
accepted?

Tom


On 2020-08-10 21:30, Alexandr Miloslavskiy wrote:

> Please find test snippet and patch attached.
>
> [[[
> Fix JavaHL crash in RequestChannel.nativeRead
>
> 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.
>
> One other problem is that when 'TunnelAgent.openTunnel()' throws an
> exception, 'OperationContext::closeTunnel()' was not called at all.
>
> [in subversion/bindings/javahl]
> * native/OperationContext.cpp
>    (close_TunnelChannel): New function to inform Java side.
>    (openTunnel): Keep references to Java tunnel objects.
>    (openTunnel): In case of exception, clean up properly.
>    (closeTunnel): Inform Java side when tunnel is closed.
> * src/org/apache/subversion/javahl/util/RequestChannel.java
>    Add 'synchronized' to avoid error described in 'syncClose()'
> * 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.
> ]]]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix JavaHL crash in RequestChannel.nativeRead

Nathan Hartman
On Wed, Sep 9, 2020 at 7:39 AM Thomas Singer <[hidden email]> wrote:
Has this patch been merged yet? If not, what input is needed to get it
accepted?

Hi Tom,

This and the other JavaHL patch are waiting for someone to review/test them.

I was hoping one of our devs would come forward to do that but I know that some people have been too busy with other work lately. We definitely need more help here.

I'll try to get my system setup to run the JavaHL tests, which has been on my to-do list for some time anyway. If I can get that to work and verify the changes don't introduce regressions, then we'll go from there.

Thanks,
Nathan

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix JavaHL crash in RequestChannel.nativeRead

Alexandr Miloslavskiy-2
On 10.09.2020 22:27, Nathan Hartman wrote:
> I'll try to get my system setup to run the JavaHL tests, which has been
> on my to-do list for some time anyway. If I can get that to work and
> verify the changes don't introduce regressions, then we'll go from there.

If I'm not missing something, macOS/Linux steps would be:
1) Download [1] JDK and extract it to some folder, for example
/home/user/Downloads/jdk. I have tested with versions 8 and 14, haven't
yet tried the newest version 15.
2) Download [2] JUnit and put .jar to some folder, for example
/home/user/Downloads/junit.
3) During 'configure' SVN build step, pass additional arguments:
    --with-jdk=/home/user/Downloads/jdk
    --with-junit=/home/user/Downloads/junit
    --enable-javahl
4) Compile with:
    make javahl
5) Run tests with:
    make check-apache-javahl

Didn't try running tests on Windows yet.

Please note that with JDK14+, a JNI error becomes visible, which I
patched in [3].

[1] https://www.oracle.com/java/technologies/javase-jdk14-downloads.html
[2] https://repo1.maven.org/maven2/junit/junit/4.12/junit-4.12.jar
[3]
https://mail-archives.apache.org/mod_mbox/subversion-dev/202009.mbox/browser
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix JavaHL crash in RequestChannel.nativeRead

Alexandr Miloslavskiy-2
>     --with-junit=/home/user/Downloads/junit

Correction: this argument should point to .jar file, like:
--with-junit=/home/user/Downloads/junit/junit-4.12.jar
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix JavaHL crash in RequestChannel.nativeRead

Alexandr Miloslavskiy
In reply to this post by Alexandr Miloslavskiy
Now available on branch 'javahl-1.14-fixes', r1882523 + r1882525