From aa14249b24361526594be313007931d32b06134f Mon Sep 17 00:00:00 2001 From: buongarzoni Date: Thu, 30 Jul 2026 01:37:52 -0300 Subject: [PATCH 1/5] fix(native-agent): request JVMTI 1.2 instead of the build JDK's version --- native-agent/src/env.rs | 98 ++++++++++++++++++++++++++++++++--------- 1 file changed, 77 insertions(+), 21 deletions(-) diff --git a/native-agent/src/env.rs b/native-agent/src/env.rs index 46bfc106..33460522 100644 --- a/native-agent/src/env.rs +++ b/native-agent/src/env.rs @@ -1,8 +1,8 @@ +use std::ffi::CStr; use std::mem::size_of; -use std::os::raw::{c_uchar, c_void}; +use std::os::raw::{c_char, c_uchar, c_void}; use std::ptr; -use c_on_exception; use errors::*; macro_rules! jvmtifn ( @@ -23,9 +23,8 @@ macro_rules! jvmtifn ( use jvmti::{ jclass, jdouble, jfloat, jint, jlong, jmethodID, jobject, jthread, jvmtiCapabilities, jvmtiEnv, - jvmtiError_JVMTI_ERROR_NONE, jvmtiEventCallbacks, jvmtiEventMode_JVMTI_ENABLE, - jvmtiEvent_JVMTI_EVENT_EXCEPTION, jvmtiFrameInfo, jvmtiLocalVariableEntry, JavaVM, JNI_ERR, - JNI_OK, JVMTI_VERSION, + jvmtiError_JVMTI_ERROR_NONE, jvmtiEvent, jvmtiEventCallbacks, jvmtiEventMode, jvmtiFrameInfo, + jvmtiLocalVariableEntry, JavaVM, JNI_ERR, JNI_OK, JVMTI_VERSION_1_2, }; /// JvmTiEnv is a wrapper around the JVMTI environment which is obtained @@ -47,10 +46,14 @@ impl JvmTiEnv { let mut penv: *mut c_void = ptr::null_mut(); let rc; unsafe { + // Request 1.2 explicitly rather than the JVMTI_VERSION baked in by bindgen + // at build time: that constant tracks the JDK the agent was *compiled* + // against, so a JDK 21 build would be rejected with JNI_EVERSION by a + // JDK 17 runtime. Nothing here needs anything newer than 1.2. rc = (**vm).GetEnv.expect("GetEnv function not found")( vm, &mut penv, - JVMTI_VERSION as i32, + JVMTI_VERSION_1_2 as i32, ); } if rc as u32 != JNI_OK { @@ -74,31 +77,84 @@ impl JvmTiEnv { jvmtifn!(self.jvmti, AddCapabilities, &capabilities) } - pub fn set_exception_handler(&mut self) -> Result<()> { - let callbacks = jvmtiEventCallbacks { - Exception: Some(c_on_exception), - ..Default::default() - }; - - // This binding is necessary, the type checker can't handle ? here - let a: Result<()> = jvmtifn!( + /// Registers the whole callback table in one shot. `SetEventCallbacks` replaces + /// the entire struct, so every callback the agent will ever use must be passed + /// here; delivery is then controlled independently via `set_event_mode`. + pub fn set_event_callbacks(&mut self, callbacks: &jvmtiEventCallbacks) -> Result<()> { + jvmtifn!( self.jvmti, SetEventCallbacks, - &callbacks, + callbacks, size_of::() as i32 - ); - if a.is_err() { - return a; - } + ) + } + + /// Enables or disables one event globally (all threads). + pub fn set_event_mode(&mut self, mode: jvmtiEventMode, event: jvmtiEvent) -> Result<()> { jvmtifn!( self.jvmti, SetEventNotificationMode, - jvmtiEventMode_JVMTI_ENABLE, - jvmtiEvent_JVMTI_EVENT_EXCEPTION, + mode, + event, ptr::null_mut() ) } + /// Compares a class signature without allocating a Rust `String`. This runs for + /// every class the VM prepares until the agent arms, so it stays on the cheap path. + pub fn class_signature_is(&mut self, klass: jclass, expected: &CStr) -> Result { + let mut sig: *mut c_char = ptr::null_mut(); + // The macro needs a typed binding; `?` alone cannot infer the error type. + let rc: Result<()> = jvmtifn!( + self.jvmti, + GetClassSignature, + klass, + &mut sig, + ptr::null_mut() + ); + rc?; + if sig.is_null() { + return Ok(false); + } + let matches = unsafe { CStr::from_ptr(sig) } == expected; + let _ = self.dealloc(sig); + Ok(matches) + } + + /// Returns a class signature (e.g. `Lcom/foo/Bar;`) as owned bytes. + pub fn get_class_signature(&mut self, klass: jclass) -> Result> { + let mut sig: *mut c_char = ptr::null_mut(); + let rc: Result<()> = jvmtifn!( + self.jvmti, + GetClassSignature, + klass, + &mut sig, + ptr::null_mut() + ); + rc?; + if sig.is_null() { + return Ok(Vec::new()); + } + let owned = unsafe { CStr::from_ptr(sig) }.to_bytes().to_vec(); + let _ = self.dealloc(sig); + Ok(owned) + } + + /// The defining loader of `klass`, or null for the bootstrap loader. + pub fn get_class_loader(&mut self, klass: jclass) -> Result { + let mut loader: jobject = ptr::null_mut(); + let rc: Result<()> = jvmtifn!(self.jvmti, GetClassLoader, klass, &mut loader); + rc?; + Ok(loader) + } + + pub fn get_method_modifiers(&mut self, method: jmethodID) -> Result { + let mut modifiers: jint = 0; + let rc: Result<()> = jvmtifn!(self.jvmti, GetMethodModifiers, method, &mut modifiers); + rc?; + Ok(modifiers) + } + pub fn get_frame_count(&mut self, thread: jthread) -> Result { let mut result: jint = 0; let rc; From 170ffe507c22c595f5bc74ddb0e5c36a7b36750e Mon Sep 17 00:00:00 2001 From: buongarzoni Date: Thu, 30 Jul 2026 01:38:51 -0300 Subject: [PATCH 2/5] perf(native-agent): gate activation on ClassPrepare and cache JNI handles --- native-agent/src/cache.rs | 203 ++++++++++++++++++++++++++++ native-agent/src/exceptions.rs | 184 ++++++++++++++++--------- native-agent/src/jni.rs | 236 ++++++++++++++++++++++++++++++++- native-agent/src/lib.rs | 129 +++++++++++++++++- 4 files changed, 672 insertions(+), 80 deletions(-) create mode 100644 native-agent/src/cache.rs diff --git a/native-agent/src/cache.rs b/native-agent/src/cache.rs new file mode 100644 index 00000000..20ac67f9 --- /dev/null +++ b/native-agent/src/cache.rs @@ -0,0 +1,203 @@ +//! Process-wide handle cache. +//! +//! Everything the Exception callback needs is resolved once and reused. Previously +//! each event re-ran `FindClass` and `GetStaticMethodID`, and each captured frame +//! re-resolved `CacheFrame`/`LocalVariable` plus a boxing class per primitive local. +//! +//! `jclass`/`jmethodID` are raw pointers and so not `Send`/`Sync` to Rust, but a JNI +//! *global* reference — and the method IDs of a class kept alive by one — are valid +//! on any thread for as long as the class stays loaded. The `unsafe impl`s below +//! encode exactly that. `OnceLock` supplies the acquire/release publication needed +//! for a callback on another thread to observe a fully initialised struct. + +use std::sync::OnceLock; + +use env::JvmTiEnv; +use errors::*; +use jni::JniEnv; +use jvmti::{jclass, jfieldID, jmethodID, jobject}; + +pub struct BoxType { + pub class: jclass, + pub value_of: jmethodID, +} + +/// Resolved when `com.rollbar.jvmti.ThrowableCache` is prepared. Its presence is +/// what arms the Exception event. +pub struct Core { + pub throwable_cache: jclass, + pub should_cache: jmethodID, + pub add: jmethodID, + /// Defining loader of `ThrowableCache`; sibling classes resolve through it. + pub loader: jobject, + /// `ThrowableCache.appPackagesVersion`, absent on older rollbar-java jars. + pub app_packages_version: Option, + /// `ThrowableCache.appPackagesSnapshot()`, absent on older rollbar-java jars. + pub app_packages_snapshot: Option, +} + +unsafe impl Send for Core {} +unsafe impl Sync for Core {} + +/// Resolved lazily on the first exception that actually gets captured, so apps that +/// never capture anything never pay for it. +pub struct Capture { + pub cache_frame: jclass, + pub cache_frame_ctor: jmethodID, + pub local_variable: jclass, + pub local_variable_ctor: jmethodID, + pub long_box: BoxType, + pub float_box: BoxType, + pub double_box: BoxType, + pub int_box: BoxType, + pub short_box: BoxType, + pub char_box: BoxType, + pub byte_box: BoxType, + pub bool_box: BoxType, +} + +unsafe impl Send for Capture {} +unsafe impl Sync for Capture {} + +pub static CORE: OnceLock = OnceLock::new(); +pub static CAPTURE: OnceLock = OnceLock::new(); + +/// Promotes the `ClassPrepare` class to a global ref and resolves the two static +/// methods on it. The supplied `jclass` is the real class from whichever loader +/// loaded it, which is why this works where `FindClass` does not. +pub fn arm(jvmti: &mut JvmTiEnv, jni: &mut JniEnv, klass: jclass) -> Result<()> { + let throwable_cache = jni.new_global_ref(klass)? as jclass; + + let should_cache = jni.get_static_method_id( + throwable_cache, + "shouldCacheThrowable", + "(Ljava/lang/Throwable;I)Z", + )?; + let add = jni.get_static_method_id( + throwable_cache, + "add", + "(Ljava/lang/Throwable;[Lcom/rollbar/jvmti/CacheFrame;)V", + )?; + + let loader_local = jvmti.get_class_loader(throwable_cache)?; + let loader = jni.new_global_ref(loader_local)?; + + // Both are absent on rollbar-java jars older than the version that added the + // native pre-filter; the agent then falls back to filtering in Java only. + let app_packages_version = + jni.get_static_field_id_opt(throwable_cache, "appPackagesVersion", "I"); + let app_packages_snapshot = jni.get_static_method_id_opt( + throwable_cache, + "appPackagesSnapshot", + "()[Ljava/lang/String;", + ); + if app_packages_version.is_none() || app_packages_snapshot.is_none() { + info!( + "rollbar agent: rollbar-java predates the native package filter; \ + falling back to filtering in Java" + ); + } + + let core = Core { + throwable_cache, + should_cache, + add, + loader, + app_packages_version, + app_packages_snapshot, + }; + + if let Err(duplicate) = CORE.set(core) { + // Another ClassPrepare won the race; release the refs we just took. + jni.delete_global_ref(duplicate.throwable_cache); + jni.delete_global_ref(duplicate.loader); + } + Ok(()) +} + +pub fn capture(jni: &mut JniEnv, core: &Core) -> Result<&'static Capture> { + if let Some(existing) = CAPTURE.get() { + return Ok(existing); + } + + let built = build_capture(jni, core)?; + if let Err(duplicate) = CAPTURE.set(built) { + release_capture(jni, &duplicate); + } + Ok(CAPTURE.get().expect("CAPTURE set above")) +} + +fn build_capture(jni: &mut JniEnv, core: &Core) -> Result { + let class_class = jni.find_class("java/lang/Class")?; + let for_name = jni.get_static_method_id( + class_class, + "forName", + "(Ljava/lang/String;ZLjava/lang/ClassLoader;)Ljava/lang/Class;", + )?; + + let cache_frame_local = jni.class_for_name( + class_class, + for_name, + "com.rollbar.jvmti.CacheFrame", + core.loader, + )?; + let cache_frame = jni.new_global_ref(cache_frame_local)? as jclass; + let cache_frame_ctor = jni.get_method_id( + cache_frame, + "", + "(Ljava/lang/reflect/Method;[Lcom/rollbar/jvmti/LocalVariable;)V", + )?; + + let local_variable_local = jni.class_for_name( + class_class, + for_name, + "com.rollbar.jvmti.LocalVariable", + core.loader, + )?; + let local_variable = jni.new_global_ref(local_variable_local)? as jclass; + let local_variable_ctor = jni.get_method_id( + local_variable, + "", + "(Ljava/lang/String;Ljava/lang/Object;)V", + )?; + + Ok(Capture { + cache_frame, + cache_frame_ctor, + local_variable, + local_variable_ctor, + long_box: box_type(jni, "java/lang/Long", "(J)Ljava/lang/Long;")?, + float_box: box_type(jni, "java/lang/Float", "(F)Ljava/lang/Float;")?, + double_box: box_type(jni, "java/lang/Double", "(D)Ljava/lang/Double;")?, + int_box: box_type(jni, "java/lang/Integer", "(I)Ljava/lang/Integer;")?, + short_box: box_type(jni, "java/lang/Short", "(S)Ljava/lang/Short;")?, + char_box: box_type(jni, "java/lang/Character", "(C)Ljava/lang/Character;")?, + byte_box: box_type(jni, "java/lang/Byte", "(B)Ljava/lang/Byte;")?, + bool_box: box_type(jni, "java/lang/Boolean", "(Z)Ljava/lang/Boolean;")?, + }) +} + +/// `java.lang.*` wrappers live in the bootstrap loader, so `FindClass` is safe here. +fn box_type(jni: &mut JniEnv, name: &str, signature: &str) -> Result { + let local = jni.find_class(name)?; + let class = jni.new_global_ref(local)? as jclass; + let value_of = jni.get_static_method_id(class, "valueOf", signature)?; + Ok(BoxType { class, value_of }) +} + +fn release_capture(jni: &mut JniEnv, capture: &Capture) { + jni.delete_global_ref(capture.cache_frame); + jni.delete_global_ref(capture.local_variable); + for boxed in &[ + &capture.long_box, + &capture.float_box, + &capture.double_box, + &capture.int_box, + &capture.short_box, + &capture.char_box, + &capture.byte_box, + &capture.bool_box, + ] { + jni.delete_global_ref(boxed.class); + } +} diff --git a/native-agent/src/exceptions.rs b/native-agent/src/exceptions.rs index 0857f793..e5660194 100644 --- a/native-agent/src/exceptions.rs +++ b/native-agent/src/exceptions.rs @@ -1,3 +1,5 @@ +use cache::{self, BoxType, Capture, Core}; +use filter; use env::JvmTiEnv; use errors::*; use jni::JniEnv; @@ -11,6 +13,12 @@ use jvmti::{ jvmtiFrameInfo, jvmtiLocalVariableEntry, }; +/// Upper bound on frames captured for a single throwable. Deep framework stacks +/// otherwise walk a local variable table per frame plus a JVMTI call per slot. +const MAX_FRAMES: jint = 64; + +const ACC_STATIC: jint = 0x0008; + pub fn inner_callback( mut jvmti_env: JvmTiEnv, mut jni_env: JniEnv, @@ -18,29 +26,46 @@ pub fn inner_callback( exception: jobject, ) -> Result<()> { trace!("on_exception called"); - let class = jni_env.find_class("com/rollbar/jvmti/ThrowableCache")?; - let should_cache_method = - jni_env.get_static_method_id(class, "shouldCacheThrowable", "(Ljava/lang/Throwable;I)Z")?; - let num_frames = jvmti_env.get_frame_count(thread)?; + // Not armed yet: ThrowableCache has not been prepared, so there is nothing to + // call. The Exception event should not even be enabled in this state. + let core: &Core = match cache::CORE.get() { + Some(core) => core, + None => return Ok(()), + }; - let shouldCache = - jni_env.call_static_LI_Z_method(class, should_cache_method, exception, num_frames)?; + // Cheap native reject before GetFrameCount (a stack walk) and before the Java + // upcall, whose shouldCacheThrowable materializes a full StackTraceElement[]. + match filter::decide(&mut jvmti_env, &mut jni_env, core, thread) { + filter::Decision::Skip | filter::Decision::NoAppFrame => return Ok(()), + filter::Decision::AskJava => {} + } + + let num_frames = jvmti_env.get_frame_count(thread)?; - if !shouldCache { + let should_cache = jni_env.call_static_LI_Z_method( + core.throwable_cache, + core.should_cache, + exception, + num_frames, + )?; + if !should_cache { return Ok(()); } - let cache_add_method = jni_env.get_static_method_id( - class, - "add", - "(Ljava/lang/Throwable;[Lcom/rollbar/jvmti/CacheFrame;)V", - )?; + let capture = cache::capture(&mut jni_env, core)?; let start_depth = 0; - let frames = build_stack_trace_frames(jvmti_env, jni_env, thread, start_depth, num_frames)?; + let frames = build_stack_trace_frames( + jvmti_env, + jni_env, + capture, + thread, + start_depth, + num_frames.min(MAX_FRAMES), + )?; - jni_env.call_static_LAL_V_method(class, cache_add_method, exception, frames)?; + jni_env.call_static_LAL_V_method(core.throwable_cache, core.add, exception, frames)?; trace!("on_exception exit"); Ok(()) } @@ -48,11 +73,17 @@ pub fn inner_callback( fn build_stack_trace_frames( mut jvmti_env: JvmTiEnv, mut jni_env: JniEnv, + capture: &Capture, thread: jthread, start_depth: jint, num_frames: jint, ) -> Result { - let mut frames: Vec = Vec::with_capacity(num_frames as usize); + if num_frames <= 0 { + return jni_env.new_object_array(0, capture.cache_frame, ptr::null_mut()); + } + + // GetStackTrace writes into this buffer; it is sized, not filled, up front. + let mut frames: Vec = vec![jvmtiFrameInfo::default(); num_frames as usize]; let mut num_frames_returned: jint = 0; jvmti_env.get_stack_trace( thread, @@ -61,18 +92,20 @@ fn build_stack_trace_frames( frames.as_mut_ptr(), &mut num_frames_returned, )?; - if num_frames_returned >= 0 && num_frames_returned as usize > frames.len() { - debug_assert!(num_frames_returned as usize <= frames.capacity()); - unsafe { - frames.set_len(num_frames_returned as usize); - } + if num_frames_returned < 0 { + num_frames_returned = 0; + } + if num_frames_returned as usize > frames.len() { + num_frames_returned = frames.len() as jint; } - let class = jni_env.find_class("com/rollbar/jvmti/CacheFrame")?; - let result = jni_env.new_object_array(num_frames_returned, class, ptr::null_mut())?; + + let result = + jni_env.new_object_array(num_frames_returned, capture.cache_frame, ptr::null_mut())?; for i in 0..num_frames_returned { let frame = build_frame( &mut jvmti_env, &mut jni_env, + capture, thread, start_depth + i, frames[i as usize].method, @@ -86,11 +119,21 @@ fn build_stack_trace_frames( fn build_frame( jvmti_env: &mut JvmTiEnv, jni_env: &mut JniEnv, + capture: &Capture, thread: jthread, depth: jint, method: jmethodID, location: jlocation, ) -> Result { + // Locals are only useful for the user's own code. A framework stack is mostly + // library frames, and each one otherwise costs a GetLocalVariableTable plus a + // JVMTI call and a boxing allocation per slot. The frame object is still emitted + // so the array stays 1:1 with the stack -- BodyFactory.frames() indexes it + // positionally and dereferences every element. + if !filter::is_app_frame(jvmti_env, method) { + return make_frame_object(jvmti_env, jni_env, capture, method, ptr::null_mut()); + } + let mut num_entries: jint = 0; let mut local_var_table: *mut jvmtiLocalVariableEntry = ptr::null_mut(); @@ -102,13 +145,21 @@ fn build_frame( if rc == jvmtiError_JVMTI_ERROR_ABSENT_INFORMATION as jint || rc == jvmtiError_JVMTI_ERROR_NATIVE_METHOD as jint => { - return make_frame_object(jvmti_env, jni_env, method, ptr::null_mut()); + return make_frame_object(jvmti_env, jni_env, capture, method, ptr::null_mut()); } _ => {} } return Err(e); } + // from_raw_parts requires a non-null, aligned pointer even for a zero length. + if num_entries <= 0 || local_var_table.is_null() { + if !local_var_table.is_null() { + let _ = jvmti_env.dealloc(local_var_table); + } + return make_frame_object(jvmti_env, jni_env, capture, method, ptr::null_mut()); + } + let local_entries; unsafe { local_entries = slice::from_raw_parts(local_var_table, num_entries as usize); @@ -117,11 +168,12 @@ fn build_frame( let result = gather_local_information( jvmti_env, jni_env, + capture, thread, depth, method, location, - &local_entries, + local_entries, ); for entry in local_entries { let _ = jvmti_env.dealloc(entry.name); @@ -134,60 +186,51 @@ fn build_frame( result } +#[allow(clippy::too_many_arguments)] fn gather_local_information( jvmti_env: &mut JvmTiEnv, jni_env: &mut JniEnv, + capture: &Capture, thread: jthread, depth: jint, method: jmethodID, location: jlocation, local_entries: &[jvmtiLocalVariableEntry], ) -> Result { - let local_class = jni_env.find_class("com/rollbar/jvmti/LocalVariable")?; - let ctor = jni_env.get_method_id( - local_class, - "", - "(Ljava/lang/String;Ljava/lang/Object;)V", + let locals = jni_env.new_object_array( + local_entries.len() as jsize, + capture.local_variable, + ptr::null_mut(), )?; - let locals = - jni_env.new_object_array(local_entries.len() as jsize, local_class, ptr::null_mut())?; for (i, entry) in local_entries.iter().enumerate() { make_local_variable( - jvmti_env, - jni_env, - thread, - depth, - local_class, - ctor, - location, - locals, - entry, - i as jint, + jvmti_env, jni_env, capture, thread, depth, location, locals, entry, i as jint, )?; } - make_frame_object(jvmti_env, jni_env, method, locals) + make_frame_object(jvmti_env, jni_env, capture, method, locals) } +#[allow(clippy::too_many_arguments)] fn make_local_variable( jvmti_env: &mut JvmTiEnv, jni_env: &mut JniEnv, + capture: &Capture, thread: jthread, depth: jint, - local_class: jclass, - ctor: jmethodID, location: jlocation, locals: jobjectArray, entry: &jvmtiLocalVariableEntry, index: jint, ) -> Result<()> { - let name = jni_env.new_string_utf(entry.name)?; + let in_scope = location >= entry.start_location + && location <= entry.start_location + i64::from(entry.length); - let local = if location >= entry.start_location - && location <= entry.start_location + i64::from(entry.length) - { - let value = get_local_value(jvmti_env, jni_env, thread, depth, entry)?; - jni_env.new_object_StringL(local_class, ctor, name, value)? + // Building the name string only matters for slots we actually emit. + let local = if in_scope { + let name = jni_env.new_string_utf(entry.name)?; + let value = get_local_value(jvmti_env, jni_env, capture, thread, depth, entry)?; + jni_env.new_object_StringL(capture.local_variable, capture.local_variable_ctor, name, value)? } else { ptr::null_mut() }; @@ -198,24 +241,29 @@ fn make_local_variable( fn make_frame_object( jvmti_env: &mut JvmTiEnv, jni_env: &mut JniEnv, + capture: &Capture, method: jmethodID, locals: jobjectArray, ) -> Result { let mut method_class: jclass = ptr::null_mut(); jvmti_env.get_method_declaring_class(method, &mut method_class)?; - let frame_method = jni_env.get_reflected_method(method_class, method, true)?; - let frame_class = jni_env.find_class("com/rollbar/jvmti/CacheFrame")?; - let ctor = jni_env.get_method_id( - frame_class, - "", - "(Ljava/lang/reflect/Method;[Lcom/rollbar/jvmti/LocalVariable;)V", - )?; - jni_env.new_object_LAL(frame_class, ctor, frame_method, locals) + let is_static = jvmti_env + .get_method_modifiers(method) + .map(|modifiers| modifiers & ACC_STATIC != 0) + .unwrap_or(false); + let frame_method = jni_env.get_reflected_method(method_class, method, is_static)?; + jni_env.new_object_LAL( + capture.cache_frame, + capture.cache_frame_ctor, + frame_method, + locals, + ) } fn get_local_value( jvmti_env: &mut JvmTiEnv, jni_env: &mut JniEnv, + capture: &Capture, thread: jthread, depth: jint, entry: &jvmtiLocalVariableEntry, @@ -238,42 +286,42 @@ fn get_local_value( b'J' => { let mut val: jlong = 0; jvmti_env.get_local_long(thread, depth, entry.slot, &mut val)?; - jni_env.value_of("java/lang/Long", "(J)Ljava/lang/Long;", val) + box_value(jni_env, &capture.long_box, val) } b'F' => { let mut val: jfloat = 0.0; jvmti_env.get_local_float(thread, depth, entry.slot, &mut val)?; - jni_env.value_of("java/lang/Float", "(F)Ljava/lang/Float;", val) + box_value(jni_env, &capture.float_box, val) } b'D' => { let mut val: jdouble = 0.0; jvmti_env.get_local_double(thread, depth, entry.slot, &mut val)?; - jni_env.value_of("java/lang/Double", "(D)Ljava/lang/Double;", val) + box_value(jni_env, &capture.double_box, val) } b'I' => { let mut val: jint = 0; jvmti_env.get_local_int(thread, depth, entry.slot, &mut val)?; - jni_env.value_of("java/lang/Integer", "(I)Ljava/lang/Integer;", val) + box_value(jni_env, &capture.int_box, val) } b'S' => { let mut val: jint = 0; jvmti_env.get_local_int(thread, depth, entry.slot, &mut val)?; - jni_env.value_of("java/lang/Short", "(S)Ljava/lang/Short;", val) + box_value(jni_env, &capture.short_box, val) } b'C' => { let mut val: jint = 0; jvmti_env.get_local_int(thread, depth, entry.slot, &mut val)?; - jni_env.value_of("java/lang/Character", "(C)Ljava/lang/Character;", val) + box_value(jni_env, &capture.char_box, val) } b'B' => { let mut val: jint = 0; jvmti_env.get_local_int(thread, depth, entry.slot, &mut val)?; - jni_env.value_of("java/lang/Byte", "(B)Ljava/lang/Byte;", val) + box_value(jni_env, &capture.byte_box, val) } b'Z' => { let mut val: jint = 0; jvmti_env.get_local_int(thread, depth, entry.slot, &mut val)?; - jni_env.value_of("java/lang/Boolean", "(Z)Ljava/lang/Boolean;", val) + box_value(jni_env, &capture.bool_box, val) } _ => { let message = "bad local variable signature".to_owned(); @@ -281,3 +329,7 @@ fn get_local_value( } } } + +fn box_value(jni_env: &mut JniEnv, boxed: &BoxType, val: T) -> Result { + jni_env.value_of_cached(boxed.class, boxed.value_of, val) +} diff --git a/native-agent/src/jni.rs b/native-agent/src/jni.rs index 37b6c3e4..5db53a7b 100644 --- a/native-agent/src/jni.rs +++ b/native-agent/src/jni.rs @@ -451,7 +451,7 @@ impl JniEnv { ); } if self.exception_occurred() || result.is_null() { - let message = "new string utf failed".to_owned(); + let message = "reflected method lookup failed".to_owned(); self.diagnose_exception(&message)?; bail!(ErrorKind::Jni(message)) } else { @@ -459,14 +459,236 @@ impl JniEnv { } } - pub fn value_of( + /// Boxes a primitive using an already-resolved class/`valueOf` pair. The + /// uncached `value_of` did a `FindClass` + `GetStaticMethodID` per local + /// variable, which dominated the capture path. + pub fn value_of_cached( &mut self, - class: &str, - signature: &str, + class: ::jvmti::jclass, + value_of: ::jvmti::jmethodID, val: T, ) -> Result<::jvmti::jobject> { - let reflect_class = self.find_class(class)?; - let value_of = self.get_static_method_id(reflect_class, "valueOf", signature)?; - self.call_static_object_method(reflect_class, value_of, val) + self.call_static_object_method(class, value_of, val) + } + + /// Clears any pending exception without describing it. The description path + /// (`diagnose_exception`) costs a Java upcall, so it must stay off the hot path, + /// but the clear itself is mandatory or the next JNI call misbehaves. + pub fn clear_pending_exception(&mut self) { + if !self.exception_occurred() { + return; + } + unsafe { + (**self.jni) + .ExceptionClear + .expect("ExceptionClear function not found")(self.jni); + } + } + + pub fn new_global_ref(&mut self, obj: ::jvmti::jobject) -> Result<::jvmti::jobject> { + if obj.is_null() { + return Ok(ptr::null_mut()); + } + let result; + unsafe { + result = (**self.jni) + .NewGlobalRef + .expect("NewGlobalRef function not found")(self.jni, obj); + } + if result.is_null() { + bail!(ErrorKind::Jni("NewGlobalRef failed".to_owned())) + } + Ok(result) + } + + pub fn delete_global_ref(&mut self, obj: ::jvmti::jobject) { + if obj.is_null() { + return; + } + unsafe { + (**self.jni) + .DeleteGlobalRef + .expect("DeleteGlobalRef function not found")(self.jni, obj); + } + } + + /// Bounds the local references created during one callback. Capturing a deep + /// stack creates thousands of refs; without a frame they all stay live until + /// the callback returns. + pub fn push_local_frame(&mut self, capacity: ::jvmti::jint) -> Result<()> { + let rc; + unsafe { + rc = (**self.jni) + .PushLocalFrame + .expect("PushLocalFrame function not found")(self.jni, capacity); + } + if rc != 0 { + self.clear_pending_exception(); + bail!(ErrorKind::Jni("PushLocalFrame failed".to_owned())) + } + Ok(()) + } + + pub fn pop_local_frame(&mut self) { + unsafe { + (**self.jni) + .PopLocalFrame + .expect("PopLocalFrame function not found")(self.jni, ptr::null_mut()); + } + } + + pub fn new_string_utf_str(&mut self, s: &str) -> Result<::jvmti::jstring> { + let c = CString::new(s)?; + self.new_string_utf(c.as_ptr()) + } + + /// Resolves a static field, returning `None` (rather than an error) when the + /// field is absent so the agent stays compatible with older rollbar-java jars. + pub fn get_static_field_id_opt( + &mut self, + class: ::jvmti::jclass, + name: &str, + signature: &str, + ) -> Option<::jvmti::jfieldID> { + let c_name = match CString::new(name) { + Ok(s) => s, + Err(_) => return None, + }; + let c_signature = match CString::new(signature) { + Ok(s) => s, + Err(_) => return None, + }; + let field_id; + unsafe { + field_id = (**self.jni) + .GetStaticFieldID + .expect("GetStaticFieldID function not found")( + self.jni, + class, + c_name.as_ptr(), + c_signature.as_ptr(), + ); + } + // A missing field leaves a pending NoSuchFieldError that must be cleared. + self.clear_pending_exception(); + if field_id.is_null() { + None + } else { + Some(field_id) + } + } + + /// Resolves a static method, returning `None` rather than an error when absent, + /// so the agent stays compatible with older rollbar-java jars. + pub fn get_static_method_id_opt( + &mut self, + class: ::jvmti::jclass, + method: &str, + signature: &str, + ) -> Option<::jvmti::jmethodID> { + let c_method = CString::new(method).ok()?; + let c_signature = CString::new(signature).ok()?; + let method_id = self.get_static_method_id_internal(class, &c_method, &c_signature); + // A missing method leaves a pending NoSuchMethodError that must be cleared. + self.clear_pending_exception(); + method_id + } + + pub fn get_static_int_field( + &mut self, + class: ::jvmti::jclass, + field: ::jvmti::jfieldID, + ) -> ::jvmti::jint { + unsafe { + (**self.jni) + .GetStaticIntField + .expect("GetStaticIntField function not found")(self.jni, class, field) + } + } + + pub fn call_static_object_method0( + &mut self, + class: ::jvmti::jclass, + method_id: ::jvmti::jmethodID, + ) -> Result<::jvmti::jobject> { + let result; + unsafe { + result = (**self.jni) + .CallStaticObjectMethod + .expect("CallStaticObjectMethod not found")(self.jni, class, method_id); + } + if self.exception_occurred() || result.is_null() { + let message = "static no-arg call failed".to_owned(); + self.diagnose_exception(&message)?; + bail!(ErrorKind::Jni(message)); + } + Ok(result) + } + + pub fn get_array_length(&mut self, array: ::jvmti::jarray) -> ::jvmti::jsize { + unsafe { + (**self.jni) + .GetArrayLength + .expect("GetArrayLength function not found")(self.jni, array) + } + } + + pub fn get_object_array_element( + &mut self, + array: ::jvmti::jobjectArray, + index: ::jvmti::jsize, + ) -> ::jvmti::jobject { + unsafe { + (**self.jni) + .GetObjectArrayElement + .expect("GetObjectArrayElement function not found")(self.jni, array, index) + } + } + + /// Copies a Java string out as owned bytes. + pub fn string_to_bytes(&mut self, s: ::jvmti::jstring) -> Option> { + if s.is_null() { + return None; + } + let (utf_chars, cstr) = self.get_string_utf_chars(s); + if utf_chars.is_null() { + return None; + } + let bytes = cstr.to_bytes().to_vec(); + self.release_string_utf_chars(s, utf_chars); + Some(bytes) + } + + /// `Class.forName(name, false, loader)`. Used to resolve `CacheFrame` and + /// `LocalVariable` through `ThrowableCache`'s own loader; plain `FindClass` + /// resolves against the system loader and so fails inside a Spring Boot fat jar. + pub fn class_for_name( + &mut self, + class_class: ::jvmti::jclass, + for_name: ::jvmti::jmethodID, + name: &str, + loader: ::jvmti::jobject, + ) -> Result<::jvmti::jclass> { + let j_name = self.new_string_utf_str(name)?; + let result; + unsafe { + result = (**self.jni) + .CallStaticObjectMethod + .expect("CallStaticObjectMethod not found")( + self.jni, + class_class, + for_name, + j_name, + // jboolean is u8, but C varargs promote it to int. + ::jvmti::JNI_FALSE as ::std::os::raw::c_int, + loader, + ); + } + if self.exception_occurred() || result.is_null() { + let message = format!("Class.forName({}) failed", name); + self.diagnose_exception(&message)?; + bail!(ErrorKind::Jni(message)) + } + Ok(result as ::jvmti::jclass) } } diff --git a/native-agent/src/lib.rs b/native-agent/src/lib.rs index 37d4a543..5878fa85 100644 --- a/native-agent/src/lib.rs +++ b/native-agent/src/lib.rs @@ -8,21 +8,62 @@ extern crate log; #[macro_use] extern crate error_chain; +mod cache; mod env; mod errors; mod exceptions; +mod filter; mod jni; -#[cfg_attr(feature = "cargo-clippy", allow(clippy))] mod jvmti; use env::JvmTiEnv; use jni::JniEnv; -use jvmti::{jint, jlocation, jmethodID, jobject, jthread, jvmtiEnv, JNIEnv, JavaVM}; +use jvmti::{ + jclass, jint, jlocation, jmethodID, jobject, jthread, jvmtiEnv, jvmtiEventCallbacks, + jvmtiEventMode_JVMTI_DISABLE, jvmtiEventMode_JVMTI_ENABLE, + jvmtiEvent_JVMTI_EVENT_CLASS_PREPARE, jvmtiEvent_JVMTI_EVENT_EXCEPTION, JNIEnv, JavaVM, +}; +use std::cell::Cell; +use std::ffi::CStr; use std::os::raw::{c_char, c_void}; -use std::sync::atomic::{AtomicBool, Ordering, ATOMIC_BOOL_INIT}; +use std::sync::atomic::{AtomicBool, Ordering}; -static INIT_SUCCESS: AtomicBool = ATOMIC_BOOL_INIT; +static INIT_SUCCESS: AtomicBool = AtomicBool::new(false); + +/// Number of JNI local references the capture path is expected to need before it +/// starts spilling into additional handle blocks. +const LOCAL_FRAME_CAPACITY: jint = 64; + +thread_local! { + /// Guards against the agent re-entering itself: a JNI call made from the + /// callback can itself raise a Java exception, which fires another Exception + /// event on the same thread. `const` init deliberately registers no TLS + /// destructor — these are JVM-owned threads and the crate builds with + /// `panic = "abort"`. + static IN_CALLBACK: Cell = const { Cell::new(false) }; +} + +struct ReentryGuard; + +impl ReentryGuard { + fn enter() -> Option { + IN_CALLBACK.with(|flag| { + if flag.get() { + None + } else { + flag.set(true); + Some(ReentryGuard) + } + }) + } +} + +impl Drop for ReentryGuard { + fn drop(&mut self) { + IN_CALLBACK.with(|flag| flag.set(false)); + } +} /// This is the Agent entry point that is called by the JVM during the loading phase. /// Any failures in this function will cause the JVM not to start which is strictly @@ -48,12 +89,43 @@ pub extern "C" fn Agent_OnLoad( fn onload(vm: *mut JavaVM) -> Result<(), jint> { let mut jvmti_env = JvmTiEnv::new(vm)?; jvmti_env.enable_capabilities()?; - jvmti_env.set_exception_handler()?; + + // SetEventCallbacks replaces the whole table, so both callbacks are registered + // here and delivery is toggled separately. + let callbacks = jvmtiEventCallbacks { + Exception: Some(c_on_exception), + ClassPrepare: Some(c_on_class_prepare), + ..Default::default() + }; + jvmti_env.set_event_callbacks(&callbacks)?; + + // The Exception event is deliberately NOT enabled yet. Arming it at load time + // means every exception thrown during VM and framework bootstrap enters the + // callback -- tens of thousands of them on a Spring Boot startup -- before + // ThrowableCache is even loadable. It is enabled from ClassPrepare instead. + jvmti_env.set_event_mode( + jvmtiEventMode_JVMTI_ENABLE, + jvmtiEvent_JVMTI_EVENT_CLASS_PREPARE, + )?; Ok(()) } -fn on_exception(jvmti_env: JvmTiEnv, jni_env: JniEnv, thread: jthread, exception: jobject) { - if let Err(e) = exceptions::inner_callback(jvmti_env, jni_env, thread, exception) { +fn on_exception(jvmti_env: JvmTiEnv, mut jni_env: JniEnv, thread: jthread, exception: jobject) { + let _guard = match ReentryGuard::enter() { + Some(guard) => guard, + None => return, + }; + + if jni_env.push_local_frame(LOCAL_FRAME_CAPACITY).is_err() { + return; + } + let result = exceptions::inner_callback(jvmti_env, jni_env, thread, exception); + // Never leave an exception pending across the frame pop or back into the + // throwing thread. + jni_env.clear_pending_exception(); + jni_env.pop_local_frame(); + + if let Err(e) = result { debug!("{}", e); } } @@ -74,3 +146,46 @@ unsafe extern "C" fn c_on_exception( on_exception(jvmti_env, JniEnv::new(jni_env), thread, exception); } } + +/// Fires for every class the VM prepares until the agent arms, so the miss path is +/// kept to a single `GetClassSignature` plus a byte comparison. +#[allow(unused_variables)] +unsafe extern "C" fn c_on_class_prepare( + jvmti_env: *mut jvmtiEnv, + jni_env: *mut JNIEnv, + thread: jthread, + klass: jclass, +) -> () { + if !INIT_SUCCESS.load(Ordering::Relaxed) || cache::CORE.get().is_some() { + return; + } + + let mut jvmti = JvmTiEnv::wrap(jvmti_env); + let expected = CStr::from_bytes_with_nul(b"Lcom/rollbar/jvmti/ThrowableCache;\0") + .expect("signature literal is nul terminated"); + match jvmti.class_signature_is(klass, expected) { + Ok(true) => {} + _ => return, + } + + let mut jni = JniEnv::new(jni_env); + if let Err(e) = cache::arm(&mut jvmti, &mut jni, klass) { + warn!("rollbar agent: could not initialise from ThrowableCache: {}", e); + jni.clear_pending_exception(); + // Leave the Exception event disabled: an inert agent is the safe failure. + return; + } + + if let Err(e) = jvmti.set_event_mode( + jvmtiEventMode_JVMTI_ENABLE, + jvmtiEvent_JVMTI_EVENT_EXCEPTION, + ) { + warn!("rollbar agent: could not enable exception events: {}", e); + return; + } + let _ = jvmti.set_event_mode( + jvmtiEventMode_JVMTI_DISABLE, + jvmtiEvent_JVMTI_EVENT_CLASS_PREPARE, + ); + info!("rollbar agent: armed, exception capture enabled"); +} From 76bca6ee1eb3aab976b670baad0a2eba061e0d59 Mon Sep 17 00:00:00 2001 From: buongarzoni Date: Thu, 30 Jul 2026 01:39:22 -0300 Subject: [PATCH 3/5] perf(native-agent): skip locals capture for non-app frames --- native-agent/src/filter.rs | 187 ++++++++++++++++++ .../com/rollbar/jvmti/ThrowableCache.java | 36 +++- 2 files changed, 220 insertions(+), 3 deletions(-) create mode 100644 native-agent/src/filter.rs diff --git a/native-agent/src/filter.rs b/native-agent/src/filter.rs new file mode 100644 index 00000000..2c271574 --- /dev/null +++ b/native-agent/src/filter.rs @@ -0,0 +1,187 @@ +//! Native app-package pre-filter. +//! +//! `ThrowableCache.shouldCacheThrowable` calls `throwable.getStackTrace()`, which +//! decodes the backtrace and allocates a `StackTraceElement[]` for *every* exception +//! in the JVM once app packages are configured. On a framework-heavy startup the vast +//! majority of those are rejected, so paying for the trace is wasted. +//! +//! This module answers the same question natively, from the JVMTI stack, memoizing the +//! per-method decision so repeated throws from the same code cost a hash lookup. +//! +//! It is only ever used to make the *reject* path cheap. Anything that passes still +//! goes through `shouldCacheThrowable`, which remains the authority and also performs +//! the per-throwable dedup. + +use std::collections::HashMap; +use std::sync::atomic::{AtomicI32, Ordering}; +use std::sync::{OnceLock, RwLock}; + +use cache::Core; +use env::JvmTiEnv; +use errors::*; +use jni::JniEnv; +use jvmti::{jint, jmethodID, jthread, jvmtiFrameInfo}; + +/// Frames examined when deciding whether a throw is app-related. The Java filter +/// scans the whole trace; this bound only applies to the fast path, and anything not +/// rejected here still gets the full Java check. +const MAX_SCAN_FRAMES: jint = 128; + +/// Cap on memoized method decisions. `jmethodID`s are invalidated by class unloading +/// and can be recycled, so the map is cleared wholesale rather than grown forever. +/// A stale entry can only mis-classify one frame; the id is compared as an integer +/// and never dereferenced. +const MAX_DECISIONS: usize = 100_000; + +static KNOWN_VERSION: AtomicI32 = AtomicI32::new(-1); + +/// Package prefixes in internal form (`com/foo`), matching JVMTI class signatures. +static PREFIXES: RwLock>> = RwLock::new(Vec::new()); + +static DECISIONS: OnceLock>> = OnceLock::new(); + +fn decisions() -> &'static RwLock> { + DECISIONS.get_or_init(|| RwLock::new(HashMap::new())) +} + +/// Outcome of the cheap pre-check. +pub enum Decision { + /// No app packages configured: `shouldCacheThrowable` could only return false. + Skip, + /// No app frame on the stack; reject without materializing a Java stack trace. + NoAppFrame, + /// Either an app frame was found, or the filter is unavailable. Ask Java. + AskJava, +} + +/// Reads the version counter and, when it has changed, refreshes the native prefix +/// list. `GetStaticIntField` is a plain memory read through the JNI table: no +/// allocation, no Java execution. +pub fn decide( + jvmti_env: &mut JvmTiEnv, + jni_env: &mut JniEnv, + core: &Core, + thread: jthread, +) -> Decision { + let (version_field, snapshot_method) = match (core.app_packages_version, core.app_packages_snapshot) + { + (Some(field), Some(method)) => (field, method), + // Older rollbar-java: no cheap gate available, defer to Java entirely. + _ => return Decision::AskJava, + }; + + let version = jni_env.get_static_int_field(core.throwable_cache, version_field); + if version == 0 { + return Decision::Skip; + } + + if version != KNOWN_VERSION.load(Ordering::Relaxed) { + if refresh(jni_env, core, snapshot_method).is_err() { + jni_env.clear_pending_exception(); + return Decision::AskJava; + } + KNOWN_VERSION.store(version, Ordering::Relaxed); + } + + match stack_has_app_frame(jvmti_env, thread) { + Ok(true) => Decision::AskJava, + Ok(false) => Decision::NoAppFrame, + // Never fail closed: if the native scan breaks, let Java decide. + Err(_) => Decision::AskJava, + } +} + +fn refresh(jni_env: &mut JniEnv, core: &Core, snapshot_method: jmethodID) -> Result<()> { + let array = jni_env.call_static_object_method0(core.throwable_cache, snapshot_method)?; + let length = jni_env.get_array_length(array); + + let mut prefixes = Vec::with_capacity(length as usize); + for i in 0..length { + let element = jni_env.get_object_array_element(array, i); + if let Some(bytes) = jni_env.string_to_bytes(element) { + // Java compares against dotted class names; JVMTI signatures use slashes. + prefixes.push( + bytes + .into_iter() + .map(|b| if b == b'.' { b'/' } else { b }) + .collect(), + ); + } + } + + if let Ok(mut guard) = PREFIXES.write() { + *guard = prefixes; + } + // Prefixes changed, so cached per-method verdicts are no longer valid. + if let Ok(mut guard) = decisions().write() { + guard.clear(); + } + Ok(()) +} + +fn stack_has_app_frame(jvmti_env: &mut JvmTiEnv, thread: jthread) -> Result { + let mut frames = [jvmtiFrameInfo::default(); MAX_SCAN_FRAMES as usize]; + let mut returned: jint = 0; + jvmti_env.get_stack_trace( + thread, + 0, + MAX_SCAN_FRAMES, + frames.as_mut_ptr(), + &mut returned, + )?; + + let returned = returned.max(0).min(MAX_SCAN_FRAMES) as usize; + for frame in frames.iter().take(returned) { + if is_app_method(jvmti_env, frame.method)? { + return Ok(true); + } + } + Ok(false) +} + +/// Whether a frame belongs to app code, for deciding if its locals are worth +/// collecting. When no prefixes are known -- an older rollbar-java, or packages not +/// yet configured -- every frame counts as app code so behaviour matches the +/// pre-filter agent rather than silently capturing nothing. +pub fn is_app_frame(jvmti_env: &mut JvmTiEnv, method: jmethodID) -> bool { + match PREFIXES.read() { + Ok(prefixes) if prefixes.is_empty() => return true, + Err(_) => return true, + Ok(_) => {} + } + is_app_method(jvmti_env, method).unwrap_or(true) +} + +fn is_app_method(jvmti_env: &mut JvmTiEnv, method: jmethodID) -> Result { + let key = method as usize; + if let Ok(guard) = decisions().read() { + if let Some(&known) = guard.get(&key) { + return Ok(known); + } + } + + let mut declaring = ::std::ptr::null_mut(); + jvmti_env.get_method_declaring_class(method, &mut declaring)?; + let signature = jvmti_env.get_class_signature(declaring)?; + + // "Lcom/foo/Bar;" -> "com/foo/Bar". Anything else (arrays, primitives) is not app code. + let internal_name = match signature.split_first() { + Some((&b'L', rest)) => rest, + _ => &[][..], + }; + + let matched = match PREFIXES.read() { + Ok(prefixes) => prefixes + .iter() + .any(|prefix| internal_name.starts_with(prefix.as_slice())), + Err(_) => false, + }; + + if let Ok(mut guard) = decisions().write() { + if guard.len() >= MAX_DECISIONS { + guard.clear(); + } + guard.insert(key, matched); + } + Ok(matched) +} diff --git a/rollbar-java/src/main/java/com/rollbar/jvmti/ThrowableCache.java b/rollbar-java/src/main/java/com/rollbar/jvmti/ThrowableCache.java index fe9b80f0..bcb3d3fa 100644 --- a/rollbar-java/src/main/java/com/rollbar/jvmti/ThrowableCache.java +++ b/rollbar-java/src/main/java/com/rollbar/jvmti/ThrowableCache.java @@ -1,5 +1,6 @@ package com.rollbar.jvmti; +import java.util.Collections; import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -10,7 +11,20 @@ * of an exception which can be queried later by the notifier for enhancing payloads. */ public final class ThrowableCache { - private static Set appPackages = new HashSet<>(); + /** + * Copy-on-write so the native agent, which reads this from arbitrary throwing threads, never + * observes a partially updated set. + */ + private static volatile Set appPackages = Collections.emptySet(); + + /** + * Incremented whenever {@link #appPackages} changes. The native agent reads this field + * directly via JNI {@code GetStaticIntField} on every exception so it can skip all further + * work while it is zero, and refresh its own copy of the prefixes when it changes. Zero means + * "no app packages configured", in which case {@link #shouldCacheThrowable} can only return + * false. Do not rename or change the type without updating native-agent/src/filter.rs. + */ + static volatile int appPackagesVersion = 0; private static ThreadLocal> cache = new ThreadLocal>() { @@ -84,7 +98,23 @@ public static boolean shouldCacheThrowable(Throwable throwable, int numFrames) { * * @param newAppPackage a string to add to the set of packages in your app. */ - public static void addAppPackage(String newAppPackage) { - appPackages.add(newAppPackage); + public static synchronized void addAppPackage(String newAppPackage) { + Set updated = new HashSet<>(appPackages); + updated.add(newAppPackage); + // Publish the set before bumping the version: a reader that sees the new version is then + // guaranteed to see the new set. + appPackages = Collections.unmodifiableSet(updated); + appPackagesVersion++; + } + + /** + * Snapshot of the configured app packages, read by the native agent so it can apply the same + * prefix filter without calling back into Java for every exception. + * + * @return the currently configured package prefixes. + */ + static String[] appPackagesSnapshot() { + Set current = appPackages; + return current.toArray(new String[0]); } } From b1fdf7883e4a6a3c6410a1a8e66cf9fbaf0de298 Mon Sep 17 00:00:00 2001 From: buongarzoni Date: Thu, 30 Jul 2026 01:39:41 -0300 Subject: [PATCH 4/5] fix(rollbar-java): handle null and throwing toString in JSON serializer --- .../sender/json/JsonSerializerImpl.java | 41 +++++++-- .../sender/json/JsonSerializerImplTest.java | 84 +++++++++++++++++++ .../notifier/sender/json/JsonTestHelper.java | 61 ++++++++++++++ 3 files changed, 180 insertions(+), 6 deletions(-) diff --git a/rollbar-java/src/main/java/com/rollbar/notifier/sender/json/JsonSerializerImpl.java b/rollbar-java/src/main/java/com/rollbar/notifier/sender/json/JsonSerializerImpl.java index 9a599f98..36d969b1 100755 --- a/rollbar-java/src/main/java/com/rollbar/notifier/sender/json/JsonSerializerImpl.java +++ b/rollbar-java/src/main/java/com/rollbar/notifier/sender/json/JsonSerializerImpl.java @@ -5,8 +5,6 @@ import com.rollbar.api.json.JsonSerializable; import com.rollbar.api.payload.Payload; import com.rollbar.notifier.sender.result.Result; -import java.io.PrintWriter; -import java.io.StringWriter; import java.util.Collection; import java.util.LinkedHashMap; import java.util.Map; @@ -165,13 +163,36 @@ private void serializeValue(StringBuilder builder, Object value, int level) { } private static void serializeThrowable(StringBuilder builder, Throwable value) { - final StringWriter writer = new StringWriter(); - value.printStackTrace(new PrintWriter(writer)); - serializeString(builder, value.toString()); + serializeToString(builder, value); } private static void serializeDefault(StringBuilder builder, Object value) { - serializeString(builder, value == null ? "" : value.toString()); + serializeToString(builder, value); + } + + /** + * Serializes an arbitrary object via toString(). The value is never null here - + * serializeValue handles that case - but toString() itself is application code and + * may return null or throw, and neither may be allowed to break the whole payload. + * This matters because the JVMTI agent puts captured local variables, i.e. arbitrary + * application objects, into the payload. + */ + private static void serializeToString(StringBuilder builder, Object value) { + String str; + try { + str = value.toString(); + } catch (Exception e) { + // Deliberately not catching Error: a StackOverflowError from a recursive + // toString() should propagate rather than be silently swallowed here. + serializeString(builder, ""); + return; + } + + if (str == null) { + serializeNull(builder); + return; + } + serializeString(builder, str); } private static void serializeNumber(StringBuilder builder, Number value) { @@ -222,6 +243,14 @@ private static Map asMap(Map value) { // Borrowed from // https://github.com/google/gson/blob/59edfc1caf2bb30e30f523f8502f23e8f8edc38e/gson/src/main/java/com/google/gson/stream/JsonWriter.java private static void serializeString(StringBuilder builder, String str) { + // Emit an empty string rather than the bare null literal: this is also used for + // object keys, where an unquoted null would be invalid JSON. Callers that want a + // real JSON null for a null value route to serializeNull themselves. + if (str == null) { + builder.append("\"\""); + return; + } + builder.append('"'); int last = 0; int length = str.length(); diff --git a/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonSerializerImplTest.java b/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonSerializerImplTest.java index 8d30d96e..7a139001 100644 --- a/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonSerializerImplTest.java +++ b/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonSerializerImplTest.java @@ -1,10 +1,12 @@ package com.rollbar.notifier.sender.json; +import static com.rollbar.notifier.sender.json.JsonTestHelper.assertValidJson; import static com.rollbar.notifier.sender.json.JsonTestHelper.fromString; import static com.rollbar.notifier.sender.json.JsonTestHelper.getValue; import static java.lang.String.format; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.core.Is.is; import com.rollbar.api.payload.Payload; @@ -137,6 +139,88 @@ public String toString() { assertThat(result, equalTo("Object(\"quoted\")")); } + @Test + public void shouldSerializeObjectWhoseToStringReturnsNullAsJsonNull() { + // The JVMTI agent puts captured local variables into the payload, so toString() + // here is arbitrary application code. Returning null is a common accident. + Object obj = new Object() { + @Override + public String toString() { + return null; + } + }; + + Payload payload = payloadWithCustom("object", obj); + + String serialized = new JsonSerializerImpl().toJson(payload); + + assertValidJson(serialized); + Map custom = getValue(fromString(serialized), "data", "custom"); + assertThat(custom.containsKey("object"), is(true)); + assertThat(custom.get("object"), is(nullValue())); + } + + @Test + public void shouldSerializeObjectWhoseToStringThrows() { + Object obj = new Object() { + @Override + public String toString() { + throw new IllegalStateException("boom"); + } + }; + + Payload payload = payloadWithCustom("object", obj); + + String serialized = new JsonSerializerImpl().toJson(payload); + + assertValidJson(serialized); + String result = getValue(fromString(serialized), "data", "custom", "object"); + assertThat(result, equalTo("")); + } + + @Test + public void shouldSerializeThrowableWhoseToStringReturnsNullAsJsonNull() { + Throwable t = new Throwable() { + @Override + public String toString() { + return null; + } + }; + + Payload payload = payloadWithCustom("throwable", t); + + String serialized = new JsonSerializerImpl().toJson(payload); + + assertValidJson(serialized); + Map custom = getValue(fromString(serialized), "data", "custom"); + assertThat(custom.containsKey("throwable"), is(true)); + assertThat(custom.get("throwable"), is(nullValue())); + } + + @Test + public void shouldSerializeNullMapValueAsJsonNull() { + Payload payload = payloadWithCustom("nothing", null); + + String serialized = new JsonSerializerImpl().toJson(payload); + + assertValidJson(serialized); + Map custom = getValue(fromString(serialized), "data", "custom"); + assertThat(custom.containsKey("nothing"), is(true)); + assertThat(custom.get("nothing"), is(nullValue())); + } + + @Test + public void shouldSerializeNullMapKeyWithoutProducingInvalidJson() { + // HashMap permits a null key, and nothing upstream filters it out. + Payload payload = payloadWithCustom(null, "a value"); + + String serialized = new JsonSerializerImpl().toJson(payload); + + assertValidJson(serialized); + Map custom = getValue(fromString(serialized), "data", "custom"); + assertThat(custom.get(""), equalTo((Object) "a value")); + } + private Payload payloadWithCustom(String key, Object value) { Map custom = new HashMap<>(); custom.put(key, value); diff --git a/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonTestHelper.java b/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonTestHelper.java index 361f2021..675a73f3 100644 --- a/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonTestHelper.java +++ b/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonTestHelper.java @@ -1,10 +1,71 @@ package com.rollbar.notifier.sender.json; import com.google.gson.Gson; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonToken; +import java.io.IOException; +import java.io.StringReader; import java.util.Map; public class JsonTestHelper { + + /** + * Strictly parses the whole document, failing on anything Gson's lenient default + * would wave through. {@link #fromString} is lenient, so it cannot by itself prove + * the serializer emitted well formed JSON - an unterminated string, for instance, + * can survive a lenient parse. + * + * @param serializedData the JSON to validate. + */ + public static void assertValidJson(String serializedData) { + JsonReader reader = new JsonReader(new StringReader(serializedData)); + reader.setLenient(false); + try { + skipValue(reader); + if (reader.peek() != JsonToken.END_DOCUMENT) { + throw new AssertionError("Trailing content after JSON document: " + serializedData); + } + } catch (IOException | RuntimeException e) { + throw new AssertionError("Not valid JSON: " + serializedData, e); + } + } + + private static void skipValue(JsonReader reader) throws IOException { + // skipValue() itself is lenient about structure, so walk the document instead. + switch (reader.peek()) { + case BEGIN_OBJECT: + reader.beginObject(); + while (reader.hasNext()) { + reader.nextName(); + skipValue(reader); + } + reader.endObject(); + break; + case BEGIN_ARRAY: + reader.beginArray(); + while (reader.hasNext()) { + skipValue(reader); + } + reader.endArray(); + break; + case STRING: + reader.nextString(); + break; + case NUMBER: + reader.nextDouble(); + break; + case BOOLEAN: + reader.nextBoolean(); + break; + case NULL: + reader.nextNull(); + break; + default: + throw new AssertionError("Unexpected token: " + reader.peek()); + } + } + @SuppressWarnings("unchecked") public static Map fromString(String serializedData) { // Gson's Json compliance seems to be pretty good, let's see if it can deserialize our From 6b9a3a65afc63b018b3f98da51e3fd6339e54108 Mon Sep 17 00:00:00 2001 From: buongarzoni Date: Thu, 30 Jul 2026 01:42:56 -0300 Subject: [PATCH 5/5] docs(native-agent): document activation model and build requirements --- native-agent/Cargo.toml | 2 +- native-agent/README.md | 46 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/native-agent/Cargo.toml b/native-agent/Cargo.toml index d905fcdc..ac7cbbd2 100644 --- a/native-agent/Cargo.toml +++ b/native-agent/Cargo.toml @@ -5,7 +5,7 @@ authors = ["Andrew Weiss "] build = "build.rs" [lib] -crate_type = [ "cdylib" ] +crate-type = [ "cdylib" ] [dependencies] jni = "0.21.1" diff --git a/native-agent/README.md b/native-agent/README.md index 2f8aeda6..69fbc9d8 100644 --- a/native-agent/README.md +++ b/native-agent/README.md @@ -42,14 +42,40 @@ new Rollbar(withAccessToken("ACCESS-TOKEN") Once you have the agent setup and `rollbar-java` configured, `rollbar-java` will attribute the exceptions using the agent as well as send back unhandled exceptions if configured. +Two things are required, and the agent captures nothing if either is missing: + +* **`appPackages` must be configured.** It is not an optional filter — it is the switch that + turns capture on. With no app packages the agent stays dormant. +* **Your classes must be compiled with debug information** (`javac -g`, which Gradle and Maven + enable by default for `debug = true`). Local variable names live in the `LocalVariableTable` + attribute; without `-g` the agent has nothing to read and frames come back with no locals. + +Note also that the cache is keyed per thread: locals are only available if the throwable is +reported on the same thread that threw it. Handing an exception to another thread (an executor, +an async error handler) before reporting it loses the locals. + +## How the agent activates + +The agent does nothing until `com.rollbar.jvmti.ThrowableCache` is loaded. It watches for that +class via a JVMTI `ClassPrepare` callback and only then enables exception capture, so an +application that never initialises Rollbar — and the whole of JVM and framework startup before +Rollbar *is* initialised — pays essentially nothing. + +This also means the agent resolves `ThrowableCache` through whichever classloader actually +loaded it. + Regardless of your JVM language of choice, at some level there will be an invocation of the JVM and therefore there is a configuration option to pass arguments directly to the JVM. ## Getting the agent library -We will attempt to distribute via the releases page pre-built versions of the agent library for -various architectures. However, if you are running in an environment where one of these libraries -does not work, then you can build your own as long as you can install the Rust toolchain. +No pre-built binaries have been published since 1.4.1, and nothing in CI builds this library, so +in practice you should expect to build it yourself. It is a plain Cargo project and is not part +of the Gradle build — `./gradlew build` does not produce it. + +Build for the architecture you will run on: an `x86_64` library will not load on an Apple Silicon +JVM (`mach-o file, but is an incompatible architecture`). `build.rs` reads `JAVA_HOME` to locate +the JNI headers, so make sure it points at a real JDK. ### Building Generically @@ -87,3 +113,17 @@ target/x86_64-unknown-linux-gnu/release/librollbar_java_agent.so If you want to see additional output from our agent, you can set the environment variable `ROLLBAR_LOG` to one of `trace`, `debug`, `info`, or `warn`. These will output different levels of information to standard out where your JVM process is running. + +At `info` you should see the agent arm itself exactly once, shortly after Rollbar is constructed: + +``` +INFO rollbar_java_agent > rollbar agent: armed, exception capture enabled +``` + +If that line never appears, `ThrowableCache` was never loaded — check that `rollbar-java` is on +the classpath and that a `Rollbar` instance is actually being created. + +If the JVM refuses to start with `agent library failed to init` and `ROLLBAR_LOG=warn` shows +`GetEnv failed: -3`, the JVM is older than the agent expects. The agent asks for JVMTI 1.2, which +every JDK 8 or newer provides; earlier builds asked for the version of the JDK they were compiled +against, so a library built with JDK 21 would refuse to load on JDK 17.