We can now use pruned module graphs in go.mod files,
and we no longer need to worry about runtime/internal/sys.
Note that I had to update testdata/mod slightly,
as the new pruned module graphs algorithm downloads an extra go.mod file.
This change also paves the way towards future Go 1.18 support.
Thanks to lu4p for cleaning up two TODOs as well.
Co-Authored-By: lu4p <lu4p@pm.me>
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#162Fixes#373
Before, we would just notice direct calls to reflect's TypeOf and
ValueOf. Any other uses of reflection, such as encoding/json or
google.golang.org/protobuf, would require hints as documented by the
README.
Issue #162 outlines some ways we could fix this issue in a general way,
automatically detecting what functions use reflection on their parameters,
even for third party API funcs.
However, that goal is pretty significant in terms of code and effort.
As a temporary improvement, we can expand the list of "known" reflection
APIs via a static table.
Since this table is keyed by "func full name" strings, we could
potentially include third party APIs, such as:
google.golang.org/protobuf/proto.Marshal
However, for now simply include all the std APIs we know about.
If we fail to do the proper fix for automatic detection in the future,
we can then fall back to expanding this global table for third parties.
Update the README's docs, to clarify that the hint is not always
necessary anymore.
Also update the reflect.txt test to stop using the hint for encoding/json,
and to also start testing text/template with a method call.
While at it, I noticed that we weren't testing the println outputs,
as they'd go to stderr - fix that too.
Updates #162.
Every now and then, a CI run would fail:
FAIL: testdata/scripts/reflect.txt:7: unexpected match for ["main.go"] in main
These were rare, and very hard to reproduce or debug.
My best guess is that, since "main.go" is a short string and we use
random eight-character obfuscated filenames ending with ".go", it was
possible that the random filename happened to end in "main" in some
cases.
Given the base64 encoding, the chances of a single suffix collision are
about 0.000006%. Note, however, that a single obfuscated build will most
likely obfuscate many filenames, especially for the tests obfuscating
multiple packages. For a single CI run with many tests across three OSs,
the chances of any collision are likely very low, but realistic.
All this has a simple fix: use longer filenames to match with. We choose
"garble_main.go" since it's long enough, but also because it's still
clear it's a "main" Go file, and it's very unlikely to cause conflicts
with filenames in upstream Go given the "garble_" prefix.
Before this change, we would try to never obfuscate alias names. That
was far from ideal, as they can end up in field names via anonymous
fields.
Even then, we would sometimes still fail to build, because we would
inconsistently obfuscate alias names. For example, in the added test
case:
--- FAIL: TestScripts/syntax (0.23s)
testscript.go:397:
> env GOPRIVATE='test/main,private.source'
> garble build
[stderr]
# test/main/sub
Lv_a8gRD.go:15: undefined: KCvSpxmQ
To fix this problem, we set obj to be the TypeName corresponding to the
alias when it is used as an embedded field. We can then make the right
choice when obfuscating the name.
Right now, all aliases will be obfuscated. A TODO exists about not
obfuscating alias names when they're used as embedded fields in a struct
type in the same package, and that package is used for reflection -
since then, the alias name ends up as the field name.
With these changes, the protobuf module now builds.
obfuscatedTypesPackage is used to figure out if a name in a dependency
package was obfuscated or not. For example, if that package used
reflection on a named type, it wasn't obfuscated, so we must have the
same information to not obfuscate the same name downstream.
obfuscatedTypesPackage could return nil if the package was indirectly
imported, though. This can happen if a direct import has a function that
returns an indirect type, or if a direct import exposes a name that's a
type alias to an indirect type.
We sort of dealt with this in two pieces of code by checking for
obfPkg!=nil, but a third one did not have this check and caused a panic
in the added test case:
--- FAIL: TestScripts/reflect (0.81s)
testscript.go:397:
> env GOPRIVATE=test/main
> garble build
[stderr]
# test/main
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x8a5e39]
More importantly though, the nil check only avoids panics. It doesn't
fix the root cause of the problem: that importcfg does not contain
indirectly imported packages. The added test case would still fail, as
we would obfuscate a type in the main package, but not in the indirectly
imported package where the type is defined.
To fix this, resurrect a bit of code from earlier garble versions, which
uses "go list -toolexec=garble" to fetch a package's export file. This
lets us fill the indirect import gaps in importcfg, working around the
problem entirely.
This solution is still not particularly great, so we add a TODO about
possibly rethinking this in the future. It does add some overhead and
complexity, though thankfully indirect imports should be uncommon.
This fixes a few panics while building the protobuf module.
Our allPkgs boolean logic was wrong, because it could still lead to
garble obfuscating a type when used in the main package, but not in its
defining package. The added test case shows such a case.
To fix that, use a package path to only record the named objects from
the target package, which is a narrower operation without this problem,
but still makes all our tests pass.
This makes the google.golang.org/protobuf/internal/filetype package
start building.
If a package A imports package B, and uses reflect.TypeOf on an unnamed
struct type B.T (such as an alias), we don't want to record B.T's fields
as "do not obfuscate". This is for the same reason that we don't if B.T
is a named struct type: the detection only works for the package
defining the type, as otherwise it's inconsistent.
We failed to handle this case well, because we assumed all struct types
would be under some named type. This is not the case for type aliases.
Fortunately, struct fields are named, and as such they are objects.
Check their package too, just like we do for named types.
Fixes another build error when obfuscating the protobuf module.
We add a simplified version of the example above as a test case,
which originated from debugging the protobuf build failure.
The detection of reflection usage is tricky and there are plenty of edge
cases to test for. We definitely want one script for it, rather than
splitting those cases between other scripts like imports.txt and
syntax.txt.
Moreover, those two were rather generic and large, so this helps keep a
balance.
Daniel Martí
lu4p