Skip to content

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 vivi365 requested a review from carminecesarano June 4, 2024 14:13
@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.

2 participants