-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add openssl thread synchronization to fix crashes #381
Conversation
Pls create a better commit comment. https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53 Although it may be too long for the commit summary, the body should mention the full context of the test: openjdk jdk_net_0 com/sun/net/httpserver/Test9a.java The copyright check is failing, the IBM copyright end date should be 2020. |
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
260b179
to
e352b52
Compare
@pshipton Thanks, the code is updated.
|
Calling |
334b453
to
0cc044c
Compare
@pshipton The code is updated.
|
jenkins test sanity.openjdk win,win32,xlinux,plinux,zlinux,aix,osx jdk8 |
I've started a cross platform test. Note the commit comment needs to be updated, see https://chris.beams.io/posts/git-commit/ |
There are some "Access is denied" failures, although they may occur before these changes. I'll run a build to check. Build without these changes to check Windows failures. |
plinux didn't successfully start the testing, try it again (edit: it passed on restart) jenkins test sanity.openjdk plinux jdk8 |
The build without these changes has the same Windows failures. |
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
The plinux sanity.openjdk run passed (on ub16p8j91), but I'll try more runs to ensure the problem is gone. These all passed. |
0cc044c
to
ec86cd0
Compare
FYI @pshipton @keithc-ca code updated |
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
ec86cd0
to
8539995
Compare
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
8539995
to
276af02
Compare
@keithc-ca code updated |
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
07cb847
to
005e633
Compare
Jenkins test sanity.openjdk plinux,win,win32 jdk8 |
The test failure seems unrelated to this. Unfortunately no core files were included in the test output archive.
|
@shanchao95 I think this is good. Would you please put together equivalent pull requests for jdk11, jdk14 and jdk(next). |
The test failure #381 (comment) is likely a dup of eclipse-openj9/openj9#8652 |
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
4db2063
to
bb98052
Compare
Jenkins test sanity.openjdk xlinux,win,win32 jdk8 |
The xlinux failures appear to be eclipse-openj9/openj9#8899. |
bb98052
to
feeabca
Compare
Jenkins test sanity.openjdk xlinux,win32 jdk8 |
@ashbm5 @pshipton Any final comments before this is merged? |
I'd also like PRs opened against the openj9-0.20.0 branches for the 0.20.0 release, although we can wait a couple days for these changes to soak in the head stream before merging them. |
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
closed/adds/jdk/src/share/native/jdk/crypto/jniprovider/NativeCrypto.c
Outdated
Show resolved
Hide resolved
Openssl 1.0.2 is not safely used in multi-thread application. The change adds threadid and locking callback functions to provide thread-safety when openssl 1.0.2 is used. It fixes issue 8373, test crash in openjdk, jdk_net_0, com/sun/net/httpserver/Test9a.java Signed-off-by: Chao Shan <chao.shan@ibm.com>
feeabca
to
131edad
Compare
@keithc-ca code updated |
Jenkins compile xlinux,win32 jdk8 |
if (NULL == lock_cs[i]) { | ||
fprintf(stderr, "CreateMutex error: %d\n", GetLastError()); | ||
for (j = 0; j < i; j++) { | ||
BOOL releaseResult = ReleaseMutex(lock_cs[j]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intialOwner parameter of CreateMutex()
was passed FALSE
, so we don't need to call ReleaseMutex
here, instead we should call CloseHandle
as we do in JNI_OnUnLoad()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think you're right. I'll open PRs to fix that.
Openssl 1.0.2 is not safely used in multi-thread application.
The change adds threadid and locking callback functions to provide
thread-safety when openssl 1.0.2 is used. It fixes issue eclipse-openj9/openj9#8373,
test crash in openjdk, jdk_net_0,
com/sun/net/httpserver/Test9a.java