Skip to content

Commit 340a8ae

Browse files
committed
Address reviewer's comment:
P1 Badge Avoid tracing when filter resolution errors occur If resolving a scope’s filter fails, scope_resolution logs the error and returns None without marking the code object as skipped. should_trace_code treats a None resolution as “trace”, so subsequent argument/local capture runs with value_policy=None, meaning values are recorded unfiltered even though the user requested filtering. A failure to read co_filename/co_qualname or any internal engine error therefore silently disables all filtering for that frame and can leak sensitive data. The safer behaviour would be to skip the scope or disable tracing on resolution errors.
1 parent 6193762 commit 340a8ae

File tree

1 file changed

+109
-8
lines changed
  • codetracer-python-recorder/src/runtime

1 file changed

+109
-8
lines changed

codetracer-python-recorder/src/runtime/mod.rs

Lines changed: 109 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ use value_capture::{
2222
use std::collections::{hash_map::Entry, HashMap, HashSet};
2323
use std::fs;
2424
use std::path::{Path, PathBuf};
25-
#[cfg(feature = "integration-test")]
25+
#[cfg(any(feature = "integration-test", test))]
2626
use std::sync::atomic::{AtomicBool, Ordering};
2727
use std::sync::Arc;
2828
#[cfg(feature = "integration-test")]
@@ -117,6 +117,13 @@ enum ShouldTrace {
117117
SkipAndDisable,
118118
}
119119

120+
#[derive(Debug)]
121+
enum ScopeResolutionState {
122+
NotConfigured,
123+
Resolved(Arc<ScopeResolution>),
124+
Error,
125+
}
126+
120127
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
121128
enum FailureStage {
122129
PyStart,
@@ -190,6 +197,14 @@ static FAILURE_MODE: OnceLock<Option<FailureMode>> = OnceLock::new();
190197
#[cfg(feature = "integration-test")]
191198
static FAILURE_TRIGGERED: AtomicBool = AtomicBool::new(false);
192199

200+
#[cfg(test)]
201+
static FORCE_SCOPE_RESOLUTION_ERROR: AtomicBool = AtomicBool::new(false);
202+
203+
#[cfg(test)]
204+
fn take_forced_scope_resolution_error() -> bool {
205+
FORCE_SCOPE_RESOLUTION_ERROR.swap(false, Ordering::SeqCst)
206+
}
207+
193208
#[cfg(feature = "integration-test")]
194209
fn configured_failure_mode() -> Option<FailureMode> {
195210
*FAILURE_MODE.get_or_init(|| {
@@ -392,12 +407,28 @@ impl RuntimeTracer {
392407
&mut self,
393408
py: Python<'_>,
394409
code: &CodeObjectWrapper,
395-
) -> Option<Arc<ScopeResolution>> {
396-
let engine = self.trace_filter.as_ref()?;
410+
) -> ScopeResolutionState {
411+
let engine = match self.trace_filter.as_ref() {
412+
Some(engine) => engine,
413+
None => return ScopeResolutionState::NotConfigured,
414+
};
397415
let code_id = code.id();
398416

399417
if let Some(existing) = self.scope_cache.get(&code_id) {
400-
return Some(existing.clone());
418+
return ScopeResolutionState::Resolved(existing.clone());
419+
}
420+
421+
#[cfg(test)]
422+
if take_forced_scope_resolution_error() {
423+
with_error_code(ErrorCode::Unknown, || {
424+
let _mute = ScopedMuteIoCapture::new();
425+
log::error!(
426+
"[RuntimeTracer] trace filter resolution failed for code id {}: forced test error",
427+
code_id
428+
);
429+
});
430+
record_dropped_event("filter_resolution_error");
431+
return ScopeResolutionState::Error;
401432
}
402433

403434
match engine.resolve(py, code) {
@@ -407,7 +438,7 @@ impl RuntimeTracer {
407438
} else {
408439
self.scope_cache.remove(&code_id);
409440
}
410-
Some(resolution)
441+
ScopeResolutionState::Resolved(resolution)
411442
}
412443
Err(err) => {
413444
let message = err.to_string();
@@ -421,7 +452,7 @@ impl RuntimeTracer {
421452
);
422453
});
423454
record_dropped_event("filter_resolution_error");
424-
None
455+
ScopeResolutionState::Error
425456
}
426457
}
427458
}
@@ -650,8 +681,8 @@ impl RuntimeTracer {
650681
return ShouldTrace::SkipAndDisable;
651682
}
652683

653-
if let Some(resolution) = self.scope_resolution(py, code) {
654-
match resolution.exec() {
684+
match self.scope_resolution(py, code) {
685+
ScopeResolutionState::Resolved(resolution) => match resolution.exec() {
655686
ExecDecision::Skip => {
656687
self.scope_cache.remove(&code_id);
657688
self.filter_stats.record_skip();
@@ -662,6 +693,15 @@ impl RuntimeTracer {
662693
ExecDecision::Trace => {
663694
// already cached for future use
664695
}
696+
},
697+
ScopeResolutionState::Error => {
698+
self.scope_cache.remove(&code_id);
699+
self.filter_stats.record_skip();
700+
self.ignored_code_ids.insert(code_id);
701+
return ShouldTrace::SkipAndDisable;
702+
}
703+
ScopeResolutionState::NotConfigured => {
704+
// Continue without filtering when no engine is configured.
665705
}
666706
}
667707

@@ -1699,6 +1739,67 @@ def emit_return(value):
16991739
fs::write(path, contents.trim_start()).expect("write filter");
17001740
}
17011741

1742+
#[test]
1743+
fn scope_resolution_error_disables_tracing() {
1744+
Python::with_gil(|py| {
1745+
ensure_test_module(py);
1746+
1747+
let project = tempfile::tempdir().expect("project dir");
1748+
let project_root = project.path();
1749+
let filters_dir = project_root.join(".codetracer");
1750+
fs::create_dir(&filters_dir).expect("create .codetracer");
1751+
let filter_path = filters_dir.join("filters.toml");
1752+
write_filter(
1753+
&filter_path,
1754+
r#"
1755+
[meta]
1756+
name = "test"
1757+
version = 1
1758+
1759+
[scope]
1760+
default_exec = "trace"
1761+
default_value_action = "allow"
1762+
"#,
1763+
);
1764+
let config = TraceFilterConfig::from_paths(&[filter_path]).expect("load filter");
1765+
let engine = Arc::new(TraceFilterEngine::new(config));
1766+
1767+
let mut tracer = RuntimeTracer::new(
1768+
"app/module.py",
1769+
&[],
1770+
TraceEventsFileFormat::Json,
1771+
None,
1772+
Some(engine),
1773+
);
1774+
1775+
let compile_fn = py
1776+
.import("builtins")
1777+
.expect("import builtins")
1778+
.getattr("compile")
1779+
.expect("fetch compile");
1780+
let compiled = compile_fn
1781+
.call1(("snapshot()", "app/module.py", "exec"))
1782+
.expect("compile code object");
1783+
let code_obj = compiled.downcast::<PyCode>().expect("downcast code object");
1784+
let wrapper = CodeObjectWrapper::new(py, &code_obj);
1785+
1786+
FORCE_SCOPE_RESOLUTION_ERROR.store(true, Ordering::SeqCst);
1787+
assert_eq!(
1788+
tracer.should_trace_code(py, &wrapper),
1789+
ShouldTrace::SkipAndDisable
1790+
);
1791+
assert!(tracer.ignored_code_ids.contains(&wrapper.id()));
1792+
assert!(tracer.scope_cache.get(&wrapper.id()).is_none());
1793+
assert_eq!(tracer.filter_stats.skipped_scopes, 1);
1794+
1795+
// Subsequent checks should keep the scope skipped without re-triggering the error.
1796+
assert_eq!(
1797+
tracer.should_trace_code(py, &wrapper),
1798+
ShouldTrace::SkipAndDisable
1799+
);
1800+
});
1801+
}
1802+
17021803
#[test]
17031804
fn trace_filter_redacts_values() {
17041805
Python::with_gil(|py| {

0 commit comments

Comments
 (0)