Skip to content
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

Handle failure in ffi_closure_alloc #1378

Conversation

@DaveCTurner
Copy link
Contributor

@DaveCTurner DaveCTurner commented Aug 30, 2021

ffi_closure_alloc may fail and return NULL if, for instance, we're
running in a locked-down operating system that forbids FFI from
allocating executable pages of memory in any of the ways that it tries.
Today we pass this NULL on to ffi_prep_closure_loc which triggers a
segmentation fault that takes down the whole JVM. With this change we
check for a failure in this call and turn it into an
UnsupportedOperationException so that the caller can handle it more
gracefully.

Relates elastic/elasticsearch#73309
Relates elastic/elasticsearch#18272
Closes #1107

@DaveCTurner
Copy link
Contributor Author

@DaveCTurner DaveCTurner commented Aug 30, 2021

I had a bit of a look for tests of similar things but didn't see anything obvious, nor can I think of good ways to test this change. Some guidance would be appreciated.

@dkocher
Copy link
Contributor

@dkocher dkocher commented Aug 30, 2021

Looks like a fix for #1107.

@matthiasblaesing
Copy link
Member

@matthiasblaesing matthiasblaesing commented Sep 5, 2021

Change looks good to me. Thank you.

I agree testing this will be difficult to impossible. You could startup a virtual machine, lock that down, test to run JNA in that setup and observe what happens. I think that the balance between effort and results would tip in a bad way, so I think review is enough here.

Could please also have a look at:

jna/native/dispatch.c

Lines 3515 to 3535 in a40db1b

JNIEXPORT jlong JNICALL
Java_com_sun_jna_Native_ffi_1prep_1closure(JNIEnv *env, jclass UNUSED(cls), jlong cif, jobject obj)
{
callback* cb = (callback *)malloc(sizeof(callback));
ffi_status s;
if ((*env)->GetJavaVM(env, &cb->vm) != JNI_OK) {
throwByName(env, EUnsatisfiedLink, "Can't get Java VM");
return 0;
}
cb->object = (*env)->NewWeakGlobalRef(env, obj);
cb->closure = ffi_closure_alloc(sizeof(ffi_closure), L2A(&cb->x_closure));
s = ffi_prep_closure_loc(cb->closure, L2A(cif), &closure_handler,
cb, cb->x_closure);
if (ffi_error(env, "ffi_prep_cif", s)) {
return 0;
}
return A2L(cb);
}

Lines 3527 and 3529 look pretty similar, so I think a guard should be placed there also.

@DaveCTurner
Copy link
Contributor Author

@DaveCTurner DaveCTurner commented Sep 6, 2021

Lines 3527 and 3529 look pretty similar, so I think a guard should be placed there also.

Sure, I think bb75249 does the right thing here.

native/dispatch.c Show resolved Hide resolved
@matthiasblaesing
Copy link
Member

@matthiasblaesing matthiasblaesing commented Sep 12, 2021

Eyeballed this and looks good to me. To get this in please squash the changes into a single commit. After merge I'll see which architectures I manage to rebuild.

`ffi_closure_alloc` may fail and return `NULL` if, for instance, we're
running in a locked-down operating system that forbids FFI from
allocating executable pages of memory in any of the ways that it tries.
Today we pass this `NULL` on to `ffi_prep_closure_loc` which triggers a
segmentation fault that takes down the whole JVM. With this change we
check for a failure in this call and turn it into an
`UnsupportedOperationException` so that the caller can handle it more
gracefully.

Relates elastic/elasticsearch#73309
Relates elastic/elasticsearch#18272
@DaveCTurner DaveCTurner force-pushed the 2021-08-30-ffi_closure_alloc-returns-NULL branch from 5299bd5 to 3f5e937 Sep 13, 2021
@DaveCTurner
Copy link
Contributor Author

@DaveCTurner DaveCTurner commented Sep 13, 2021

Sure thing. The squashed commit is 3f5e937.

@matthiasblaesing matthiasblaesing merged commit 4ebb64a into java-native-access:master Sep 14, 2021
9 checks passed
@matthiasblaesing
Copy link
Member

@matthiasblaesing matthiasblaesing commented Sep 14, 2021

Thank you.

@DaveCTurner DaveCTurner deleted the 2021-08-30-ffi_closure_alloc-returns-NULL branch Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants