[PATCH] Fwd: JNI segfault while running Java tests

[Daniel Sahlberg <da...@gmail.com>]

Just a kind ping on this patch. I realise I mark the subject of the
original message so maybe it went under the radar.

I also forgot the log message:
[[[
* subversion/bindings/javahl/native/jniwrapper/jni_string_map.hpp
  (Java::BaseImmutableMap::Entry::key): Create the String::Contents object
with a projer object since it keeps a reference throughout the life of
String::Contents. Fixes https://svn.haxx.se/dev/archive-2020-08/0010.shtml
]]]

The patch itself is the same as James verified.

Kind regards
Daniel

---------- Forwarded message ---------
Från: Daniel Sahlberg <da...@gmail.com>
Date: sön 9 aug. 2020 kl 23:10
Subject: Re: JNI segfault while running Java tests
To: Nathan Hartman <ha...@gmail.com>
Cc: Subversion Development <de...@subversion.apache.org>


Den sön 9 aug. 2020 kl 15:28 skrev Daniel Sahlberg <
daniel.l.sahlberg@gmail.com>:

> Den sön 9 aug. 2020 15:14Nathan Hartman <ha...@gmail.com> skrev:
>
>> On Sat, Aug 8, 2020 at 1:23 PM James McCoy <ja...@jamessan.com> wrote:
>>
>>> On Sat, Aug 08, 2020 at 10:35:14AM -0400, James McCoy wrote:
>>> > The Debian builds for 1.14.0 recently started crashing while running
>>> the
>>> > Java tests.  This is pretty far out of my expertise, so hopefully
>>> > someone can help out.
>>
>>
>> I don't know if it's related, but a few days ago we received a patch from
>> Alexandr Miloslavskiy to fix a crash which is caused by a garbage collected
>> object not being pinned before use by native code [1]. Perhaps Alexandr
>> found the issue because of a similar crash to the one you're experiencing.
>> Could you try the patch?
>>
>
> I thought about the same. However the patch didn't seem to make a
> difference.
>
> I can confirm James' statement that it crashes when compiled using GCC 10
> but it seems to work with GCC 9. In my case I'm using Fedora 32 versus
> Fedora 30 so I can't rule out that there are other differences but it seems
> reasonable that GCC is doing something strange.
>
> I have some done some preliminary investigations but I'm done yet, it
> seems that the code is using an object that has already been destructed.
>
> Kind regards
> Daniel
>

I have investigated further and I think I have found the issue. A patch is
attached, basically changing
    const String::Contents key(String(m_env, jkey));
to
    const String str(m_env, jkey);
    const String::Contents key(str);
in ImmutableMap.for_each.

If I understand things correctly (admittedly I'm not an expert in C++), the
lifetime of the String object is just the execution of the constructor of
the Contents class. But the Contents class saves a reference to the String
object in a member variable. When the Contents object is destroyed at the
end of the function, it references the already previously destroyed String
object.

This is the same in GCC 9 as well as GCC 10 (also the same in Visual Studio
2019!) so I'm guessing that GCC 10 is better at "cleaing up" destroyed
object to the point where it trigger a segfault (but it's not consistent as
a "minimal example" with GCC 10 show this behaviour but still doesn't
segfault).

When the String object is assigned to it's own variable it lives until the
end of the function and it is destroyed after the Contents object, thus the
destructor of the Contents class succeeds.

With this patch make check-javahl succeeds with GCC 10. I have also applied
it in my GCC 9 build and all checks still succeed.

Kind regards
Daniel

Re: [PATCH] Fwd: JNI segfault while running Java tests

[Nathan Hartman <ha...@gmail.com>]

On Sun, Aug 16, 2020 at 9:11 AM Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>
> Nathan Hartman wrote on Sun, 16 Aug 2020 00:47 -0400:
> > On Fri, Aug 14, 2020 at 9:08 AM Daniel Sahlberg
> > <da...@gmail.com> wrote:
> > > Just a kind ping on this patch. I realise I mark the subject of the original message so maybe it went under the radar.
> >
> > Committed in r1880886.
> >
>
> Backport nomination?

Nominated for backport to 1.14.x and 1.10.x.

Cheers,
Nathan

Re: [PATCH] Fwd: JNI segfault while running Java tests

[Daniel Shahaf <d....@daniel.shahaf.name>]

Nathan Hartman wrote on Sun, 16 Aug 2020 00:47 -0400:
> On Fri, Aug 14, 2020 at 9:08 AM Daniel Sahlberg
> <da...@gmail.com> wrote:
> > Just a kind ping on this patch. I realise I mark the subject of the original message so maybe it went under the radar.  
> 
> Committed in r1880886.
> 

Backport nomination?

Cheers,

Daniel


> Thanks for your patch and good catch on this subtle error!
> 
> The patch looks good to me. Although I don't have the setup to observe
> the crash, I studied the code and I agree with the analysis (yours and
> Alexandr's) in the other thread [1].
> 
> Thanks again,
> Nathan
> 
> [1] "JNI segfault while running Java tests" started by James McCoy on
> 2020/08/08 archived at:
> https://lists.apache.org/thread.html/rff3fa5ea97267adf36cc9daa3be01392e53fbc8050a91df4bbac01d8%40%3Cdev.subversion.apache.org%3E

Re: [PATCH] Fwd: JNI segfault while running Java tests

[Nathan Hartman <ha...@gmail.com>]

On Fri, Aug 14, 2020 at 9:08 AM Daniel Sahlberg
<da...@gmail.com> wrote:
> Just a kind ping on this patch. I realise I mark the subject of the original message so maybe it went under the radar.

Committed in r1880886.

Thanks for your patch and good catch on this subtle error!

The patch looks good to me. Although I don't have the setup to observe
the crash, I studied the code and I agree with the analysis (yours and
Alexandr's) in the other thread [1].

Thanks again,
Nathan

[1] "JNI segfault while running Java tests" started by James McCoy on
2020/08/08 archived at:
https://lists.apache.org/thread.html/rff3fa5ea97267adf36cc9daa3be01392e53fbc8050a91df4bbac01d8%40%3Cdev.subversion.apache.org%3E