-
Notifications
You must be signed in to change notification settings - Fork 574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: do not trim api names that include :: #1897
Conversation
would you consider adding a test for this? the routine you're updating is starting to look a little complicated, so maybe soon it's time to attempt to refactor, but without tests, i wouldn't be comfortable. even one trivial test would encode the special case/scenario that you encountered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, my bad. This is a pretty crucial step so I should have created more tests to prove it's working for all cases. Maybe we should add different trim/symbol generation functions for .NET vs. native vs. others.
Added an issue to track the potentially larger refactor. I think we can merge here with a test that shows the handling of the encountered case. |
@williballenthin @mr-tz I've added a simple to test to cover this specific case. I'd agree that a larger refactor may be needed here as now mentioned in #1899. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
capa v7 ignores the DLL portion of API features. Unfortunately the logic that implements this created a subtle bug that breaks matching for
.NET
api
features e.g.System.Convert::FromBase64String
. This PR updates thetrim_dll_part
function to fix the bug.Checklist