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

FileDescriptor writev core dump #7507

Merged
merged 1 commit into from Dec 15, 2017

Conversation

Projects
None yet
4 participants
@Scottmitch
Member

Scottmitch commented Dec 14, 2017

Motivation:
FileDescriptor#writev calls JNI code, and that JNI code dereferences a NULL pointer which crashes the application. This occurs when writing a single CompositeByteBuf object with more than one component.

Modifications:

  • Initialize the iovec iterator properly to avoid the core dump
  • Fix the array length calculation if we aren't able to fit all the ByteBuffer objects in the iovec array

Result:
No more core dump.

@Scottmitch Scottmitch added the defect label Dec 14, 2017

@Scottmitch Scottmitch added this to the 4.1.19.Final milestone Dec 14, 2017

@Scottmitch Scottmitch self-assigned this Dec 14, 2017

@Scottmitch Scottmitch requested a review from normanmaurer Dec 14, 2017

@Scottmitch Scottmitch requested a review from carl-mastrangelo Dec 14, 2017

@@ -163,13 +163,13 @@ static jlong netty_unix_filedescriptor_writev(JNIEnv* env, jclass clazz, jint fd
(*env)->DeleteLocalRef(env, bufObj);
}
} else if (limitFieldId != NULL) {
for (i = offset; i < num; i++) {
for (i = offset; i < num; ++i) {
jobject bufObj = (*env)->GetObjectArrayElement(env, buffers, i);
jint pos = (*env)->CallIntMethod(env, bufObj, posId, NULL);
jint limit = (*env)->GetIntField(env, bufObj, limitFieldId);

This comment has been minimized.

@todor-m

todor-m Dec 14, 2017

What if you have a pending exception? Will this code work without calling ExceptionOccurred/ExceptionClear?

@todor-m

todor-m Dec 14, 2017

What if you have a pending exception? Will this code work without calling ExceptionOccurred/ExceptionClear?

This comment has been minimized.

@Scottmitch

Scottmitch Dec 14, 2017

Member

We typically don't call ExceptionOccurred in JNI unless there is a specific need to. We generally let the exceptions be thrown (if any occurred) after transitioning back from JNI to Java.

Do you see a problem with this approach on this line/method, or just a general question?

@Scottmitch

Scottmitch Dec 14, 2017

Member

We typically don't call ExceptionOccurred in JNI unless there is a specific need to. We generally let the exceptions be thrown (if any occurred) after transitioning back from JNI to Java.

Do you see a problem with this approach on this line/method, or just a general question?

This comment has been minimized.

@todor-m

todor-m Dec 14, 2017

The exception will be thrown when you return from the method (if I remember correctly). I think JNI will keep returning 0s (or some other garbage) if you don't clear the exception. Think about what can happen when pos and limit are 0s (or a bad bufObj).

@todor-m

todor-m Dec 14, 2017

The exception will be thrown when you return from the method (if I remember correctly). I think JNI will keep returning 0s (or some other garbage) if you don't clear the exception. Think about what can happen when pos and limit are 0s (or a bad bufObj).

This comment has been minimized.

@todor-m

todor-m Dec 14, 2017

Are you accounting for the case when JNI will start returning bad values because of an exception?

@todor-m

todor-m Dec 14, 2017

Are you accounting for the case when JNI will start returning bad values because of an exception?

This comment has been minimized.

@todor-m

todor-m Dec 14, 2017

It is more like a general question since it applies to all the JNI calls in netty, not that particular one.

@todor-m

todor-m Dec 14, 2017

It is more like a general question since it applies to all the JNI calls in netty, not that particular one.

This comment has been minimized.

@Scottmitch

Scottmitch Dec 14, 2017

Member

Lets consider this as a separate issue as the behavior w.r.t to JNI exception hasn't changed in this PR and scope is larger. It may boil down to how paranoid/defensive do we want to be. Perhaps we should check the state before we make the system call.

@Scottmitch

Scottmitch Dec 14, 2017

Member

Lets consider this as a separate issue as the behavior w.r.t to JNI exception hasn't changed in this PR and scope is larger. It may boil down to how paranoid/defensive do we want to be. Perhaps we should check the state before we make the system call.

This comment has been minimized.

@Scottmitch

Scottmitch Dec 14, 2017

Member

Thanks for bringing this up @todor-m ... see #7508

@Scottmitch

Scottmitch Dec 14, 2017

Member

Thanks for bringing this up @todor-m ... see #7508

FileDescriptor writev core dump
Motivation:
FileDescriptor#writev calls JNI code, and that JNI code dereferences a NULL pointer which crashes the application. This occurs when writing a single CompositeByteBuf object with more than one component.

Modifications:
- Initialize the iovec iterator properly to avoid the core dump
- Fix the array length calculation if we aren't able to fit all the ByteBuffer objects in the iovec array

Result:
No more core dump.

@Scottmitch Scottmitch merged commit af2f343 into netty:4.1 Dec 15, 2017

1 check passed

continuous-integration/teamcity Finished TeamCity Build pull requests :: netty : Tests passed: 13474, ignored: 199
Details

@Scottmitch Scottmitch deleted the Scottmitch:writev_core_dump branch Dec 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment