FunctionID: Add option to disable namespace stripping #2
Conversation
|
There is a simple solution for this known issue actually:
Select the function and press L (rename):
Do not change anything just click OK. The namespace is registered:
|
|
Warning Rate limit exceeded@visz11 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughA new option to control namespace stripping was introduced throughout the FunctionID library creation workflow in Ghidra. This involved adding a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PopulateFidDialog
participant IngestTask
participant FidService
participant FidServiceLibraryIngest
User->>PopulateFidDialog: Open dialog
PopulateFidDialog->>User: Show "Disable namespace stripping" checkbox
User->>PopulateFidDialog: Submit selection
PopulateFidDialog->>IngestTask: Create task with disableNamespaceStripping
IngestTask->>FidService: createNewLibraryFromPrograms(..., disableNamespaceStripping, ...)
FidService->>FidServiceLibraryIngest: new(..., disableNamespaceStripping)
FidServiceLibraryIngest->>FidServiceLibraryIngest: hashAllTheFunctions(..., disableNamespaceStripping)
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/src/main/java/ghidra/feature/fid/plugin/PopulateFidDialog.java (1)
153-157: Good UI implementation with clear labeling.The checkbox is properly added to the panel with a descriptive label that appropriately marks this feature as experimental. Consider adding a tooltip to provide more information about this option.
panel.add(new GLabel("Disable namespace stripping (experimental)", SwingConstants.RIGHT)); namespaceStrippingCheckBox = new JCheckBox(); +namespaceStrippingCheckBox.setToolTipText("When enabled, namespaces will be preserved in function names. This helps prevent name clashes in languages like C++ and Rust."); namespaceStrippingCheckBox.addActionListener(e -> updateOkEnablement()); panel.add(namespaceStrippingCheckBox);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Ghidra/Features/FunctionID/ghidra_scripts/CreateMultipleLibraries.java(3 hunks)Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/plugin/IngestTask.java(3 hunks)Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/plugin/PopulateFidDialog.java(3 hunks)Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidService.java(2 hunks)Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidServiceLibraryIngest.java(7 hunks)
🔇 Additional comments (20)
Ghidra/Features/FunctionID/ghidra_scripts/CreateMultipleLibraries.java (3)
61-61: Field declaration is appropriately placed.The new boolean field
disableNamespaceStrippingis properly declared with the other class-level fields.
374-376: Good user interaction for the new option.This prompt appropriately asks users for their preference on namespace stripping when running the script in headless mode, ensuring consistency with the GUI option.
282-284: Parameter is correctly passed to the service method.The
disableNamespaceStrippingflag is properly passed to thecreateNewLibraryFromProgramsmethod, maintaining the correct method signature and parameter ordering.Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/plugin/PopulateFidDialog.java (3)
66-66: Field declaration is appropriately placed.The new checkbox field is properly declared with other UI component fields.
100-102: Clean helper method implementation.Good practice to create a dedicated method for retrieving the checkbox state, making the code more maintainable.
91-92: Parameter is correctly retrieved and passed.The namespace stripping option is properly retrieved and passed to the IngestTask constructor.
Also applies to: 95-95
Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/plugin/IngestTask.java (3)
48-48: Field declaration is appropriately placed.The new boolean field is properly declared with other class-level fields.
52-54: Constructor parameter and initialization are correctly implemented.The constructor signature update and field initialization look good.
Also applies to: 66-66
98-102: Parameter is correctly passed to the service method.The
disableNamespaceStrippingflag is properly passed to thefidService.createNewLibraryFromProgramsmethod.Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidService.java (3)
185-185: Good Javadoc update.The method's Javadoc is properly updated to document the new parameter.
195-196: Method signature update is correct.The method signature is properly updated to include the new parameter.
199-201: Parameter is correctly passed to the library ingest constructor.The
disableNamespaceStrippingflag is properly passed to theFidServiceLibraryIngestconstructor, completing the propagation chain from UI to core functionality.Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidServiceLibraryIngest.java (8)
52-52: Good addition of the disableNamespaceStripping flag.This private boolean field properly stores the namespace stripping preference that will be used throughout the class.
180-181: Constructor parameter and documentation looks good.The constructor has been properly updated to accept the new
disableNamespaceStrippingparameter with appropriate JavaDoc, and the parameter is correctly assigned to the instance variable.Also applies to: 185-186, 197-197
247-247: Parameter correctly passed to child method.The
disableNamespaceStrippingparameter is properly passed to thepopulateLibraryFromProgrammethod, maintaining the user preference throughout the call chain.
268-272: Method signature and documentation properly updated.The
populateLibraryFromProgrammethod signature has been updated to include the new parameter with appropriate JavaDoc.
279-279: Parameter correctly propagated to hashAllTheFunctions.The
disableNamespaceStrippingparameter is properly passed to the next method in the chain, ensuring consistent behavior.
337-343: Method signature and documentation properly updated for hashAllTheFunctions.The
hashAllTheFunctionsmethod has been correctly updated to accept the new parameter with appropriate JavaDoc.
1-661: Overall implementation is cohesive and well-structured.The changes consistently implement the new feature to optionally disable namespace stripping during FID database creation. The boolean parameter is properly propagated through the call chain from the constructor down to where function names are retrieved, making the feature work seamlessly while maintaining backward compatibility.
As noted in the PR objectives, this addresses issue NationalSecurityAgency#5858 where namespace stripping caused name clashes in languages like C++ and Rust where functions often share simple names but differ by namespace.
362-362:✅ Verification successful
Core implementation looks correct.
This is the key implementation line where the flag is actually used to determine whether namespace paths should be included in function names. The usage of
getName(disableNamespaceStripping)aligns with the PR objectives to optionally disable namespace stripping.Note that as mentioned in the PR objectives, this is marked as experimental since applying symbols from a .fidb file with namespaces may result in namespaces being embedded directly into function names.
🏁 Script executed:
#!/bin/bash # Check the Function interface to verify the getName method signature # that's being used in this implementation # Find the Function interface definition fd -t f "Function.java" | xargs grep -A 3 "getName.*boolean"Length of output: 419
🏁 Script executed:
#!/bin/bash # Locate Symbol.java files in the repository echo "Symbol.java files:" fd -t f "Symbol.java" # Check for the getName(boolean) signature in each Symbol.java echo echo "Checking for 'String getName(boolean' in Symbol.java:" fd -t f "Symbol.java" | xargs grep -nE "String\s+getName\s*\(\s*boolean"Length of output: 40802
Verified:
Symbol.getName(boolean)signature and semantics match the PR change.The call
name = function.getSymbol().getName(disableNamespaceStripping);correctly passes your experimental flag into
Symbol.getName(boolean includeNamespace), which exists inghidra/program/model/symbol/Symbol.javaand is overridden inGlobalSymbolandAbstractDBTraceSymbol. This aligns exactly with the PR’s goal to optionally include namespace paths.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Fixes NationalSecurityAgency#5858
Overview
The linked issue isnt tracked or assigned but it has been getting a lot of attention. I also run into this namespace stripping problem all the time when reversing Rust binaries and using function IDs.
When we create function ID database (.fidb) files, the populate functions strip the function namespaces. This is a well documented limitation/feature of the FID implementation.
The linked issue describes the problem: C++ (and Rust) code often has simple names that clash when you remove the namespace. I added a checkbox under
Tools -> Function ID -> Populate FidDb from programs...which can be used to disable namespace stripping. This checkbox is disabled by default so it does not affect the original behavior if left unchecked. Here is the key change inFidServiceLibraryIngest.javaat this line:before:
after:
where
functionis of typeFunctionwhich is an interface extendingNamespaceso this method is available:Using that we get the full symbol names including namespaces. The rest of the changes make sure the checkbox is added to the panel under
Tools -> Function ID -> Populate FidDb from programs...and the setting gets passed through the code properly. I also updatedCreateMultipleLibraries.javafor headless mode.Testing
Before:
Note that the namespaces of
drop_sloware stripped.After:
Note that the namespaces of
drop_sloware not stripped.Known issue
After running a FID analysis and applying symbols from a .fidb file (containing function names with namespaces), the namespaces end up getting embedded in the function name:
This is why I marked this feature as experimental for now. I think this is a small issue compared to the huge gain we can achieve with keeping the namespaces.
I intend to improve/fix this in the future.
Summary by CodeRabbit