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

revised reflection handling #889

Merged
merged 1 commit into from
Nov 27, 2024
Merged

revised reflection handling #889

merged 1 commit into from
Nov 27, 2024

Conversation

lu4p
Copy link
Member

@lu4p lu4p commented Nov 23, 2024

(see commit message)

@lu4p

This comment was marked as outdated.

@lu4p

This comment was marked as outdated.

@lu4p lu4p changed the title WIP: revised reflection handling revised reflection handling Nov 25, 2024
@lu4p lu4p removed request for mvdan and pagran November 25, 2024 23:10
@lu4p lu4p requested a review from mvdan November 25, 2024 23:48
@lu4p

This comment was marked as outdated.

@lu4p lu4p requested a review from pagran November 26, 2024 12:42
shared.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 Outdated Show resolved Hide resolved
reflect_abi_patch.go Outdated Show resolved Hide resolved
reflect_abi_patch.go Show resolved Hide resolved
reflect_abi_patch.go Outdated Show resolved Hide resolved
reflect_abi_patch.go Outdated Show resolved Hide resolved
reflect_abi_patch.go Show resolved Hide resolved
@lu4p lu4p force-pushed the obfuscateAll branch 3 times, most recently from a4dc374 to 18fcf7c Compare November 27, 2024 17:06
reflect_abi_patch.go Outdated Show resolved Hide resolved
reflect_abi_patch.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
reflect_abi_patch.go Show resolved Hide resolved
reflect_abi_patch.go Show resolved Hide resolved
@lu4p lu4p force-pushed the obfuscateAll branch 3 times, most recently from f7e59c3 to e31525c Compare November 27, 2024 19:49
@lu4p lu4p requested a review from mvdan November 27, 2024 19:52
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.

SGTM - looks a lot better without all the pkgcache changes. I'm still mildly worried about the internal/abi performance but we can measure and improve that once it hits master. Please revert the change to the hashed name lengths though, as described.

reflect_abi_patch.go Show resolved Hide resolved
@mvdan
Copy link
Member

mvdan commented Nov 27, 2024

Also, how about this for a more detailed commit message:

obfuscate all names used in reflection

Go code can retrieve and use field and method names via the `reflect` package.
For that reason, historically we did not obfuscate names of fields and methods
underneath types that we detected as used for reflection, via e.g. `reflect.TypeOf`.

However, that caused a number of issues. Since we obfuscate and build one package
at a time, we could only detect when types were used for reflection in their own package
or in upstream packages. Use of reflection in downstream packages would be detected
too late, causing one package to obfuscate the names and the other not to, and a failure.

A different approach is implemented here. All names are obfuscated now, but we collect
those types used for reflection, and at the end of a build in `package main`,
we inject a function into the runtime's `internal/abi` package to reverse the obfuscation
for those names which can be used for reflection.

This does mean that the obfuscation for the names is very weak, as the binary
contains a one-to-one mapping to their original names, but they cannot be obfuscated
without breaking too many Go packages out in the wild. There is also some amount
of overhead in `internal/abi` due to this, but we aim to make the overhead insignificant.

Go code can retrieve and use field and method names via the `reflect` package.
For that reason, historically we did not obfuscate names of fields and methods
underneath types that we detected as used for reflection, via e.g. `reflect.TypeOf`.

However, that caused a number of issues. Since we obfuscate and build one package
at a time, we could only detect when types were used for reflection in their own package
or in upstream packages. Use of reflection in downstream packages would be detected
too late, causing one package to obfuscate the names and the other not to, leading to a build failure.

A different approach is implemented here. All names are obfuscated now, but we collect
those types used for reflection, and at the end of a build in `package main`,
we inject a function into the runtime's `internal/abi` package to reverse the obfuscation
for those names which can be used for reflection.

This does mean that the obfuscation for these names is very weak, as the binary
contains a one-to-one mapping to their original names, but they cannot be obfuscated
without breaking too many Go packages out in the wild. There is also some amount
of overhead in `internal/abi` due to this, but we aim to make the overhead insignificant.

Fixes #884, #799, #817, #881, #858, #843, #842

Closes #406
@lu4p lu4p merged commit 926f3de into master Nov 27, 2024
4 checks passed
@lu4p lu4p deleted the obfuscateAll branch November 27, 2024 21:38
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.

2 participants