Compile natives as C instead of C++, check malloc/calloc return values for null

[lax1dude]

I’m having trouble understanding why the source files in native/src/main/c are 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.

[lax1dude]

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.

[Janmm14]

lgtm on first glance

[lax1dude]

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

[lax1dude]

@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.

[Janmm14]

Cannot reproduce junit bug you claim to have, junit does not require test method names to start with "test", there's a reason for the annotation.

This is ran on bungeecord without your PR.

[INFO] Running net.md_5.bungee.NativeZlibTest
Testing: net.md_5.bungee.jni.zlib.JavaZlib@6302bbb1
Took: 205ms
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.212 s -- in net.md_5.bungee.NativeZlibTest

maybe your maven is using some outdated, bugged surefire version (as bungee does not specify the surefire plugin's version)? My maven is atm using surefire v3.2.2

[lax1dude]

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.

[lax1dude]

Here's the output of one of my attempts before I changed the function name, I'm running the tests using mvn test

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running net.md_5.bungee.NativeCipherTest
Testing native cipher...
This cipher works correctly!
Benchmarking native cipher...
Encryption Iteration: 4096, Elapsed: 610 ms
Decryption Iteration: 4096, Elapsed: 603 ms
Testing Java cipher...
This cipher works correctly!
Benchmarking Java cipher...
Encryption Iteration: 4096, Elapsed: 852 ms
Decryption Iteration: 4096, Elapsed: 837 ms
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.1 sec
Running net.md_5.bungee.NativeZlibTest
Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0 sec

Results :

Tests run: 4, Failures: 0, Errors: 0, Skipped: 0

Am I blind?

[Janmm14]

Maybe try with mvn clean test? (If its not some outdated and possibly bugged maven-surefire-plugin issue - which might not recognize junit 5 annotation, only junit 4 or so?)

[lax1dude]

Alright, I renamed the first one back to doTest and ran mvn clean test and got this output:

Running net.md_5.bungee.NativeZlibTest
Testing Exception: net.md_5.bungee.jni.zlib.NativeZlib@192b07fd
Testing Exception: net.md_5.bungee.jni.zlib.JavaZlib@60c6f5b
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.004 sec

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

[Janmm14]

do you get this output @lax1dude I mean especially lines 1 and 2?

[INFO] --- surefire:3.2.2:test (default-test) @ bungeecord-native ---
[INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider
[INFO] 
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running net.md_5.bungee.NativeCipherTest
Testing Java cipher...
This cipher works correctly!
Benchmarking Java cipher...
Encryption Iteration: 4096, Elapsed: 314 ms
Decryption Iteration: 4096, Elapsed: 302 ms
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.763 s -- in net.md_5.bungee.NativeCipherTest
[INFO] Running net.md_5.bungee.NativeZlibTest
Testing: net.md_5.bungee.jni.zlib.JavaZlib@6302bbb1
Took: 197ms
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.203 s -- in net.md_5.bungee.NativeZlibTest
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0

[lax1dude]

Mine doesn't look like that, apparently I'm using maven-surefire-plugin:2.12.4

[INFO] --- maven-surefire-plugin:2.12.4:test (default-test) @ bungeecord-native ---
[INFO] Surefire report directory: /main/calder/BungeeCord2/BungeeCord/native/target/surefire-reports

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running net.md_5.bungee.NativeCipherTest
Testing native cipher...
This cipher works correctly!
Benchmarking native cipher...
Encryption Iteration: 4096, Elapsed: 585 ms
Decryption Iteration: 4096, Elapsed: 575 ms
Testing Java cipher...
This cipher works correctly!
Benchmarking Java cipher...
Encryption Iteration: 4096, Elapsed: 841 ms
Decryption Iteration: 4096, Elapsed: 837 ms
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.002 sec
Running net.md_5.bungee.NativeZlibTest
Testing Exception: net.md_5.bungee.jni.zlib.NativeZlib@192b07fd
Testing Exception: net.md_5.bungee.jni.zlib.JavaZlib@60c6f5b
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.004 sec

Results :

Tests run: 5, Failures: 0, Errors: 0, Skipped: 0

[lax1dude]

Oh god, that version is from Sep 24th 2012

https://mvnrepository.com/artifact/org.apache.maven.plugins/maven-surefire-plugin/2.12.4

[Janmm14]

Oh god, that version is from Sep 24th 2012

https://mvnrepository.com/artifact/org.apache.maven.plugins/maven-surefire-plugin/2.12.4

surefire-plugin 2.22 has introduced junit 5 support

[lax1dude]

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

[Janmm14]

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

Add this to parent pom in <pluginsManagement>

            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-surefire-plugin</artifactId>
                <version>3.3.0</version>
            </plugin>

Maven with its implicit definition of default plugins / option to just not set the version to use is not doing a good job with reproducibility it otherwise seeks to have (as the actually used plugin version is entirely dependant on the local maven repository cache).

[lax1dude]

Yeah that fixed it, it's running both tests now

[INFO] --- maven-surefire-plugin:3.2.5:test (default-test) @ bungeecord-native ---
[INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider
[INFO]
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running net.md_5.bungee.NativeCipherTest
Testing Java cipher...
This cipher works correctly!
Benchmarking Java cipher...
Encryption Iteration: 4096, Elapsed: 719 ms
Decryption Iteration: 4096, Elapsed: 635 ms
Testing native cipher...
This cipher works correctly!
Benchmarking native cipher...
Encryption Iteration: 4096, Elapsed: 635 ms
Decryption Iteration: 4096, Elapsed: 575 ms
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.733 s -- in net.md_5.bungee.NativeCipherTest
[INFO] Running net.md_5.bungee.NativeZlibTest
Testing Exception: net.md_5.bungee.jni.zlib.NativeZlib@429bffaa
Testing Exception: net.md_5.bungee.jni.zlib.JavaZlib@57d7f8ca
Testing: net.md_5.bungee.jni.zlib.NativeZlib@76c3e77a
Took: 299ms
Testing: net.md_5.bungee.jni.zlib.JavaZlib@53fdffa1
Took: 298ms
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.617 s -- in net.md_5.bungee.NativeZlibTest
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0

[lax1dude]

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

[Janmm14]

@lax1dude I created separate PR for this: #3701