rustvulncheck/lessons.md

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<T> From<Py<T>> for PyObject, the <.*> consumed <T> From<Py<T>> (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::<method> 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.

Bug 4: Duplicate symbols within entries (~43 entries, 195 extra symbols)

Symptom: The same function appearing 2+ times in an entry, e.g. open_mmap x2.

Root cause: When a function signature is both removed (-fn foo(old: X)) and added (+fn foo(new: Y)), both lines match fn_decl_re independently, each producing a symbol. The existing dedup check (symbols.iter().any(...)) only applied to the body-change code path (lines 131-154), not the fn-declaration code path (lines 106-124).

Fix: Added dedup to the fn-declaration path: when a symbol with the same qualified name already exists, upgrade its change_type to Modified instead of adding a duplicate. This is semantically correct — if a fn is both deleted and added, it was modified.

Bug 5: Trait name captured as implementing type (~146 symbols)

Symptom: Symbols like From::from, Into::new, TryFrom::try_from where a standard trait name appears where the implementing type should be.

Root cause: Hunk headers often show truncated impl lines like impl From<Error> without the for ActualType part (which is on the next line or beyond the context window). parse_impl_type correctly extracted the text after impl<generics>, but without a for, it captured the trait name as the type.

Fix: Added is_std_trait() blocklist — when parse_impl_type finds no for keyword and the captured type name is a well-known trait (From, Into, TryFrom, Clone, Default, Iterator, operator traits, serde traits, etc.), it returns None instead of the trait name. This causes the symbol to fall through to the <method> placeholder, which is less precise but at least not wrong.

Lesson: When parsing partial/truncated context (like git hunk headers), a wrong answer is worse than no answer. The <method> placeholder is imprecise but correct; From::from is precise but wrong and will never match real call sites.

Remaining Known Limitations

These are documented in CLAUDE.md but not yet fixed:

• <method> placeholders (~90+ entries): When git hunk headers don't include the fn declaration, the tool records Type::<method>. This is a fundamental limitation of diff-based analysis — the fn context depends on git's hunk header generation, which uses a limited window. The trait-name fix increases this count slightly (trait impls now fall through to <method> instead of Trait::method).
• 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<T> From<Py<T>> 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 bracket-counting parser
• test_duplicate_fn_deduped_as_modified — dedup when same fn is both + and -
• test_trait_name_not_captured_as_type — blocklist prevents From/Into as type
• test_trait_as_type_not_in_symbols — end-to-end with truncated hunk header
• test_is_std_trait — unit test for the trait blocklist