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

Better reflection detection #380

Merged
merged 1 commit into from
Sep 13, 2021
Merged

Better reflection detection #380

merged 1 commit into from
Sep 13, 2021

Conversation

lu4p
Copy link
Member

@lu4p lu4p commented Aug 10, 2021

No description provided.

@lu4p lu4p changed the title reflectionPOC WIP: better reflection detection POC Aug 10, 2021
@lu4p lu4p changed the title WIP: better reflection detection POC Better reflection detection Sep 4, 2021
Copy link
Member

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add more tests? Including that e.g. fmt.Printf("%s\n", myType) doesn't prevent a named type from being obfuscated, as that would have too many false positives.

Another test to include is a user-written API that uses reflection, e.g. defined in a sub-package inside the testdata input module.

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
@capnspacehook
Copy link
Member

Can we test that some simple protobuf code works correctly? Maybe some example code?

@lu4p
Copy link
Member Author

lu4p commented Sep 5, 2021

Can we test that some simple protobuf code works correctly? Maybe some example code?

I tested the go examples at https://github.com/protocolbuffers/protobuf/tree/master/examples, works!

@lu4p lu4p requested a review from mvdan September 5, 2021 15:57
@capnspacehook
Copy link
Member

Sweet, we don't have to do it for this pr but you guys think it's possible to test against protobuf code in our tests?

@mvdan
Copy link
Member

mvdan commented Sep 10, 2021

Sweet, we don't have to do it for this pr but you guys think it's possible to test against protobuf code in our tests?

That's #240 :) protobuf is indeed one of the best candidates, due to its use of reflection and new-ish Go features like type aliases.

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
@lu4p
Copy link
Member Author

lu4p commented Sep 12, 2021

@mvdan @capnspacehook I think this is ready.

We should probably also tag a new release once this is merged.

Copy link
Member

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Thanks for bearing with me with the reviews. This is non-trivial, and I've been travelling for a week.

There are some edge cases that this PR might not cover yet; for example:

func indirectlyViaAddress(v interface{}) {
    _ = reflect.TypeOf(&v)
}

func indirectlyViaMethod(v SelfInterface) {
    _ = reflect.TypeOf(v.Self())
}

func indirectlyViaVariable(v interface{}, someCond bool) {
    if someCond {
        x := v
        _ = reflect.TypeOf(x)
    }
}

We can add more tests and improve with future PRs. Just something for you to keep in mind.

README.md Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@lu4p lu4p closed this Sep 13, 2021
Functions which use reflection on one of their parameters are,
now added to knownReflectAPIs automatically.

This makes most explicit hints for reflection redundant.
Simple protobuf code now works correctly when obfuscated.

Fixes #162
Fixes #373
@lu4p lu4p reopened this Sep 13, 2021
@lu4p lu4p merged commit aafd845 into burrowers:master Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants