FunctionID: Add AddSingleFunction.java Ghidra script #1
Conversation
WalkthroughA new Ghidra script, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AddSingleFunction Script
participant FidService
participant FID Database
User->>AddSingleFunction Script: Run script
AddSingleFunction Script->>AddSingleFunction Script: Validate program, address, function
AddSingleFunction Script->>FidService: Compute function hash
AddSingleFunction Script->>User: Prompt to select FID database
User->>AddSingleFunction Script: Select FID database
AddSingleFunction Script->>FID Database: Open database
AddSingleFunction Script->>FID Database: Fetch libraries
alt No libraries exist
AddSingleFunction Script->>User: Prompt for new library details
User->>AddSingleFunction Script: Provide details
AddSingleFunction Script->>FID Database: Create new library
else Libraries exist
AddSingleFunction Script->>User: Select existing library
User->>AddSingleFunction Script: Select library
end
AddSingleFunction Script->>User: Prompt for namespace stripping
User->>AddSingleFunction Script: Respond
AddSingleFunction Script->>FidServiceLibraryIngest: Check for terminator
AddSingleFunction Script->>FID Database: Add function record
AddSingleFunction Script->>FID Database: Save and close
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java (1)
34-115: Consider adding more documentation comments to improve maintainability.While the script is well-structured, adding more documentation comments explaining the purpose and logic of different sections would improve maintainability and help future developers understand the code more easily.
public class AddSingleFunction extends GhidraScript { + /** + * FID database instance that will be used to store the function + */ private FidDB fidDb = null; @Override protected void run() throws Exception { + // Validate prerequisites: program, address and function if (currentProgram == null) { printerr("No current program"); return; } // ... existing code ... + // Calculate function hash FidService service = new FidService(); FidHashQuad hashFunction = service.hashFunction(function); // ... existing code ... + // Retrieve and select FID database FidFileManager fidFileManager = FidFileManager.getInstance(); // ... existing code ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java(1 hunks)Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidServiceLibraryIngest.java(2 hunks)
🔇 Additional comments (11)
Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidServiceLibraryIngest.java (2)
40-40: LGTM: Class visibility changed to public to support external access.The change from package-private to public visibility allows the new
AddSingleFunctionscript to access this class.
526-527: LGTM: Method visibility changed to public to enable terminator detection.Making the
findTerminatormethod public and static enables the new script to check if a function contains a terminator instruction, which is necessary for the FID database entry creation process.Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java (9)
17-19: LGTM: Clear and concise script description.The script description correctly states its purpose and is properly categorized under FunctionID.
41-54: LGTM: Proper validation of prerequisites.The script correctly validates that the current program, address, and function exist before proceeding, providing appropriate error messages when conditions aren't met.
56-61: LGTM: Function hash verification.The script properly verifies that the function can be hashed and terminates with an appropriate message if the function is too small.
63-70: LGTM: FID database availability check and selection.The script validates that FID databases are available and provides a user selection prompt.
71-94: LGTM: Library handling logic.The script correctly handles both cases: creating a new library when none exist or selecting an existing one from the FID database.
95-98: LGTM: Optional namespace stripping configuration.Providing the option to disable namespace stripping gives users flexibility in how function names are stored in the FID database.
101-101: LGTM: Usage of the now-public findTerminator method.The script correctly uses the newly exposed
FidServiceLibraryIngest.findTerminatormethod to determine if the function has a terminator.
105-107: LGTM: Function registration with appropriate attributes.The script creates a new function entry in the FID database with all required attributes: library, hash, name (with namespace handling option), offset, source file name, and terminator status.
109-113: LGTM: Proper resource management.The script properly saves changes to the database and ensures it's closed in a finally block, following good resource management practices.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java (2)
63-70: Consider improving error message for no available FID databases.The error message "No available FID DB" is clear but doesn't provide guidance on how to resolve the issue.
Consider enhancing the error message to guide users on how to add a FID database:
- printerr("No available FID DB"); + printerr("No available FID databases. Please add a FID database to your project first.");
34-38: Add success message.The script currently doesn't inform the user when a function is successfully added to the database.
Consider adding a success message after the function is added:
fidDb.createNewFunction(library, hashFunction, function.getName(disableNamespaceStripping), offset, domainFile.getName(), hasTerminator); + + println("Successfully added function " + function.getName(disableNamespaceStripping) + + " to library " + library.getName()); fidDb.saveDatabase("Saving", monitor);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java(1 hunks)Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidServiceLibraryIngest.java(2 hunks)
🔇 Additional comments (9)
Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidServiceLibraryIngest.java (2)
40-40: Changed class visibility to support new script.The
FidServiceLibraryIngestclass visibility has been changed from package-private to public, making it accessible for use in the newAddSingleFunction.javascript. This approach allows the script to leverage existing functionality while maintaining separation of concerns.
526-526: Changed method visibility to support functionality in new script.The
findTerminatormethod visibility has been changed from private to public static, allowing it to be used by the newAddSingleFunction.javascript to determine if a function contains a terminator instruction. This is necessary for the function creation process within the script.Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java (7)
17-19: Good script documentation.The script is well-documented with a concise description and appropriate category annotation, making it easy for users to understand its purpose and find it in Ghidra's script manager.
41-54: Proper prerequisite validation.The script appropriately checks for the existence of a current program, address, and function before proceeding. This prevents potential null pointer exceptions and provides clear error messages to the user.
56-61: Appropriate early termination for unsuitable functions.The script correctly checks if the function is too small to be hashed and provides a clear error message in this case. This prevents adding problematic functions to the FID database that wouldn't be useful for matching.
76-89: Good user prompting for library creation.The script effectively guides users through creating a new library when none exists, collecting all necessary metadata and using current program information where appropriate.
95-98: Clear namespace configuration.The script provides a clear prompt for configuring namespace stripping, which is an important option when adding functions to a FID database as it affects function name matching.
101-101: Effective use of public API.The script makes good use of the newly public
FidServiceLibraryIngest.findTerminatormethod to determine if the function contains a terminator instruction, which is necessary for creating accurate function records.
109-113: Proper resource management.The script correctly uses a try-finally block to ensure the database is closed even if an exception occurs during processing. This prevents resource leaks and database corruption.
| fidDb.createNewFunction(library, hashFunction, | ||
| function.getName(disableNamespaceStripping), offset, domainFile.getName(), | ||
| hasTerminator); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing duplicate check before adding function.
The script adds the function to the database without checking if it already exists, which could lead to duplicate entries.
Consider adding a check to determine if the function already exists in the database:
+ // Check if function already exists in the database
+ List<FunctionRecord> existingFunctions =
+ fidDb.findFunctionsByLibraryAndName(library, function.getName(disableNamespaceStripping));
+ boolean functionExists = false;
+ for (FunctionRecord existingFunction : existingFunctions) {
+ if (existingFunction.getFullHash() == hashFunction.getFullHash() &&
+ existingFunction.getSpecificHash() == hashFunction.getSpecificHash()) {
+ functionExists = true;
+ break;
+ }
+ }
+
+ if (functionExists) {
+ println("Function already exists in the database. Skipping.");
+ return;
+ }
+
fidDb.createNewFunction(library, hashFunction,
function.getName(disableNamespaceStripping), offset, domainFile.getName(),
hasTerminator);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fidDb.createNewFunction(library, hashFunction, | |
| function.getName(disableNamespaceStripping), offset, domainFile.getName(), | |
| hasTerminator); | |
| // Check if function already exists in the database | |
| List<FunctionRecord> existingFunctions = | |
| fidDb.findFunctionsByLibraryAndName(library, function.getName(disableNamespaceStripping)); | |
| boolean functionExists = false; | |
| for (FunctionRecord existingFunction : existingFunctions) { | |
| if (existingFunction.getFullHash() == hashFunction.getFullHash() && | |
| existingFunction.getSpecificHash() == hashFunction.getSpecificHash()) { | |
| functionExists = true; | |
| break; | |
| } | |
| } | |
| if (functionExists) { | |
| println("Function already exists in the database. Skipping."); | |
| return; | |
| } | |
| fidDb.createNewFunction(library, hashFunction, | |
| function.getName(disableNamespaceStripping), offset, domainFile.getName(), | |
| hasTerminator); |
| try { | ||
| fidDb = fidFile.getFidDB(true); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing exception handling.
The code opens the FID database but doesn't handle specific exceptions that might occur during this operation, such as file not found, permission issues, or database corruption.
Consider adding specific exception handling:
try {
fidDb = fidFile.getFidDB(true);
+ } catch (IOException e) {
+ printerr("Failed to open FID database: " + e.getMessage());
+ return;
Committable suggestion skipped: line range outside the PR's diff.
Fixes NationalSecurityAgency#5212
Introduction
As described in the linked issue there is currently no way to add a single function to a FID DB which is something I have wanted for a long time as well. The Ghidra script
AddSingleFunction.javanow implements this functionality.Overview of the script
Prerequisites:
The script checks if there is any available library in the FID DB. If a library exists the user can select it. If not the user will be prompted to create one. The user can then decide whether to preserve the namespaces of the selected function or save only the basename. Finally the function entry is saved.
Testing
Empty FID DB (create new library)
Select a function (
rayon::main):Run
AddSingleFunction.java:List the FID DB content with
ListFunctions.java:Non-empty FID DB (already contains at least one library)
Select a function (
crossbeam_deque::deque::Stealer<T>::steal):Run
AddSingleFunction.java:List the FID DB content with
ListFunctions.java:Additional notes
I had to make
FidServiceLibraryIngestandfindTerminator()public because they are required to calculatehasTerminatorwhich is passed tocreateNewFunction(). If this is not allowed let me know and I will duplicatefindTerminator()instead.Summary by CodeRabbit
New Features
Improvements