JNI code safety issues in LibrustzcashJNIImpl and LibsodiumJNIImpl

[Federico2014]

Description

During a comprehensive code review of the JNI native layer, multiple critical code quality and safety issues were identified in LibrustzcashJNIImpl.cpp and LibsodiumJNIImpl.cpp. These issues could lead to:

• Memory leaks - Improper JNI array release on error paths
• Undefined behavior - Null pointer dereference due to incorrect check ordering
• Build failures - Unused headers causing compilation errors on macOS
• Poor maintainability - C-style casts and inconsistent code patterns

Environment

Network: N/A (native library code)

Software Versions

OS: macOS 24.6.0, Linux
JVM: JDK 8+
Git Commit: 834f0da
Version: 1.0.0
Code: zksnark-java-sdk

Configuration

• CMake 3.10.2+
• Rust (cargo) 1.50.0+
• Native library: libzksnarkjni.jnilib / libzksnarkjni.so

Expected Behavior

1. JNI native code should properly release array elements on all error paths
2. Null pointer checks should execute BEFORE function calls that dereference pointers
3. C++ code should use modern C++ style (nullptr, reinterpret_cast, etc.)
4. Build should succeed on all supported platforms (macOS x86_64/aarch64, Linux)

Actual Behavior

1. Memory leaks on error paths - When GetByteArrayElements returns null for only some arrays, previously acquired arrays are not released
2. Null check after use - In librustzcashToScalar, the null check executes after the function call, so it never protects against null dereference
3. Build failures on macOS - Unused #include <iostream> causes 'iostream' file not found error
4. Compilation errors with modern JDKs - Missing const_cast when releasing const jbyte* arrays

Frequency

• Always (100%) - Code defects exist in all executions
• Frequently (>50%)
• Sometimes (10-50%)
• Rarely (<10%)

Steps to Reproduce

Issue 1: Null Check After Function Call (librustzcashToScalar)

// Line 837-845 (original code)
const unsigned char * i = (const unsigned char *) env->GetByteArrayElements(input, nullptr);
unsigned char * r = (unsigned char *) env->GetByteArrayElements(result, nullptr);
librustzcash_to_scalar(i, r);           // Called FIRST
if (r == NULL || i == NULL) {           // Check AFTER - never executes!
    return;
}

Reproduction: Trigger GetByteArrayElements to return null (e.g., invalid array), the function will crash.

Issue 2: Memory Leak on Error Path

// Multiple functions (original code)
const unsigned char * s = reinterpret_cast<const unsigned char *>(env->GetByteArrayElements(seed, nullptr));
unsigned char * x = reinterpret_cast<unsigned char *>(env->GetByteArrayElements(xsk_master, nullptr));
if (s == NULL || x == NULL) {
    return;  // Leaks s if x is null, leaks x if s is null!
}

Reproduction: Pass one invalid array and one valid array - the valid array's native reference leaks.

Issue 3: Build Failure on macOS

cd cpp && mkdir build && cd build
cmake ..
make -j4
# Error: 'iostream' file not found

Additional Context

Possible Solution

The fix involves:

1. Replace unused headers: <iostream> → <cstring> and <new>
2. Fix null check ordering: Move null checks BEFORE function calls
3. Add proper error cleanup: Release acquired arrays on all error paths with JNI_ABORT
4. Use modern C++ casts: Replace C-style casts with reinterpret_cast, static_cast, const_cast
5. Use nullptr instead of NULL
6. Add const_cast for JNI array release: Required when releasing const jbyte* with JNI_ABORT
7. Use new (std::nothrow): Prevent unhandled C++ exceptions in JNI code