Skip to content
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

reflection: method invocation approach. #5

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

reflection: method invocation approach. #5

wants to merge 4 commits into from

Conversation

vivi365
Copy link
Contributor

@vivi365 vivi365 commented Jun 3, 2024

This is not quite ready.

The blacklisted function approach is not sufficient. It e.g. does not catch m.Call() where m is of type reflect.Value and thus misses to catch the POC case.

I need to look into this a bit more

@vivi365
Copy link
Contributor Author

vivi365 commented Jun 4, 2024

To find calls via identifiers like m we need to type check the file.

v := reflect.ValueOf(f)
m := v.MethodByName("Method"
m.Call(nil)

This is implemented with packages go/importer and go/types.

This parser now catches the POC case!

CAVEATS:

  1. Type checking files: I encountered several errors of type checking files for go ethereum (importing issue). I chose to ignore the errors (could not find easy fix), which means some cases of reflection method invocations could potentially be missed.
// LIMITATION/TODO: not all files can be type checked due to import issues
if err != nil {
    //fmt.Printf("Error type-checking file %s: %v\n", path, err)
}
  1. Reported occurrences: For go-ethereum/accounts, capslock finds 9 occurrences, whereas gosurface finds 19.

I should look into the exact reported cases we get...

  1. Overhead?: I have not checked overhead for introducing type checking, but this was the only viable option I found

@vivi365
Copy link
Contributor Author

vivi365 commented Jun 4, 2024

Regarding point 2. Reported occurrences

As previously known, capslock does not check subdirectories. This is the reason it finds fewer occurrences (also capslock checks transitive uses as well, we do not do this, so it would still probably differ if checking subdirs).

@vivi365 vivi365 marked this pull request as ready for review June 4, 2024 14:33
@vivi365
Copy link
Contributor Author

vivi365 commented Jun 4, 2024

Note: should probably have proper tests...

@vivi365 vivi365 mentioned this pull request Jun 15, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant