Skip to content

perf: fix JNI/CGo correctness issues and reduce allocation overhead #83

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.

Sign up for GitHub

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

Open
jptosso wants to merge 5 commits into
base: main
Choose a base branch
from
Open

perf: fix JNI/CGo correctness issues and reduce allocation overhead #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a993251
fix(swig/java): get array length before entering JNI critical section
jptosso Apr 3, 2026
3e737f1
fix(swig/java): check AttachCurrentThreadAsDaemon return, detach on t…
jptosso Apr 3, 2026
9c1fb8e
perf(swig/python): release GIL for Go-crossing calls, excluding body …
jptosso Apr 3, 2026
bf91e7c
perf(cgo): replace C.GoBytes with unsafe.Slice for zero-copy body writes
jptosso Apr 3, 2026
4336cd1
perf(cgo): hoist read buffer out of loop in coraza_request_body_from_…
jptosso Apr 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 66 additions & 16 deletions coraza.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,11 +184,13 @@ int coraza_set_debug_log_callback(coraza_waf_config_t cfg, PyObject *cb) {
%typemap(jstype) (const unsigned char *data, int length) "byte[]"
%typemap(javain) (const unsigned char *data, int length) "$javainput"
%typemap(in) (const unsigned char *data, int length) {
$1 = (unsigned char *)JCALL2(GetByteArrayElements, jenv, $input, NULL);
/* GetArrayLength must be called before GetPrimitiveArrayCritical: no other
* JNI calls are permitted while a critical section is open. */
$2 = (int)JCALL1(GetArrayLength, jenv, $input);
$1 = (unsigned char *)JCALL2(GetPrimitiveArrayCritical, jenv, $input, NULL);
}
%typemap(argout) (const unsigned char *data, int length) {
JCALL3(ReleaseByteArrayElements, jenv, $input, (jbyte *)$1, JNI_ABORT);
JCALL3(ReleasePrimitiveArrayCritical, jenv, $input, (jbyte *)$1, JNI_ABORT);
}
Comment on lines 186 to 194

@coderabbitai coderabbitai Bot Apr 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "coraza.i" -o -name "coraza.go" | head -20

Repository: corazawaf/libcoraza

Length of output: 95

🏁 Script executed:

sed -n '180,200p' ./coraza.i

Repository: corazawaf/libcoraza

Length of output: 1051

🏁 Script executed:

sed -n '295,305p' ./libcoraza/coraza.go

Repository: corazawaf/libcoraza

Length of output: 509

🏁 Script executed:

sed -n '318,328p' ./libcoraza/coraza.go

Repository: corazawaf/libcoraza

Length of output: 403

🏁 Script executed:

sed -n '1,50p' ./libcoraza/coraza.go | grep -A 5 "WriteRequestBody\|WriteResponseBody"

Repository: corazawaf/libcoraza

Length of output: 45

🏁 Script executed:

grep -n "GetPrimitiveArrayCritical" ./coraza.i

Repository: corazawaf/libcoraza

Length of output: 231

🏁 Script executed:

sed -n '186,210p' ./coraza.i

Repository: corazawaf/libcoraza

Length of output: 1130

🏁 Script executed:

sed -n '175,200p' ./coraza.i

Repository: corazawaf/libcoraza

Length of output: 1137

🏁 Script executed:

rg -n "coraza_append_request_body|coraza_append_response_body" ./coraza.i -A 10

Repository: corazawaf/libcoraza

Length of output: 1933

🏁 Script executed:

rg -n "NULL" ./coraza.i | head -30

Repository: corazawaf/libcoraza

Length of output: 768

🏁 Script executed:

sed -n '186,210p' ./coraza.i && echo "---" && sed -n '290,310p' ./libcoraza/coraza.go

Repository: corazawaf/libcoraza

Length of output: 1851

Guard GetPrimitiveArrayCritical against NULL and prevent JNI callback re-entrancy during array pinning.

Line 190 does not check if GetPrimitiveArrayCritical returns NULL. In Go 1.20+, unsafe.Slice(nil, length) where length > 0 panics; non-empty input will hit this path in libcoraza/coraza.go lines 299 and 322. Additionally, the array remains pinned from line 190 to the argout release, and if WriteRequestBody or WriteResponseBody trigger Java callbacks (error or debug logging), those callbacks invoke JNI while the critical section is open, violating JNI rules. Either check for NULL and fail fast, or use a copying approach to avoid holding the critical section across callbacks.


/*
Expand All @@ -212,28 +214,59 @@ typedef struct {
jmethodID mid;
} _swig_java_cb_ctx_t;

/*
* Obtain a JNIEnv for the current thread, attaching it as a daemon if needed.
*
* Attachment is cached per OS thread via __thread storage so repeated calls on
* the same thread skip the GetEnv round-trip. Attached threads are detached
* automatically by _swig_jni_detach_thread, which is registered as a
* pthread destructor on first attachment and fires when the OS thread exits.
*
* Returns NULL if attachment fails (JVM overloaded or shutting down); callers
* must check before dereferencing env.
*/
#include <pthread.h>

static pthread_key_t _swig_jni_tls_key;
static pthread_once_t _swig_jni_tls_once = PTHREAD_ONCE_INIT;

static void _swig_jni_detach_thread(void *jvm_ptr) {
if (jvm_ptr)
((JavaVM *)jvm_ptr)->DetachCurrentThread((JavaVM *)jvm_ptr);
}
Comment on lines +233 to +236

@coderabbitai coderabbitai Bot Apr 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, locate the coraza.i file
find . -name "coraza.i" -type f

Repository: corazawaf/libcoraza

Length of output: 73

🏁 Script executed:

# Read the coraza.i file around lines 233-236
sed -n '230,250p' coraza.i

Repository: corazawaf/libcoraza

Length of output: 797

🏁 Script executed:

# Search for other JNI invocation patterns to understand the code style
rg 'DetachCurrentThread|AttachCurrentThread' -B 2 -A 2 coraza.i

Repository: corazawaf/libcoraza

Length of output: 582

Use the C JNI invocation form in _swig_jni_detach_thread.

Line 235 uses C++ member-access syntax (->) on a cast JavaVM pointer. The correct C JNI ABI requires dereferencing the function pointer structure: (*jvm)->MethodName(jvm, ...). This matches the pattern used in _swig_ensure_jni_env and is required for C compilation.

Proposed fix 
 static void _swig_jni_detach_thread(void *jvm_ptr) {
-    if (jvm_ptr)
-        ((JavaVM *)jvm_ptr)->DetachCurrentThread((JavaVM *)jvm_ptr);
+    if (jvm_ptr) {
+        JavaVM *jvm = (JavaVM *)jvm_ptr;
+        (*jvm)->DetachCurrentThread(jvm);
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static void _swig_jni_detach_thread(void *jvm_ptr) {
if (jvm_ptr)
((JavaVM *)jvm_ptr)->DetachCurrentThread((JavaVM *)jvm_ptr);
}
static void _swig_jni_detach_thread(void *jvm_ptr) {
if (jvm_ptr) {
JavaVM *jvm = (JavaVM *)jvm_ptr;
(*jvm)->DetachCurrentThread(jvm);
}
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@coraza.i` around lines 233 - 236, The C++-style call uses member access on a
JavaVM pointer; change _swig_jni_detach_thread to use the C JNI invocation form:
cast jvm_ptr to JavaVM * (e.g. JavaVM *jvm = (JavaVM *)jvm_ptr;), check jvm,
then call the function via the function-pointer table:
(*jvm)->DetachCurrentThread(jvm); referencing _swig_jni_detach_thread and
JavaVM/DetachCurrentThread to locate where to make this change.


static void _swig_jni_tls_init(void) {
pthread_key_create(&_swig_jni_tls_key, _swig_jni_detach_thread);
}

static JNIEnv *_swig_ensure_jni_env(JavaVM *jvm) {
JNIEnv *env = NULL;
jint rc = (*jvm)->GetEnv(jvm, (void **)&env, JNI_VERSION_1_6);
if (rc == JNI_OK)
return env;
if (rc != JNI_EDETACHED)
return NULL; /* JVM error — caller must handle NULL */
if ((*jvm)->AttachCurrentThreadAsDaemon(jvm, (void **)&env, NULL) != JNI_OK)
return NULL;
/* Register DetachCurrentThread to run when this OS thread exits. */
pthread_once(&_swig_jni_tls_once, _swig_jni_tls_init);
pthread_setspecific(_swig_jni_tls_key, jvm);
return env;
}

static void _swig_java_error_trampoline(void *ctx, coraza_matched_rule_t rule) {
_swig_java_cb_ctx_t *jctx = (_swig_java_cb_ctx_t *)ctx;
JNIEnv *env = NULL;
jboolean attached = JNI_FALSE;
if ((*jctx->jvm)->GetEnv(jctx->jvm, (void **)&env, JNI_VERSION_1_6) == JNI_EDETACHED) {
(*jctx->jvm)->AttachCurrentThreadAsDaemon(jctx->jvm, (void **)&env, NULL);
attached = JNI_TRUE;
}
JNIEnv *env = _swig_ensure_jni_env(jctx->jvm);
if (!env) return;
(*env)->CallVoidMethod(env, jctx->obj, jctx->mid, (jlong)(uintptr_t)rule);
if ((*env)->ExceptionCheck(env)) (*env)->ExceptionDescribe(env);
if (attached) (*jctx->jvm)->DetachCurrentThread(jctx->jvm);
}

static void _swig_java_debug_trampoline(void *ctx, coraza_debug_log_level_t level,
const char *msg, const char *fields) {
_swig_java_cb_ctx_t *jctx = (_swig_java_cb_ctx_t *)ctx;
JNIEnv *env = NULL;
jboolean attached = JNI_FALSE;
if ((*jctx->jvm)->GetEnv(jctx->jvm, (void **)&env, JNI_VERSION_1_6) == JNI_EDETACHED) {
(*jctx->jvm)->AttachCurrentThreadAsDaemon(jctx->jvm, (void **)&env, NULL);
attached = JNI_TRUE;
}
JNIEnv *env = _swig_ensure_jni_env(jctx->jvm);
if (!env) return;
jstring jmsg = (*env)->NewStringUTF(env, msg ? msg : "");
jstring jfields = (*env)->NewStringUTF(env, fields ? fields : "");
/* NewStringUTF returns NULL on OOM; skip the call rather than crash. */
Expand All @@ -243,7 +276,6 @@ static void _swig_java_debug_trampoline(void *ctx, coraza_debug_log_level_t leve
if (jmsg) (*env)->DeleteLocalRef(env, jmsg);
if (jfields) (*env)->DeleteLocalRef(env, jfields);
if ((*env)->ExceptionCheck(env)) (*env)->ExceptionDescribe(env);
if (attached) (*jctx->jvm)->DetachCurrentThread(jctx->jvm);
}

JNIEXPORT jint JNICALL Java_coraza_coraza_1set_1error_1callback(
Expand Down Expand Up @@ -353,7 +385,25 @@ typedef enum coraza_severity_t {

/*
* Function declarations
*
* Release the Python GIL for calls that cross into Go and do real work, so
* other Python threads can run concurrently. coraza_append_request_body and
* coraza_append_response_body are intentionally excluded: the Python typemap
* extracts a raw pointer via PyBytes_AS_STRING / PyByteArray_AS_STRING and
* that pointer must remain valid for the duration of the call — releasing the
* GIL would allow another thread to resize or deallocate the buffer.
*/
#ifdef SWIGPYTHON
%thread coraza_new_waf;
%thread coraza_rules_merge;
%thread coraza_process_connection;
%thread coraza_process_uri;
%thread coraza_process_request_headers;
%thread coraza_process_request_body;
%thread coraza_process_response_headers;
%thread coraza_process_response_body;
%thread coraza_process_logging;
#endif

extern coraza_waf_config_t coraza_new_waf_config();
extern int coraza_rules_add_file(coraza_waf_config_t c, const char *file);
Expand Down
9 changes: 5 additions & 4 deletions libcoraza/coraza.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,9 @@ func coraza_process_logging(t C.coraza_transaction_t) C.int {
//export coraza_append_request_body
func coraza_append_request_body(t C.coraza_transaction_t, data *C.uchar, length C.int) C.int {
tx := fromRaw[types.Transaction](t)
if _, _, err := tx.WriteRequestBody(C.GoBytes(unsafe.Pointer(data), length)); err != nil {
// unsafe.Slice creates a Go slice header over the C buffer without copying.
// Safe because WriteRequestBody does not retain the slice after returning.
if _, _, err := tx.WriteRequestBody(unsafe.Slice((*byte)(unsafe.Pointer(data)), length)); err != nil {
return 1
}
return 0
Expand All @@ -317,7 +319,7 @@ func coraza_add_response_header(t C.coraza_transaction_t, name *C.char, name_len
//export coraza_append_response_body
func coraza_append_response_body(t C.coraza_transaction_t, data *C.uchar, length C.int) C.int {
tx := fromRaw[types.Transaction](t)
if _, _, err := tx.WriteResponseBody(C.GoBytes(unsafe.Pointer(data), length)); err != nil {
if _, _, err := tx.WriteResponseBody(unsafe.Slice((*byte)(unsafe.Pointer(data)), length)); err != nil {
return 1
}
return 0
Expand Down Expand Up @@ -385,9 +387,8 @@ func coraza_request_body_from_file(t C.coraza_transaction_t, file *C.char) C.int
return 1
}
defer f.Close()
// we read the file in chunks and send it to the engine
buf := make([]byte, 32*1024)
for {
buf := make([]byte, 1024)
n, err := f.Read(buf)
if err != nil {
if err == io.EOF {
Expand Down
Loading