diff --git a/lessons.md b/lessons.md new file mode 100644 index 0000000..ae31072 --- /dev/null +++ b/lessons.md @@ -0,0 +1,63 @@ +# Lessons Learned from Spot-Checking vuln_db.json + +## Summary + +A spot-check of the enriched vulnerability database (642 entries, 147 with symbols) uncovered three classes of bugs in `diff_analyzer.rs`, all now fixed with regression tests. + +## Bug 1: `for` keyword leaking into symbol names (~14 entries) + +**Symptom:** Symbols like `instance::for PyObject::from` instead of `instance::PyObject::from`. + +**Root cause:** The `impl_re` regex used `(?:<.*>)?` to skip generic params after `impl`. The greedy `.*` matched across nested `<>` brackets. For `impl From> for PyObject`, the `<.*>` consumed ` From>` (everything up to the *last* `>`), leaving ` for PyObject` as the "type" capture. + +**Fix:** Replaced the regex-based impl parser with `parse_impl_type()`, which uses bracket-counting (`skip_balanced_angles`) to correctly handle nested generics, then finds ` for ` only at the top level (not inside `<>`). + +**Lesson:** Greedy regex quantifiers inside delimiter pairs (`<.*>`) are a classic source of over-matching when the delimiters can nest. Use iterative bracket-counting instead. + +## Bug 2: `where` clause leaking into symbol names (~5 entries) + +**Symptom:** Symbols like `lock_api::mutex::where::` or `array::where T: HasAfEnum::get_backend`. + +**Root cause:** Same as Bug 1 — the regex type capture `([a-zA-Z_][a-zA-Z0-9_:<>, ]*)` included spaces and continued matching through `where` clauses. Even after generics were (incorrectly) consumed, the remaining text was slurped into the type name. + +**Fix:** The new `parse_impl_type()` uses `find_top_level_keyword(type_str, " where ")` to stop the type capture at `where` clauses, again only at the top bracket-nesting level. + +**Lesson:** When parsing structured syntax, stop-conditions matter as much as start-conditions. The type name capture needed explicit terminators (` where `, `{`, newline). + +## Bug 3: Test functions included in symbols (~39 entries) + +**Symptom:** Symbols like `pycell::impl_::test_inherited_size` or `fyrox_core::test_combine_uuids` appearing as "vulnerable symbols" even though they're test functions. + +**Root cause:** The `is_test_file()` filter only checked file paths (`tests/`, `*_test.rs`, etc.). Functions marked with `#[test]` or `#[cfg(test)]` inside library source files (`src/lib.rs`, `src/pycell/impl_.rs`, etc.) were not caught. + +**Fix:** Added two filters in `extract_symbols()`: +1. Track `#[test]` and `#[cfg(test)]` attributes in diff lines; skip the next fn declaration when seen. +2. Skip any function whose name starts with `test_` (standard Rust convention). + +**Lesson:** Path-based test detection is necessary but not sufficient. Rust commonly has `#[cfg(test)] mod tests { ... }` inside library files. Attribute-level and naming-convention filters are needed too. + +## Verified Good Entries + +Not everything was broken. Several entries verified correctly against their actual GitHub commits: + +- **RUSTSEC-2026-0076 (libcrux-ml-dsa):** `libcrux_ml_dsa::encoding::signature::deserialize` — correct crate name conversion from hyphenated path, correct function extraction, test files properly filtered. +- **RUSTSEC-2022-0022 (hyper):** `Client::record_header_indices` — correctly identified from `MaybeUninit` safety fix. Impl types `Server`/`Client` properly tracked. +- **RUSTSEC-2023-0001 (tokio):** Named pipe functions (`pipe_mode`, `opts_default_pipe_mode`) plausible for `reject_remote_clients` fix. + +## Remaining Known Limitations + +These are documented in CLAUDE.md but not yet fixed: + +- **`` placeholders (~90 entries):** When git hunk headers don't include the fn declaration, the tool records `Type::`. This is a fundamental limitation of diff-based analysis — the fn context depends on git's hunk header generation, which uses a limited window. +- **Missing crate name prefix:** For non-workspace repos, `src/foo/bar.rs` → `foo::bar` instead of `crate_name::foo::bar`. The crate name isn't available from the file path alone; fixing this would require reading `Cargo.toml` from the repo. +- **Empty symbol lists (495/642):** Many advisories lack GitHub commit references, or the referenced commits don't contain extractable Rust function changes (version bumps only, C code, etc.). + +## Testing Strategy + +Each fix has targeted regression tests using synthetic diffs that reproduce the exact patterns found in the real vuln_db: +- `test_for_keyword_no_longer_leaks_into_symbol` — reproduces pyo3's `impl From> for PyObject` +- `test_where_clause_no_longer_leaks_into_symbol` — reproduces lock_api's `impl ... where R: Send` +- `test_test_functions_filtered_by_attribute` — `#[test]` fn inside library source +- `test_test_functions_filtered_by_name_prefix` — `test_*` naming convention +- `test_cfg_test_functions_filtered` — `#[cfg(test)]` attribute +- `test_parse_impl_type_*` — unit tests for the new bracket-counting parser diff --git a/src/diff_analyzer.rs b/src/diff_analyzer.rs index ffed8fb..ebfbf34 100644 --- a/src/diff_analyzer.rs +++ b/src/diff_analyzer.rs @@ -36,10 +36,6 @@ pub fn extract_symbols(diff: &PatchDiff) -> Vec { ) .unwrap(); let hunk_header_re = Regex::new(r"^@@.*@@\s*(.*)$").unwrap(); - // Match impl blocks, allowing nested generics (e.g. impl> Type) - // We match `impl` then skip everything up to the last `>>` or `>` before the type name. - let impl_re = - Regex::new(r"impl\b(?:<.*>)?\s+(?:([a-zA-Z_][a-zA-Z0-9_:]*)\s+for\s+)?([a-zA-Z_][a-zA-Z0-9_:<>, ]*)").unwrap(); for file_patch in &diff.files { // Skip test files — they aren't callable library code @@ -50,16 +46,17 @@ pub fn extract_symbols(diff: &PatchDiff) -> Vec { let mut current_context_fn: Option = None; let mut current_impl_type: Option = None; + let mut seen_test_attr = false; for line in file_patch.patch.lines() { // Track hunk headers - they often show the enclosing function if let Some(caps) = hunk_header_re.captures(line) { let context = &caps[1]; let has_fn = fn_decl_re.captures(context); - let has_impl = impl_re.captures(context); + let has_impl = parse_impl_type(context); - if let Some(icaps) = has_impl { - current_impl_type = Some(icaps[2].trim().to_string()); + if let Some(impl_type) = has_impl { + current_impl_type = Some(impl_type); } if let Some(fcaps) = has_fn { @@ -79,14 +76,20 @@ pub fn extract_symbols(diff: &PatchDiff) -> Vec { // Track impl blocks in context lines if line.starts_with(' ') || line.starts_with('+') || line.starts_with('-') { let content = &line[1..]; - if let Some(icaps) = impl_re.captures(content) { + if let Some(impl_type) = parse_impl_type(content) { if !content.trim_start().starts_with("//") { - current_impl_type = Some(icaps[2].trim().to_string()); + current_impl_type = Some(impl_type); // In a context/changed line showing impl, we're entering // a new impl block — clear the fn context. current_context_fn = None; } } + + // Track #[test] and #[cfg(test)] attributes + let trimmed = content.trim(); + if trimmed == "#[test]" || trimmed.starts_with("#[cfg(test)]") { + seen_test_attr = true; + } } // Look for fn declarations in changed lines @@ -102,6 +105,11 @@ pub fn extract_symbols(diff: &PatchDiff) -> Vec { } if let Some(caps) = fn_decl_re.captures(content) { let fn_name = &caps[1]; + // Skip test functions (marked with #[test] or named test_*) + if seen_test_attr || fn_name.starts_with("test_") { + seen_test_attr = false; + continue; + } let qualified = qualify_fn_name(&module_path, ¤t_impl_type, fn_name); let change_type = if is_added { ChangeType::Added @@ -156,7 +164,14 @@ pub fn extract_symbols(diff: &PatchDiff) -> Vec { }; if content.contains("fn ") && !content.trim_start().starts_with("//") { if let Some(caps) = fn_decl_re.captures(content) { - current_context_fn = Some(caps[1].to_string()); + let fn_name = &caps[1]; + if seen_test_attr || fn_name.starts_with("test_") { + // Don't track test functions as context + seen_test_attr = false; + current_context_fn = None; + } else { + current_context_fn = Some(fn_name.to_string()); + } } } } @@ -166,6 +181,107 @@ pub fn extract_symbols(diff: &PatchDiff) -> Vec { symbols } +/// Parse an `impl` line and extract the implementing type name. +/// +/// Handles nested generics correctly by counting `<>` brackets instead of +/// using a greedy regex. This avoids the `for` and `where` keyword leaks +/// that occur when `<.*>` over-matches nested generics like +/// `impl From> for PyObject where T: AsRef`. +fn parse_impl_type(s: &str) -> Option { + // Find the `impl` keyword (must be word-bounded) + let impl_pos = s.find("impl")?; + // Make sure `impl` is at a word boundary (not part of "implement" etc.) + if impl_pos > 0 { + let prev = s.as_bytes()[impl_pos - 1]; + if prev.is_ascii_alphanumeric() || prev == b'_' { + return None; + } + } + let after_impl = &s[impl_pos + 4..]; + // Check trailing boundary: next char must be <, whitespace, or end of string + if let Some(next) = after_impl.as_bytes().first() { + if next.is_ascii_alphanumeric() || *next == b'_' { + return None; + } + } + + // Skip impl's own generic params (balanced <>) + let rest = skip_balanced_angles(after_impl); + let rest = rest.trim_start(); + + if rest.is_empty() { + return None; + } + + // Now rest is either "Trait<...> for Type<...> ..." or "Type<...> ..." + // Find " for " at the top level (not inside <>) to split trait from type + let type_str = if let Some(pos) = find_top_level_keyword(rest, " for ") { + rest[pos + 5..].trim_start() + } else { + rest + }; + + // Stop at `where` clause (top-level) or `{` + let type_str = if let Some(pos) = find_top_level_keyword(type_str, " where ") { + &type_str[..pos] + } else { + type_str + }; + let type_str = type_str.split('{').next().unwrap_or(type_str); + let type_str = type_str.trim(); + + if type_str.is_empty() { + None + } else { + Some(type_str.to_string()) + } +} + +/// Skip past balanced `<...>` generics at the start of a string. +/// Returns the remainder after the closing `>`, or the original string +/// if it doesn't start with `<`. +fn skip_balanced_angles(s: &str) -> &str { + if !s.starts_with('<') { + return s; + } + let mut depth = 0; + for (i, c) in s.char_indices() { + match c { + '<' => depth += 1, + '>' => { + depth -= 1; + if depth == 0 { + return &s[i + 1..]; + } + } + _ => {} + } + } + s // unbalanced — return as-is +} + +/// Find a keyword (like " for " or " where ") at the top level of a string, +/// i.e. not nested inside `<>` brackets. +fn find_top_level_keyword(s: &str, keyword: &str) -> Option { + let mut depth: i32 = 0; + let bytes = s.as_bytes(); + for i in 0..bytes.len() { + match bytes[i] { + b'<' => depth += 1, + b'>' => { + if depth > 0 { + depth -= 1; + } + } + _ if depth == 0 && s[i..].starts_with(keyword) => { + return Some(i); + } + _ => {} + } + } + None +} + /// Convert a file path like `src/http/request.rs` to a module path like `http::request`. /// /// Handles workspace layouts like `crates/foo/src/bar.rs` → `foo::bar` @@ -396,4 +512,211 @@ mod tests { assert!(!symbols.is_empty()); assert!(symbols.iter().any(|s| s.function.contains("parse"))); } + + #[test] + fn test_parse_impl_type_simple() { + assert_eq!(parse_impl_type("impl Request {"), Some("Request".into())); + assert_eq!( + parse_impl_type("impl Vec {"), + Some("Vec".into()) + ); + } + + #[test] + fn test_parse_impl_type_trait_for() { + // This was the core bug: greedy <.*> consumed across nested generics + assert_eq!( + parse_impl_type("impl std::convert::From> for PyObject"), + Some("PyObject".into()) + ); + assert_eq!( + parse_impl_type("impl From for NotNan"), + Some("NotNan".into()) + ); + assert_eq!( + parse_impl_type("impl AddAssign for NotNan"), + Some("NotNan".into()) + ); + } + + #[test] + fn test_parse_impl_type_where_clause() { + // where clause should be stripped + assert_eq!( + parse_impl_type("impl From> for PyObject where T: AsRef {"), + Some("PyObject".into()) + ); + assert_eq!( + parse_impl_type("impl Array where T: Clone {"), + Some("Array".into()) + ); + } + + #[test] + fn test_parse_impl_type_not_in_word() { + // "implement" shouldn't match + assert_eq!(parse_impl_type("fn implement_thing()"), None); + } + + #[test] + fn test_for_keyword_no_longer_leaks_into_symbol() { + // Reproduces the pyo3 RUSTSEC-2020-0074 bug + let diff = PatchDiff { + commit_sha: "abc123".to_string(), + files: vec![crate::github::FilePatch { + filename: "src/instance.rs".to_string(), + patch: concat!( + "@@ -495,7 +495,9 @@ impl std::convert::From> for PyObject\n", + " fn from(ob: Py) -> Self {\n", + "- let old = ob.0;\n", + "+ unsafe { ob.into_non_null() }\n", + " }\n", + ) + .to_string(), + }], + }; + + let symbols = extract_symbols(&diff); + for s in &symbols { + assert!( + !s.function.contains("for "), + "Symbol should not contain 'for ': got '{}'", + s.function + ); + } + // Should find the `from` function under `PyObject`, not `for PyObject` + assert!( + symbols.iter().any(|s| s.function.contains("PyObject::from")), + "Expected PyObject::from, got: {:?}", + symbols + ); + } + + #[test] + fn test_where_clause_no_longer_leaks_into_symbol() { + // Reproduces the lock_api RUSTSEC-2020-0070 bug + let diff = PatchDiff { + commit_sha: "abc123".to_string(), + files: vec![crate::github::FilePatch { + filename: "lock_api/src/mutex.rs".to_string(), + patch: concat!( + "@@ -100,6 +100,8 @@ impl Mutex where R: Send {\n", + "- let old = self.0;\n", + "+ let new = self.0;\n", + ) + .to_string(), + }], + }; + + let symbols = extract_symbols(&diff); + for s in &symbols { + assert!( + !s.function.contains("where"), + "Symbol should not contain 'where': got '{}'", + s.function + ); + } + assert!( + symbols + .iter() + .any(|s| s.function.contains("Mutex") && !s.function.contains("where")), + "Expected clean Mutex symbol, got: {:?}", + symbols + ); + } + + #[test] + fn test_test_functions_filtered_by_attribute() { + // #[test] functions in library source should be skipped + let diff = PatchDiff { + commit_sha: "abc123".to_string(), + files: vec![crate::github::FilePatch { + filename: "src/pycell/impl_.rs".to_string(), + patch: concat!( + "@@ -100,6 +100,12 @@ impl PyCell {\n", + " pub fn new() -> Self {\n", + "- let old = 1;\n", + "+ let new = 2;\n", + " }\n", + "+ #[test]\n", + "+ fn test_inherited_size() {\n", + "+ assert_eq!(1, 1);\n", + "+ }\n", + ) + .to_string(), + }], + }; + + let symbols = extract_symbols(&diff); + assert!( + !symbols.iter().any(|s| s.function.contains("test_inherited")), + "test_ functions should be filtered out, got: {:?}", + symbols + ); + // The real function should still be present + assert!( + symbols.iter().any(|s| s.function.contains("new")), + "Expected new() to be found, got: {:?}", + symbols + ); + } + + #[test] + fn test_test_functions_filtered_by_name_prefix() { + // test_ prefix functions should be filtered even without #[test] attribute + let diff = PatchDiff { + commit_sha: "abc123".to_string(), + files: vec![crate::github::FilePatch { + filename: "src/lib.rs".to_string(), + patch: concat!( + "@@ -10,6 +10,8 @@\n", + "+fn test_combine_uuids() {\n", + "+ assert!(true);\n", + "+}\n", + "+pub fn real_function() {\n", + "+ do_work();\n", + "+}\n", + ) + .to_string(), + }], + }; + + let symbols = extract_symbols(&diff); + assert!( + !symbols.iter().any(|s| s.function.contains("test_combine")), + "test_ prefix functions should be filtered, got: {:?}", + symbols + ); + assert!( + symbols.iter().any(|s| s.function.contains("real_function")), + "Non-test functions should be kept, got: {:?}", + symbols + ); + } + + #[test] + fn test_cfg_test_functions_filtered() { + // Functions after #[cfg(test)] should be filtered + let diff = PatchDiff { + commit_sha: "abc123".to_string(), + files: vec![crate::github::FilePatch { + filename: "src/core.rs".to_string(), + patch: concat!( + "@@ -200,6 +200,10 @@\n", + " #[cfg(test)]\n", + "+fn helper_for_tests() {\n", + "+ setup();\n", + "+}\n", + ) + .to_string(), + }], + }; + + let symbols = extract_symbols(&diff); + assert!( + !symbols.iter().any(|s| s.function.contains("helper_for_tests")), + "#[cfg(test)] functions should be filtered, got: {:?}", + symbols + ); + } }