perf(swig): release Python GIL, skip JNI detach, use critical array access#82
perf(swig): release Python GIL, skip JNI detach, use critical array access#82
Conversation
…itical array access
📝 WalkthroughWalkthroughUpdated SWIG bindings for Coraza across Java and Python. Java typemaps now use critical array access for byte data, callback trampolines employ daemon thread attachment without detaching, and Python API functions release the GIL during execution for improved concurrency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR targets binding-layer performance improvements for Python and Java SWIG bindings without changing the public API, aiming to improve concurrency and reduce per-call overhead in high-throughput use cases.
Changes:
- Java: switch byte[]→(data,length) typemap to
GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical. - Java: adjust callback trampolines to attach threads as daemons and avoid per-callback
DetachCurrentThread. - Python: add SWIG
%threaddirectives to release the GIL around key Go-crossing functions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $1 = (unsigned char *)JCALL2(GetPrimitiveArrayCritical, jenv, $input, NULL); | ||
| $2 = (int)JCALL1(GetArrayLength, jenv, $input); | ||
| } | ||
| %typemap(argout) (const unsigned char *data, int length) { | ||
| JCALL3(ReleaseByteArrayElements, jenv, $input, (jbyte *)$1, JNI_ABORT); | ||
| JCALL3(ReleasePrimitiveArrayCritical, jenv, $input, (jbyte *)$1, JNI_ABORT); |
There was a problem hiding this comment.
Using GetPrimitiveArrayCritical here keeps the Java byte[] pinned until the typemap(argout) runs (i.e., after the Go call returns). That can block/stall JVM GC for the full duration of request/response body processing, and the current code also calls GetArrayLength while the array is in a critical section (contradicting the comment that no other JNI calls may occur). Prefer a non-critical access pattern (e.g., GetByteArrayElements / GetByteArrayRegion) and ensure any critical region is entered only after length is known and held for the shortest possible time.
| /* Attach as a daemon if needed and leave the thread attached. Daemon | ||
| * threads are cleaned up by the JVM on exit, so DetachCurrentThread is | ||
| * unnecessary and adds ~microseconds of JVM registry overhead per call. */ | ||
| if ((*jctx->jvm)->GetEnv(jctx->jvm, (void **)&env, JNI_VERSION_1_6) == JNI_EDETACHED) | ||
| (*jctx->jvm)->AttachCurrentThreadAsDaemon(jctx->jvm, (void **)&env, NULL); |
There was a problem hiding this comment.
This callback trampoline now attaches threads to the JVM but never detaches them. JNI generally requires DetachCurrentThread before a native thread exits to avoid leaking JVM-side thread resources; with Go creating/destroying OS threads over time, this can accumulate. If the goal is to avoid per-callback attach/detach overhead, consider caching attachment per thread and detaching via a thread-exit hook (or keep the previous detach-once-attached behavior).
| (*env)->CallVoidMethod(env, jctx->obj, jctx->mid, (jlong)(uintptr_t)rule); | ||
| if ((*env)->ExceptionCheck(env)) (*env)->ExceptionDescribe(env); |
There was a problem hiding this comment.
After CallVoidMethod, ExceptionDescribe is invoked but the exception is not cleared. With the new “leave the thread attached” behavior, a pending exception can persist on this JNIEnv and cause subsequent JNI calls on the same thread to fail. Clear the exception (and consider returning early) after reporting it.
| JNIEnv *env = NULL; | ||
| jboolean attached = JNI_FALSE; | ||
| if ((*jctx->jvm)->GetEnv(jctx->jvm, (void **)&env, JNI_VERSION_1_6) == JNI_EDETACHED) { | ||
| 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; | ||
| } | ||
| jstring jmsg = (*env)->NewStringUTF(env, msg ? msg : ""); |
There was a problem hiding this comment.
GetEnv/AttachCurrentThreadAsDaemon results aren’t checked here. If GetEnv returns JNI_EVERSION/JNI_ERR or Attach fails, env may remain NULL and will be dereferenced immediately. Please handle the non-OK cases and bail out safely if an env cannot be obtained.
| @@ -243,7 +242,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); | |||
There was a problem hiding this comment.
As in the error trampoline, ExceptionDescribe does not clear the pending exception. Since the thread may remain attached across callbacks now, leaving the exception pending can break later callbacks on the same thread. Clear the exception after describing it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coraza.i`:
- Line 367: The wrappers coraza_append_request_body and
coraza_append_response_body use a typemap that extracts a raw pointer via
PyByteArray_AS_STRING and therefore must not release the GIL with %thread;
either remove the %thread directive for these two functions or modify the call
so you make a GIL-protected copy of the bytearray buffer (e.g., convert/copy the
contents to a new bytes/heap buffer while the GIL is held) before any C.GoBytes
or memcpy occurs; ensure the fix targets the typemap usage around
PyByteArray_AS_STRING and the subsequent C.GoBytes usage so the pointer cannot
be invalidated by another Python thread.
- Around line 221-225: The code calls
(*jctx->jvm)->AttachCurrentThreadAsDaemon(jctx->jvm, (void **)&env, NULL)
without checking its return value and then dereferences env; change this to
store the return code, only use env if the attach returned JNI_OK, and handle
attach failures (e.g., log/error-out or avoid dereferencing env); also ensure
any successful attach is paired with (*jctx->jvm)->DetachCurrentThread() on
native thread exit to avoid leaking JVM thread state (update the misleading
comment about daemon threads and DetachCurrentThread accordingly); apply the
same checks for the other AttachCurrentThreadAsDaemon usage that currently
dereferences env.
- Around line 186-195: The code opens a JNI critical section with
GetPrimitiveArrayCritical in the %typemap(in) block and then calls
GetArrayLength inside that section, violating the JNI rule that no other JNI
calls may occur while the array is pinned; fix by calling GetArrayLength
(JCALL1(GetArrayLength, jenv, $input)) before calling
JCALL2(GetPrimitiveArrayCritical, jenv, $input, NULL) so $2 (length) is
populated prior to entering the critical region, leaving
ReleasePrimitiveArrayCritical (JCALL3(ReleasePrimitiveArrayCritical, jenv,
$input, (jbyte *)$1, JNI_ABORT)) unchanged in the %typemap(argout) to release
the pinned array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| %typemap(in) (const unsigned char *data, int length) { | ||
| $1 = (unsigned char *)JCALL2(GetByteArrayElements, jenv, $input, NULL); | ||
| /* GetPrimitiveArrayCritical avoids a copy on most JVMs by pinning the | ||
| * heap array directly. The critical section ends in argout below; no | ||
| * other JNI calls may occur between Get and Release. */ | ||
| $1 = (unsigned char *)JCALL2(GetPrimitiveArrayCritical, jenv, $input, NULL); | ||
| $2 = (int)JCALL1(GetArrayLength, jenv, $input); | ||
| } | ||
| %typemap(argout) (const unsigned char *data, int length) { | ||
| JCALL3(ReleaseByteArrayElements, jenv, $input, (jbyte *)$1, JNI_ABORT); | ||
| JCALL3(ReleasePrimitiveArrayCritical, jenv, $input, (jbyte *)$1, JNI_ABORT); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine the coraza.i file around lines 186-195
find . -name "coraza.i" -type fRepository: corazawaf/libcoraza
Length of output: 73
🏁 Script executed:
#!/bin/bash
# Check the structure to understand the codebase layout
git ls-files | head -30Repository: corazawaf/libcoraza
Length of output: 613
🏁 Script executed:
#!/bin/bash
# Search for coraza_append_request_body and coraza_append_response_body
rg "coraza_append_(request|response)_body" --type cRepository: corazawaf/libcoraza
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for callback registration patterns in Go files
rg "call_debug_log_cb|call_error_cb" --type go -A 5Repository: corazawaf/libcoraza
Length of output: 1064
🏁 Script executed:
#!/bin/bash
# Read coraza.i around lines 186-195
sed -n '180,200p' coraza.iRepository: corazawaf/libcoraza
Length of output: 1062
🏁 Script executed:
#!/bin/bash
# Search for body append functions more broadly
rg "append.*body|body.*append" --type-list | head -5
rg "append.*body|body.*append" -iRepository: corazawaf/libcoraza
Length of output: 5219
🏁 Script executed:
#!/bin/bash
# Look at the Go file structure to understand callback registration
sed -n '100,180p' libcoraza/coraza.goRepository: corazawaf/libcoraza
Length of output: 3034
🏁 Script executed:
#!/bin/bash
# Check if there are any references to WAF body processing functions
rg "waf.*body|body.*waf" -iRepository: corazawaf/libcoraza
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Get the full implementation of coraza_append_request_body
rg -A 20 "//export coraza_append_request_body" libcoraza/coraza.goRepository: corazawaf/libcoraza
Length of output: 872
🏁 Script executed:
#!/bin/bash
# Get the full implementation of coraza_append_response_body
rg -A 20 "//export coraza_append_response_body" libcoraza/coraza.goRepository: corazawaf/libcoraza
Length of output: 753
🏁 Script executed:
#!/bin/bash
# Check if AppendRequestBody or AppendResponseBody can trigger callbacks
rg -A 10 "AppendRequestBody|AppendResponseBody" libcoraza/coraza.goRepository: corazawaf/libcoraza
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look at lines around where the callbacks are actually used
sed -n '250,350p' libcoraza/coraza.goRepository: corazawaf/libcoraza
Length of output: 3093
🏁 Script executed:
#!/bin/bash
# Check what the WAF does during body processing - search for callback invocations
rg "WithErrorCallback|WithDebugLogger" libcoraza/coraza.go -B 2 -A 10Repository: corazawaf/libcoraza
Length of output: 1525
🏁 Script executed:
#!/bin/bash
# Check JNI documentation about GetArrayLength in critical sections
# Also search for what actually happens during WriteRequestBody/WriteResponseBody
# Check if there's Go interface or type definition showing what WriteRequestBody does
rg "WriteRequestBody|WriteResponseBody" libcoraza/coraza.go -B 5 -A 5Repository: corazawaf/libcoraza
Length of output: 796
🏁 Script executed:
#!/bin/bash
# Check if the WAF processes/matches rules during WriteRequestBody or only during ProcessRequestBody
rg "WriteRequestBody|WriteResponseBody" -i --type go | head -20Repository: corazawaf/libcoraza
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Look at the coraza.i file more broadly to understand all the typemaps
sed -n '180,250p' coraza.iRepository: corazawaf/libcoraza
Length of output: 3522
🏁 Script executed:
#!/bin/bash
# Check if there are any other JNI calls or callback invocations that happen during body writing
# Look for what the Java callback examples do
cat examples/java/CorazaDebugLogCallback.javaRepository: corazawaf/libcoraza
Length of output: 982
🏁 Script executed:
#!/bin/bash
# Check the error callback example too
cat examples/java/CorazaErrorCallback.javaRepository: corazawaf/libcoraza
Length of output: 1038
GetArrayLength inside GetPrimitiveArrayCritical violates the JNI critical-section contract.
Line 191 calls GetArrayLength, which is a JNI function. The JNI specification prohibits any JNI calls inside a critical section opened by GetPrimitiveArrayCritical. The comment on line 187–189 even acknowledges this restriction: "no other JNI calls may occur between Get and Release."
Minimal safe fallback
%typemap(in) (const unsigned char *data, int length) {
- $1 = (unsigned char *)JCALL2(GetPrimitiveArrayCritical, jenv, $input, NULL);
$2 = (int)JCALL1(GetArrayLength, jenv, $input);
+ $1 = (unsigned char *)JCALL2(GetByteArrayElements, jenv, $input, NULL);
}
%typemap(argout) (const unsigned char *data, int length) {
- JCALL3(ReleasePrimitiveArrayCritical, jenv, $input, (jbyte *)$1, JNI_ABORT);
+ JCALL3(ReleaseByteArrayElements, jenv, $input, (jbyte *)$1, JNI_ABORT);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coraza.i` around lines 186 - 195, The code opens a JNI critical section with
GetPrimitiveArrayCritical in the %typemap(in) block and then calls
GetArrayLength inside that section, violating the JNI rule that no other JNI
calls may occur while the array is pinned; fix by calling GetArrayLength
(JCALL1(GetArrayLength, jenv, $input)) before calling
JCALL2(GetPrimitiveArrayCritical, jenv, $input, NULL) so $2 (length) is
populated prior to entering the critical region, leaving
ReleasePrimitiveArrayCritical (JCALL3(ReleasePrimitiveArrayCritical, jenv,
$input, (jbyte *)$1, JNI_ABORT)) unchanged in the %typemap(argout) to release
the pinned array.
| /* Attach as a daemon if needed and leave the thread attached. Daemon | ||
| * threads are cleaned up by the JVM on exit, so DetachCurrentThread is | ||
| * unnecessary and adds ~microseconds of JVM registry overhead per call. */ | ||
| if ((*jctx->jvm)->GetEnv(jctx->jvm, (void **)&env, JNI_VERSION_1_6) == JNI_EDETACHED) | ||
| (*jctx->jvm)->AttachCurrentThreadAsDaemon(jctx->jvm, (void **)&env, NULL); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "coraza.i" -type fRepository: corazawaf/libcoraza
Length of output: 73
🏁 Script executed:
cat -n coraza.i | sed -n '210,245p'Repository: corazawaf/libcoraza
Length of output: 2131
🌐 Web query:
JNI AttachCurrentThreadAsDaemon detach requirement specification
💡 Result:
Yes, a native thread attached to the Java VM using AttachCurrentThreadAsDaemon must call DetachCurrentThread before terminating. This requirement applies identically to both AttachCurrentThread (non-daemon) and AttachCurrentThreadAsDaemon (daemon), as the JNI specification treats attached native threads uniformly regarding detachment. Key points from the specification: - "A native thread attached to the VM must call DetachCurrentThread to detach itself before terminating." (Explicitly stated multiple times.) - Failure to detach can cause memory leaks, prevent VM shutdown (for non-daemon threads), or crash the VM (e.g., in older Android Dalvik). - DetachCurrentThread releases held monitors, notifies waiting threads, and marks the native thread as terminated. - Cannot detach if Java methods are on the call stack; must unwind first. - Calling on already-detached or never-attached threads is a no-op. - Daemon status (set by AttachCurrentThreadAsDaemon) affects VM shutdown waiting (VM doesn't wait for daemon threads), but detachment is still mandatory. This is unchanged in recent Java versions (up to 22+), per official docs.
Citations:
- 1: https://cr.openjdk.org/~prr/8357176/specs/jni/invocation.html
- 2: https://developer.android.com/ndk/guides/jni-tips
- 3: https://docs.oracle.com/en/java/javase/22/docs/specs/jni/invocation.html
- 4: https://docs.oracle.com/en/java/javase/21/docs/specs/jni/invocation.html
🌐 Web query:
JNI ExceptionDescribe vs ExceptionClear pending exception behavior
💡 Result:
JNI ExceptionDescribe prints the pending exception and its stack trace to stderr (or similar), and clears the pending exception as a side effect. In contrast, ExceptionClear only clears the pending exception without printing or describing it. Key behaviors with pending exceptions: - ExceptionDescribe: Describes (prints) the exception + clears it. - ExceptionClear: Clears it (no description). Official JNI spec notes: "The pending exception is cleared as a side-effect of calling this function [ExceptionDescribe]." This is consistent across Java 7, 8, 18, and 21 documentation. When a pending exception exists, safe JNI calls are limited (e.g., ExceptionOccurred, ExceptionDescribe, ExceptionClear). ExceptionDescribe is for debugging and consumes the exception by clearing it.
Citations:
- 1: https://docs.oracle.com/en/java/javase/18/docs/specs/jni/functions.html
- 2: https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/functions.html
- 3: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html
- 4: https://docs.oracle.com/en/java/javase/21/docs/specs/jni/functions.html
🌐 Web query:
JNI AttachCurrentThreadAsDaemon return value failure error handling
💡 Result:
The JNI function JavaVM::AttachCurrentThreadAsDaemon attaches the current native thread to the JVM as a daemon thread and returns a JNIEnv pointer via the p_env argument. Return value: - JNI_OK (0) on success: Thread attached successfully, p_env is set to valid JNIEnv. - Negative value (JNI error code) on failure: Common codes include: - JNI_ERR (-1): Unknown error. - JNI_EVERSION (-3): Unsupported JNI version. - JNI_ENOMEM (-4): Out of memory. - JNI_EINVAL (-6): Invalid arguments (e.g., invalid JavaVMAttachArgs). - Other implementation-specific errors. Error handling: Always check the return value before using the JNIEnv*: jint res = jvm->AttachCurrentThreadAsDaemon((void)&env, &attachArgs); if (res != JNI_OK) { // Handle failure: log error, abort operation, etc. // Do NOT use env, as it may be NULL or invalid. fprintf(stderr, "Attach failed: %d\n", res); return; // or appropriate error path } // Now safe to use env - On failure, *p_env may be NULL or invalid; never dereference it. - Common failure causes (from examples): - Insufficient thread stack space (e.g., < default Java thread stack). - Incompatible JNI_VERSION in JavaVMAttachArgs. - JVM shutting down or invalid state (late attach may return JNI_OK but invalid env->functions; fixed in later JDKs). - Thread already attached to another VM. - No exceptions thrown; errors returned via jint. - Attached threads must call DetachCurrentThread before exit to avoid leaks/crashes (Android/ART enforces strictly). - Best practice: Use GetEnv first to check attachment status before attaching. This matches AttachCurrentThread behavior.
Citations:
- 1: https://docs.oracle.com/en/java/javase/24/docs/specs/jni/invocation.html
- 2: https://docs.oracle.com/en/java/javase/24/docs/specs/jni/functions.html
- 3: https://docs.oracle.com/en/java/javase/21/docs/specs/jni/invocation.html
- 4: https://stackoverflow.com/questions/55785464/jni-attachnativethread-always-fails-with-1-return-value-on-linux
- 5: https://stackoverflow.com/questions/36486298/jni-attachcurrentthread-returns-1
- 6: https://bugs.openjdk.org/browse/JDK-8238676
Check the return value of AttachCurrentThreadAsDaemon before dereferencing env.
AttachCurrentThreadAsDaemon can fail and return negative error codes, leaving env NULL or invalid. Lines 226 and 236 dereference env immediately without checking the return value, creating a potential NULL dereference bug. Additionally, the code comment on lines 221–223 is misleading—the JNI specification explicitly requires calling DetachCurrentThread before native thread exit, even for daemon threads. Daemon status only affects VM shutdown waiting; it does not eliminate the detach requirement. Leaving threads attached can accumulate JVM thread state. Please check the attach return value, ensure proper detach on thread exit, and correct the misleading comment.
Also applies to: 234-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coraza.i` around lines 221 - 225, The code calls
(*jctx->jvm)->AttachCurrentThreadAsDaemon(jctx->jvm, (void **)&env, NULL)
without checking its return value and then dereferences env; change this to
store the return code, only use env if the attach returned JNI_OK, and handle
attach failures (e.g., log/error-out or avoid dereferencing env); also ensure
any successful attach is paired with (*jctx->jvm)->DetachCurrentThread() on
native thread exit to avoid leaking JVM thread state (update the misleading
comment about daemon threads and DetachCurrentThread accordingly); apply the
same checks for the other AttachCurrentThreadAsDaemon usage that currently
dereferences env.
| %thread coraza_process_uri; | ||
| %thread coraza_process_request_headers; | ||
| %thread coraza_process_request_body; | ||
| %thread coraza_append_request_body; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "coraza.i" | head -5Repository: corazawaf/libcoraza
Length of output: 73
🏁 Script executed:
find . -type f -path "*libcoraza/coraza.go" | head -5Repository: corazawaf/libcoraza
Length of output: 84
🏁 Script executed:
git ls-files | grep -E "(coraza\.i|coraza\.go)" | head -10Repository: corazawaf/libcoraza
Length of output: 91
🏁 Script executed:
wc -l coraza.iRepository: corazawaf/libcoraza
Length of output: 75
🏁 Script executed:
cat -n coraza.i | sed -n '60,90p'Repository: corazawaf/libcoraza
Length of output: 1292
🏁 Script executed:
cat -n coraza.i | sed -n '360,380p'Repository: corazawaf/libcoraza
Length of output: 1095
🏁 Script executed:
rg "coraza_append_request_body|coraza_append_response_body" coraza.i -A 3 -B 1Repository: corazawaf/libcoraza
Length of output: 979
🏁 Script executed:
rg "coraza_process_request_body|coraza_process_response_body" coraza.i -A 3 -B 1Repository: corazawaf/libcoraza
Length of output: 1000
🏁 Script executed:
wc -l libcoraza/coraza.goRepository: corazawaf/libcoraza
Length of output: 86
🏁 Script executed:
rg "func.*append_request_body|func.*append_response_body" libcoraza/coraza.go -A 20Repository: corazawaf/libcoraza
Length of output: 1554
🏁 Script executed:
cat -n libcoraza/coraza.go | sed -n '1,50p'Repository: corazawaf/libcoraza
Length of output: 1570
🏁 Script executed:
rg "%thread" coraza.i -B 5 -A 5 | head -40Repository: corazawaf/libcoraza
Length of output: 843
🏁 Script executed:
cat -n coraza.i | sed -n '67,90p'Repository: corazawaf/libcoraza
Length of output: 1028
🏁 Script executed:
python3 << 'EOF'
# Check Python's bytearray behavior regarding pointer validity after resize
# This is to understand the hazard
print("C.GoBytes behavior:")
print("- Makes a memcpy of data from C pointer")
print("- Happens AFTER GIL is released (due to %thread)")
print("")
print("PyByteArray_AS_STRING behavior:")
print("- Returns direct pointer to internal buffer")
print("- If bytearray is resized, pointer becomes invalid")
print("- If GIL is released, another thread can resize")
print("")
print("Race condition window:")
print("1. SWIG typemap calls PyByteArray_AS_STRING (GIL held)")
print("2. %thread decorator releases GIL")
print("3. Another Python thread resizes/reallocates bytearray")
print("4. Pointer extracted in step 1 now points to freed memory")
print("5. C.GoBytes attempts memcpy from invalid pointer")
print("6. Result: undefined behavior (crash, data corruption)")
EOFRepository: corazawaf/libcoraza
Length of output: 678
Remove %thread from coraza_append_request_body and coraza_append_response_body or copy the buffer first.
These functions use the typemap that extracts a raw pointer from bytearray via PyByteArray_AS_STRING (line 77). When %thread releases the GIL, another Python thread can resize or reallocate that same bytearray, invalidating the pointer. The subsequent C.GoBytes memcpy then reads from freed memory. PyBytes are immutable and safe; the other %thread additions take no buffer parameters and are safe.
Minimal fix
%thread coraza_process_request_body;
-%thread coraza_append_request_body;
%thread coraza_process_response_headers;
%thread coraza_process_response_body;
-%thread coraza_append_response_body;
%thread coraza_process_logging;Also applies to: 370-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coraza.i` at line 367, The wrappers coraza_append_request_body and
coraza_append_response_body use a typemap that extracts a raw pointer via
PyByteArray_AS_STRING and therefore must not release the GIL with %thread;
either remove the %thread directive for these two functions or modify the call
so you make a GIL-protected copy of the bytearray buffer (e.g., convert/copy the
contents to a new bytes/heap buffer while the GIL is held) before any C.GoBytes
or memcpy occurs; ensure the fix targets the typemap usage around
PyByteArray_AS_STRING and the subsequent C.GoBytes usage so the pointer cannot
be invalidated by another Python thread.
Summary
Three binding-layer performance improvements with no API changes:
%threaddirectives for the 11 functions that cross into Go and do real work (coraza_process_*,coraza_append_*,coraza_new_waf,coraza_rules_merge). Without this, multi-threaded Python servers serialize entirely on WAF processing even though the underlying Go code is thread-safe.DetachCurrentThread— the callback trampolines previously attached + detached on every invocation from an unattached thread (common for Go goroutines). Daemon threads are cleaned up by the JVM on exit, so detaching is unnecessary overhead (~microseconds of JVM registry work per callback).GetPrimitiveArrayCriticalfor body buffers — avoids a JVM-side copy of the byte array on JVMs that support it. The critical section is safe here because no other JNI calls occur between Get and Release.Test plan
make -C examples/python runpasses all tests locally before pushSummary by CodeRabbit