support fetching original package names and import paths via reflection #904
Conversation
This function returns obfuscated names, so use that as its name. Moreover, some of the callers still called the result "objStr", which misled me into thinking the string was a unique object path. Leave a TODO behind about using go/types/objectpath too. Our own recordedObjectString is sort of similar, but not as principled.
Only two callers did pass nil, and there's no reason for them to do so. They should be the ones to check that typeToObj did not return nil.
There's no need to reach for sharedCache.ListedPackages when the caller is computePkgCache, which already has the lpkg value. While here, compare *go/types.Package pointers directly rather than via their import path strings.
Reflection can show package names alone via reflect.Type.String, and I'm sure they are available in other ways. We were obfuscating "package p" exactly like the import path "foo.com/p" which worked OK for the most part, but not for reflection. This is also a slight improvement to the quality of the obfuscation, given that package names and import paths will no longer be aligned when obfuscated.
obfuscatedImportPath already handled ToObfuscate, so the callers don't have to do that as well. Handle main packages too, whose logic was sprinkled and repeated throughout the project.
Just like we support fetching original type, field, and method names via reflection to ensure that no Go package is broken by garble, do the same for package names and import paths, which are reachable too. This means we can remove the test function printfWithoutPackage, which in hindsight should have been a clue as we were working around a bug in garble. Updates burrowers#849.
| // At least one type in this package is used with reflection; | ||
| // the package name and import path are reachable via reflection as well. | ||
| // Note that package main is compiled as having import path "main" as well. | ||
| ri.result.ReflectObjectNames[ri.lpkg.obfuscatedPackageName()] = ri.lpkg.Name | ||
| if ri.lpkg.Name != "main" { | ||
| ri.result.ReflectObjectNames[ri.lpkg.obfuscatedImportPath()] = ri.lpkg.ImportPath | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this is too broad, reflection is very common I expect this would exclude most import paths of real world packages from obfuscation.
Maybe instead hide this behavior behind a flag like -includePaths, or make it opt-in on a package level via a magic comment/ constant. Or maybe even add the ability to exclude specific packages from obfuscation via the inverse of GOGARBLE.
There was a problem hiding this comment.
Yes, this would lower the quality of obfuscation in some cases, but I strongly believe that the default behavior should be for any Go package to work out of the box. Stronger obfuscation which may break some code should be opt-in, not opt-out. That is better UX - stronger obfuscation is a bit of a power user feature - and should also prevent us from getting dozens more "garble breaks with package X" issues filed when the problem tends to be related to reflection.
So, yes, we can and should add some way to opt into stronger obfuscation even when reflection is used. I would suggest that we tackle that in a follow-up PR because it would be a slightly involved change. I lean towards some sort of directive comment like you suggest.
There was a problem hiding this comment.
I think this is very similar to giving up on import path obfuscation entirely. I also think that import path obfuscation is one of the most important things we do.
Other than wails I think there has been no one who's programs break due to import path obfuscation, so I think this is a rare edge case and should be opt-in.
| if obj == nil { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
I think that the obfuscated name of a nil obj is "" is fine. I don't see a benefit of the callers having to make sure obj is non-nil.
There was a problem hiding this comment.
I don't think this is a big deal either way, but I personally disagree - a nil obj is an edge case that we should break out of as soon as reasonable, rather than keep going down the logic. All other callers already ensure that obj is non-nil, it was just two that were careless. If any code path ends up with a nil obj, that's possibly a bug that we need to investigate and fix - rather than silently assuming that the result is an empty string.
| if usedForReflect(ri.result, obj) { | ||
| if obj != nil && ri.usedForReflect(ri.result, obj) { |
| if usedForReflect(ri.result, obj) { | ||
| if obj != nil && ri.usedForReflect(ri.result, obj) { |
| ! binsubstr main$exe 'garble_main.go' 'test/main' 'importedpkg.' 'DownstreamObfuscated' 'SiblingObfuscated' 'IndirectObfuscated' 'IndirectNamedWithoutReflect' 'AliasIndirectNamedWithReflect' 'AliasIndirectNamedWithoutReflect' 'FmtTypeField' 'LocalObfuscated' | ||
| ! binsubstr main$exe 'garble_main.go' 'DownstreamObfuscated' 'SiblingObfuscated' 'IndirectObfuscated' 'IndirectNamedWithoutReflect' 'AliasIndirectNamedWithReflect' 'AliasIndirectNamedWithoutReflect' 'FmtTypeField' 'LocalObfuscated' |
There was a problem hiding this comment.
I would like to see importedpkg replaced by indirect to check that only certain import paths are excluded from obfuscation. Also check that importedpkg can be found in the binary with binsubstr.
There was a problem hiding this comment.
yeah it's a fair point - I should update this PR to ensure that package import paths unrelated to reflection are excluded from the binary.
There was a problem hiding this comment.
I meant that once this is opt-in you show that one packages import path is obfuscated while another's is not although both packages use reflection.
(see commit messages - please do not squash)
Updates #849.