Compile natives as C instead of C++, check malloc/calloc return values for null #3693
Conversation
"GetByteArrayElements" will usually copy the array out of the JVM's heap to a temporary buffer first for safety before allowing it to be accessed, which is pointless if all you're doing is preparing arguments for a memcpy. "GetByteArrayRegion" can memcpy the data in the array directly from the JVM heap to a destination address without allocating a useless intermediate copy first.
|
Edit: I improved NativeCipherImpl.c to only require 1 pointer to be malloc'd instead of 2 per context by making the key variable part of the crypto_context struct instead of a separate pointer. Unit tests are still passing for me locally. |
|
lgtm on first glance |
The code was not throwing a DataFormatException on invalid data, also the reason those JNI pointers "can't be static for some unknown reason" is most likely due to garbage collection of the "NativeCodeException" class, which I fixed by calling the constructor through a helper function in "NativeCompressImpl.java" instead.
I discovered unit tests for zlib weren't being run because JUnit was ignoring the "doTest" function because it's name doesn't start with "test". Also I added a new test to ensure a DataFormatException is being thrown on invalid data without crashing.
|
Edit: I improved the code that throws the OutOfMemoryError so failing to load "java/lang/OutOfMemoryError" doesn't cause it to crash from a null pointer, since if the proxy is low on memory it may fail to correctly load any new classes. Edit: I improved the native zlib library to correctly throw a "java.util.zip.DataFormatException" on invalid data like the Java one, and fixed the throwException function to not need to call FindClass and GetMethodID again every time it's called Edit: JUnit wasn't running any tests on zlib because the function was called "doTest" instead of "test*", I fixed the incorrectly named function and also added a second test to the class make sure a DataFormatException is thrown when random bytes are decompressed |
|
@md-5 Any thoughts? Just to be clear, a failure to properly check the return value of malloc/calloc is an actual bug, there’s no such thing as exceptions in C so error handling for malloc has to be done manually by checking the return value every single time. It’s probably best not to have the proxy dereferencing null pointers when it runs out of memory. Also, the unit tests for zlib aren’t being run because the test function was named incorrectly. |
This comment was marked as resolved.
This comment was marked as resolved.
|
That's weird, for me it said "Tests run: 0, Failures: 0, Errors: 0, Skipped: 0" every time for NativeZlibTest until I renamed it to start with "test", is there a reason JUnit would ever skip a test and not report it skipped? I'm not an expert with JUnit. |
|
Here's the output of one of my attempts before I changed the function name, I'm running the tests using Am I blind? |
This comment was marked as resolved.
This comment was marked as resolved.
|
Alright, I renamed the first one back to Doesn't seem to be picking up the "doTest", I don't know what I could've done to mess up the config files, git doesn't say I have any changed files |
This comment was marked as resolved.
This comment was marked as resolved.
|
Mine doesn't look like that, apparently I'm using |
|
Oh god, that version is from Sep 24th 2012 https://mvnrepository.com/artifact/org.apache.maven.plugins/maven-surefire-plugin/2.12.4 |
This comment was marked as resolved.
This comment was marked as resolved.
|
Sorry I don't know that much about maven, how do I make it use the new version, I don't see it in pom.xml |
This comment was marked as resolved.
This comment was marked as resolved.
|
Yeah that fixed it, it's running both tests now |
|
It looks like github actions is having the same issue I was having with maven tests using a really outdated version of surefire (2.12.4), I can see in the Java 8 build it only ran my exception test case, it did not run the main compress/decompress test case named "doTest" on the natives module and did not report it as skipped either: https://github.com/SpigotMC/BungeeCord/actions/runs/9588586031/job/26557479620?pr=3693#step:5:507 |
SpigotMC#3693: Compile natives as C instead of C++, check malloc/calloc retur…
I’m having trouble understanding why the source files in
native/src/main/care being compiled as C++. They were written in C, no features from C++ are actually used besides implicitly passing the JNIEnv as first arg. Memory allocation is still done via C's malloc/free, and the header files used are C's “stdlib.h” and “string.h” instead of C++’s “cstdlib” and “cstring”. ZLib and mbed-tls are C libraries, they won’t benefit from being linked as a C++ program or being mixed with C++ source code in these specific files, all compiling and linking as C++ is really doing with these libraries is increase the size of the final executable.I’ve renamed these files from .CPP to .C to avoid confusion and modified the build script to use gcc to compile and link them instead of g++. I compiled the SO files and ran the unit tests for the natives module and they all passed with no issues.
While reading the code I also noticed the return value of malloc/calloc isn’t being checked in several places, the code should always check the return value of these functions for null because that’s what gets returned when the program runs out of memory, which obviously can (and will) happen at any time. I’ve added code to use the JNI to throw an OutOfMemoryError if any of these memory allocations results in a null pointer to make it easier for people to debug.
If my change from C++ to C is not accepted I’ll start a new PR to only add these null checks since null pointers caused by a memory shortage in this code currently leads to a generic “EXCEPTION_ACCESS_VIOLATION” JVM crash which isn’t traditionally associated with running out of memory by most server owners.