From 437c8d34dacde0e3cc50c8706cf9bc290ff089a2 Mon Sep 17 00:00:00 2001 From: "Andreas Fankhauser hiddenalpha.ch" <23085769+hiddenalpha@users.noreply.github.com> Date: Thu, 9 Nov 2023 22:55:48 +0100 Subject: [PATCH 1/4] Add more error handling in writeBytes. - Throw NullPointerException (in place of crashing) if caller passes null buffer. - Pass-through OutOfMemoryError if GetByteArrayElements() fails to alloc memory. Cherry-picked from 5abdd451099e5749b92afc8fc189e8afca2985a2. --- src/main/cpp/_nix_based/jssc.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/cpp/_nix_based/jssc.cpp b/src/main/cpp/_nix_based/jssc.cpp index c650e8d50..71ab80e6b 100644 --- a/src/main/cpp/_nix_based/jssc.cpp +++ b/src/main/cpp/_nix_based/jssc.cpp @@ -530,8 +530,14 @@ JNIEXPORT jboolean JNICALL Java_jssc_SerialNativeInterface_setDTR */ JNIEXPORT jboolean JNICALL Java_jssc_SerialNativeInterface_writeBytes (JNIEnv *env, jobject, jlong portHandle, jbyteArray buffer){ + if( buffer == NULL ){ + jclass exClz = env->FindClass("java/lang/NullPointerException"); + if( exClz != NULL ) env->ThrowNew(exClz, "buffer"); + return 0; + } jboolean ret = JNI_FALSE; jbyte* jBuffer = env->GetByteArrayElements(buffer, JNI_FALSE); + if( jBuffer == NULL ) return 0; jint bufferSize = env->GetArrayLength(buffer); jint result = write(portHandle, jBuffer, (size_t)bufferSize); if( result == -1 ){ From 768f32f656a01aee2805f7b4defff3bd42be1d51 Mon Sep 17 00:00:00 2001 From: "Andreas Fankhauser hiddenalpha.ch" <23085769+hiddenalpha@users.noreply.github.com> Date: Fri, 10 Nov 2023 02:06:18 +0100 Subject: [PATCH 2/4] I was really surprised to find that GetByteArrayElements seems to just return NULL, without any code that would prepare a java exception. In JNI doc I read that an exception gets setup if a JNI function fails. (Read "https://www.developer.com/open-source/exception-handling-in-jni/" to see what I mean) But for whatever reason this seems not to be the case for GetByteArrayElements(). - writeBytes() calls GetByteArrayElements(). - GetByteArrayElements() calls NEW_C_HEAP_ARRAY_RETURN_NULL(). - NEW_C_HEAP_ARRAY_RETURN_NULL() calls NEW_C_HEAP_ARRAY3(). - NEW_C_HEAP_ARRAY3() calls AllocateHeap(). - AllocateHeap() uses malloc() to get memory. Direct Links: GetByteArrayElements: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/prims/jni.cpp#L3597 call to NEW_C_HEAP_ARRAY_RETURN_NULL: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/prims/jni.cpp#L3611 NEW_C_HEAP_ARRAY_RETURN_NULL: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/memory/allocation.hpp#L680 call to NEW_C_HEAP_ARRAY3: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/memory/allocation.hpp#L681 NEW_C_HEAP_ARRAY3: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/memory/allocation.hpp#L668 call to AllocateHeap: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/memory/allocation.hpp#L669 AllocateHeap: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/memory/allocation.inline.hpp#L52 Picked from 4a805a7902cf0c572f7237661f2684cf9f557f0b. --- src/main/cpp/_nix_based/jssc.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/cpp/_nix_based/jssc.cpp b/src/main/cpp/_nix_based/jssc.cpp index 71ab80e6b..7dc8a6584 100644 --- a/src/main/cpp/_nix_based/jssc.cpp +++ b/src/main/cpp/_nix_based/jssc.cpp @@ -537,7 +537,11 @@ JNIEXPORT jboolean JNICALL Java_jssc_SerialNativeInterface_writeBytes } jboolean ret = JNI_FALSE; jbyte* jBuffer = env->GetByteArrayElements(buffer, JNI_FALSE); - if( jBuffer == NULL ) return 0; + if( jBuffer == NULL ){ + jclass exClz = env->FindClass("java/lang/RuntimeException"); + if( exClz != NULL ) env->ThrowNew(exClz, "jni->GetByteArrayElements() failed"); + return 0; + } jint bufferSize = env->GetArrayLength(buffer); jint result = write(portHandle, jBuffer, (size_t)bufferSize); if( result == -1 ){ From 16b8a33f8512afb72f8daec81d36cd164d7c6037 Mon Sep 17 00:00:00 2001 From: "Andreas Fankhauser hiddenalpha.ch" <23085769+hiddenalpha@users.noreply.github.com> Date: Tue, 14 Nov 2023 20:28:42 +0100 Subject: [PATCH 3/4] Add same null checks for windoof. Add unit test for the null buffer case Picked from 86aad7e512ad3b9d1f551286bc4dc8d2413720ab. --- src/main/cpp/windows/jssc.cpp | 12 ++++++++++- .../java/jssc/SerialNativeInterfaceTest.java | 20 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/main/cpp/windows/jssc.cpp b/src/main/cpp/windows/jssc.cpp index bc9cfc685..4d60c9c2d 100644 --- a/src/main/cpp/windows/jssc.cpp +++ b/src/main/cpp/windows/jssc.cpp @@ -236,9 +236,19 @@ JNIEXPORT jboolean JNICALL Java_jssc_SerialNativeInterface_writeBytes HANDLE hComm = (HANDLE)portHandle; DWORD lpNumberOfBytesTransferred; DWORD lpNumberOfBytesWritten; - OVERLAPPED *overlapped = new OVERLAPPED(); jboolean returnValue = JNI_FALSE; + if( buffer == NULL ){ + jclass exClz = env->FindClass("java/lang/NullPointerException"); + if( exClz != NULL ) env->ThrowNew(exClz, "buffer"); + return 0; + } jbyte* jBuffer = env->GetByteArrayElements(buffer, JNI_FALSE); + if( jBuffer == NULL ){ + jclass exClz = env->FindClass("java/lang/RuntimeException"); + if( exClz != NULL ) env->ThrowNew(exClz, "jni->GetByteArrayElements() failed"); + return 0; + } + OVERLAPPED *overlapped = new OVERLAPPED(); overlapped->hEvent = CreateEventA(NULL, true, false, NULL); if(WriteFile(hComm, jBuffer, (DWORD)env->GetArrayLength(buffer), &lpNumberOfBytesWritten, overlapped)){ returnValue = JNI_TRUE; diff --git a/src/test/java/jssc/SerialNativeInterfaceTest.java b/src/test/java/jssc/SerialNativeInterfaceTest.java index 6890aab04..68454d833 100644 --- a/src/test/java/jssc/SerialNativeInterfaceTest.java +++ b/src/test/java/jssc/SerialNativeInterfaceTest.java @@ -1,15 +1,23 @@ package jssc; +import org.junit.Before; import org.junit.Test; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.CoreMatchers.nullValue; import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; public class SerialNativeInterfaceTest { + private SerialNativeInterface testTarget; + + @Before + public void before(){ + testTarget = new SerialNativeInterface(); + } @Test public void testInitNativeInterface() { @@ -47,4 +55,16 @@ public void reportsWriteErrorsAsIOException() throws Exception { testTarget.writeBytes(fd, buf); } + @Test + public void throwsNpeIfPassedBufferIsNull() throws Exception { + long fd = 1; + byte[] buf = null; + try{ + testTarget.writeBytes(fd, buf); + fail("Where is the exception?"); + }catch( NullPointerException ex ){ + assertTrue(ex.getMessage().contains("buffer")); + } + } + } From b7d9022480d1d04432d0297805277154462fd0d7 Mon Sep 17 00:00:00 2001 From: "Andreas Fankhauser hiddenalpha.ch" <23085769+hiddenalpha@users.noreply.github.com> Date: Thu, 30 Nov 2023 22:38:05 +0100 Subject: [PATCH 4/4] Apply https://github.com/java-native/jssc/pull/152#discussion_r1394476235 Taken from 618158eb48bbf545cef4cb449cdad3c87e9a6775 . --- src/test/java/jssc/SerialNativeInterfaceTest.java | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/test/java/jssc/SerialNativeInterfaceTest.java b/src/test/java/jssc/SerialNativeInterfaceTest.java index 779acb5b0..5bbcb2c1f 100644 --- a/src/test/java/jssc/SerialNativeInterfaceTest.java +++ b/src/test/java/jssc/SerialNativeInterfaceTest.java @@ -88,17 +88,9 @@ public void throwsIllegalArgumentExceptionIfPortHandleIllegalOsx() throws Except } } - @Test + @Test(expected = java.lang.NullPointerException.class) public void throwsNpeIfPassedBufferIsNull() throws Exception { - SerialNativeInterface testTarget = new SerialNativeInterface(); - long fd = 1; - byte[] buf = null; - try{ - testTarget.writeBytes(fd, buf); - fail("Where is the exception?"); - }catch( NullPointerException ex ){ - assertTrue(ex.getMessage().contains("buffer")); - } + new SerialNativeInterface().writeBytes(1, null); } }