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.
Otherwise, the added test case would fail, as we don't modify the C code
and so there would be a name mismatch.
In the far future we might start modifying Go names in C code,
similar to what we did for Go assembly,
but right now that seems out of scope and too complex.
An easier fix is to simply record those (hopefully few) names in ignoreObjects.
While at it, recordReflectArgs started to really outgrow its name, as it
also collected expressions used as constants for literal obfuscation.
Give it a better name.
Fixes#366.
With the -literals flag, we try to convert some const declarations to
vars, as long as that doesn't break typechecking. We really only do that
for typed constant strings, really.
There was a quirk: if a numerical constant had a type and used iota, we
would not obfuscate its value, but we would still convert the
declaration from const to var. Since iotas only work within const
declarations, that would break compilation:
> garble -literals build
[stderr]
# test/main
FeWE3zwi.go:19: undefined: iota
exit status 2
To fix the problem, make the logic more conservative: only obfuscate
constant declarations where the values are typed strings, meaning that
any numerical constants are left entirely untouched.
This fixes the build of google.golang.org/protobuf/runtime/protoiface
with -literals turned on.
Back in early April we added initial support for Go 1.17,
working on a commit from master at that time. For that to work, we just
needed to add a couple of packages to runtimeRelated and tweak printFile
a bit to not break the new "//go:build" directives.
A significant amount of changes have landed since, though, and the tests
broke in multiple ways.
Most notably, the new register ABI is enabled by default for GOOS=amd64.
That affected garble indirectly in two ways: there's a new internal
package to add to runtimeRelated, and we must make reverse.txt more
clever in making its output constant across ABIs.
Another noticeable change is that Go 1.17 changes how its own version is
injected into the runtime package. It used to be via a constant in
runtime/internal/sys, such as:
const TheVersion = `devel ...`
Since we couldn't override such constants via the linker's -X flag,
we had to directly alter the declaration while compiling.
Thankfully, Go 1.17 simply uses a "var buildVersion string" in the
runtime package, and its value is injected by the linker.
This means we can now override it with the linker's -X flag.
We make the code to alter TheVersion for Go 1.16 a bit more clever,
to not break the package when building with Go 1.17.
Finally, our hack to work around ambiguous TOOLEXEC_IMPORTPATH values
now only kicks in for non-test packages, since Go 1.17 includes our
upstream fix. Otherwise, some tests would end up with the ".test"
variant suffix added a second time:
test/bar [test/bar.test] [test/bar [test/bar.test].test]
All the code to keep compatibility with Go 1.16.x remains in place.
We're still leaving TODOs to remind ourselves to remove it or simplify
it once we remove support for 1.16.x.
The 1.17 development freeze has already been in place for a month,
and beta1 is due to come this week, so it's unlikely that Go will change
in any considerable way at this point. Hence, we can say that support
for 1.17 is done.
Fixes#347.
Our recent work in fieldToAlias worked well when the embedded field
declaration (using an alias) was in the same package as the use of that
field. We would have the *ast.Ident for the field declaration, so
types.Info.Uses would give us the TypeName for the alias.
Unfortunately, if the declaration was in a dependency package, we did
not have that same *ast.Ident, as we weren't parsing the source code for
dependencies for type-checking. This resulted in us incorrectly
obfuscating the use of such an embedded field:
> garble build
[stderr]
# test/main
JtzmzxWf.go:4: unknown field 'ExternalForeignAlias' in struct literal of type _BdSNiEL.Vcs_smer
To fix this, look through the direct imports of the package defining the
field to find an alias under the exact same name. Not a foolproof
solution, as there's a TODO, but it should work for most cases.
Fixes the obfuscation of google.golang.org/grpc/internal/status, too.
Updates #349.
The added test case, which is obfuscating and linking os/user, would fail
before this fix:
> garble build
[stderr]
# test/main
/usr/lib/go/pkg/tool/linux_amd64/link: running gcc failed: exit status 1
/usr/bin/ld: $WORK/.tmp/go-link-073246656/go.o: in function `Chz0Yfs2._cgo_cmalloc':
go.go:(.text+0x993cc): undefined reference to `Chz0Yfs2.runtime_throw'
/usr/bin/ld: $WORK/.tmp/go-link-073246656/go.o: in function `Chz0Yfs2.tDfhQ8uK':
go.go:(.text+0x99801): undefined reference to `Chz0Yfs2._cgo_runtime_gostring'
/usr/bin/ld: go.go:(.text+0x9982a): undefined reference to `Chz0Yfs2._cgo_runtime_gostring'
/usr/bin/ld: go.go:(.text+0x99853): undefined reference to `Chz0Yfs2._cgo_runtime_gostring'
collect2: error: ld returned 1 exit status
The reason is that we would alter the linkname directives of cgo-generated
code, but we would not obfuscate the code itself at all.
The generated code would end up being transformed into:
//go:linkname zh_oKZIy runtime.throw
func runtime_throw(string)
One can clearly see the error there; handleDirectives obfuscated the
local linkname name, but since transformGo didn't run, the actual Go
declaration was not obfuscated in the same way. Thus, the linker fails
to find a function body for runtime_throw, and fails.
The solution is simple: handleDirectives assumes that it's running on
code being obfuscated, so only run it when transformGo is running.
We can also remove the cgo skip check in handleDirectives, as it never
runs on cgo-generated code now.
Fixes a number of build errors that have been noticed since
907aebd770.
When such an alias name was used to define an embedded field, we handled
that case gracefully via the code using:
tf.info.Uses[node].(*types.TypeName)
Unfortunately, when the same field name was used elsewhere, such as a
composite literal, tf.Info.Uses gave us a *types.Var, not a
*types.TypeName, meaning we could no longer tell if this was an alias,
or what it pointed to.
Thus, we failed to obfuscate the name properly in the added test case:
> garble build
[stderr]
# test/main/sub
xxWZf66u.go:36: unknown field 'foreignAlias' in struct literal of type smhWelwn
It doesn't seem like any of the go/types APIs allows us to obtain the
*types.TypeName directly in this scenario. Thus, use a trick that we
used before: after typechecking, but before obfuscating, record all
embedded struct field *types.Var which are aliases via a map, where the
value holds the *types.TypeName for the alias.
Updates #349.
We already added support for "//go:embed" with string and []byte,
by not obfuscating the "embed" import path.
However, embed.FS was still failing:
> garble build
[stderr]
# test/main
:13: go:embed cannot apply to var of type embed.WtKNvwbN
The compiler detects the type by matching its name to exactly "embed.FS",
so don't obfuscate the name "FS" either.
While at it, ensure that the embed code behaves the same with "go build".
Updates #349.
Before, "garble build -h" would print the same as "garble -h", which is
too much and confusing, as it doesn't tell us much about "build".
Now it's far better, and includes the output of "go build -h":
$ garble build -h
usage: garble [garble flags] build [arguments]
This command wraps "go build". Below is its help:
usage: go build [-o output] [build flags] [packages]
Run 'go help build' for details.
We do the same for "garble reverse -h", since it doesn't wrap a Go tool
command.
I added a second one recently without noticing, since the original was
too far below and I had written commands above it.
Deduplicate them, and put the new env line as early as possible, to
prevent further issues and confusion.
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.
I've wanted this to more easily debug build failures.
To not force a build failure in a test script, as that would require
some trickery to remain stable, we use runtime.Caller without printing
the line number. Before this patch, those filenames without line numbers
would not be reversed at all.
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.
We were using temporary filenames for modified Go and assembly files.
For example, an obfuscated "encoding/json/encode.go" would end up as:
/tmp/garble-shared123/encode.go.456.go
where "123" and "456" are random numbers, usually longer.
This was usually fine for two reasons:
1) We would add "/tmp/garble-shared123/" to -trimpath, so the temporary
directory and its random number would be invisible.
2) We would add "//line" directives to the source files, replacing
the filename with obfuscated versions excluding any random number.
Unfortunately, this broke in multiple ways. Most notably, assembly files
do not have any line directives, and it's not clear that there's any
support for them. So the random number in their basename could end up in
the binary, breaking reproducibility.
Another issue is that the -trimpath addition described above was only
done for cmd/compile, not cmd/asm, so assembly filenames included the
randomized temporary directory.
To fix the issues above, the same "encoding/json/encode.go" would now
end up as:
/tmp/garble-shared123/encoding/json/encode.go
Such a path is still unique even though the "456" random number is gone,
as import paths are unique within a single build.
This fixes issues with the base name of each file, so we no longer rely
on line directives as the only way to remove the second original random
number.
We still rely on -trimpath to get rid of the temporary directory in
filenames. To fix its problem with assembly files, also amend the
-trimpath flag when running the assembler tool.
Finally, add a test that reproducible builds still work when a full
rebuild is done. We choose goprivate.txt for such a test as its
stdimporter package imports a number of std packages, including uses of
assembly and cgo.
For the time being, we don't use such a "full rebuild" reproducibility
test in other test scripts, as this step is expensive, rebuilding many
packages from scratch.
This issue went unnoticed for over a year because such random numbers
"123" and "456" were created when a package was obfuscated, and that
only happened once per package version as long as the build cache was
kept intact.
When clearing the build cache, or forcing a rebuild with -a, one gets
new random numbers, and thus a different binary resulting from the same
build input. That's not something that most users would do regularly,
and our tests did not cover that edge case either, until now.
Fixes#328.
I've tripped over this GODEBUG env var four or five times already.
Since it affects any Go program by making them print tons of runtime
information, it also affects "go env", breaking garble horribly.
To prevent further issues, unset the env var when done.
Packages P1 and P2 can define identical struct types T1 and T2, and one
can convert from type T1 to T2 or vice versa.
The spec defines two identical struct types as:
Two struct types are identical if they have the same sequence of
fields, and if corresponding fields have the same names, and
identical types, and identical tags. Non-exported field names
from different packages are always different.
Unfortunately, garble broke this: since we obfuscated field names
differently depending on the package, cross-package conversions like the
case above would result in typechecking errors.
To fix this, implement Joe Tsai's idea: hash struct field names with the
string representation of the entire struct. This way, identical struct
types will have their field names obfuscated in the same way in all
packages across a build.
Note that we had to refactor "reverse" a bit to start using transformer,
since now it needs to keep track of struct types as well.
This failure was affecting the build of google.golang.org/protobuf,
since it makes regular use of cross-package struct conversions.
Note that the protobuf module still fails to build, but for other
reasons. The package that used to fail now succeeds, so the build gets a
bit further than before. #240 tracks adding relevant third-party Go
modules to CI, so we'll track the other remaining failures there.
Fixes#310.
Obfuscating some std packages for windows/arm triggered a bug; when
encountering a call to runtime·memmove, we'd hash "memmove" with the
current package's action ID.
This is wrong on two levels: First, we aren't obfuscating the runtime
package yet. And second, if we did, we would have to hash the symbol
appropriately, with that package's action ID.
For now, only hashing the local names does the trick. That's all that
the code currently supports, anyway.
Since 1a8e32227f we have been setting line numbers on call sites, and
that line number is always the minimum value for -tiny: 1.
Still not as zero-overhead as the old mechanism which entirely deleted
line numbers, but a useless and small line number is still pretty good.
THey don't show up in stack traces, and if they show up in regular
program output, we should in theory not obfuscate those names via the
detection of reflection.
However, there's one relatively common scenario where obfuscated field
names can appear: in build error messages, when obfuscation fails a
build.
Our previous logic only took care of fairly simple types, such as a
simple struct or a pointer to a struct. If we had a struct embedding
another struct, we'd fail to record the objects for the fields in the
inner struct, and that would lead to miscompilation:
> garble build
[stderr]
# test/main
LZmt64Nm.go:7: outer.InnerField undefined (type *CcUt1wkQ.EmbeddingOuter has no field or method InnerField)
To fix this issue, make the function that records all objects under a
types.Type smarter. Since it now does more than just dealing with
structs, it's also renamed.
Since the function now walks types properly, we get to remove the extra
ast.Inspect in recordReflectArgs, which is nice.
We also make it a method, to avoid the map parameter. A boolean
parameter is also added, since we need this feature to only look at the
current package when looking at reflect calls.
Finally, we add a test case, a simplified version of the original bug
report.
Fixes#315.
The docs for go/importer.ForCompiler say:
The lookup function is called each time the resulting importer
needs to resolve an import path. In this mode the importer can
only be invoked with canonical import paths (not relative or
absolute ones); it is assumed that the translation to canonical
import paths is being done by the client of the importer.
We use a lookup func for two reasons: first, to support modules, and
second, to be able to use our information from "go list -json -export".
However, go/types does not canonicalize import paths before calling
ImportFrom. This is somewhat understandable; it doesn't know whether an
importer was created with a lookup func, and ImportFrom only requires
the input path to be canonicalized in that scenario. When the lookup
func is nil, the importer canonicalizes by itself via go/build.Import.
Before this change, the added crossbuild test would fail:
> garble build net/http
[stderr]
# vendor/golang.org/x/crypto/chacha20
typecheck error: /usr/lib/go/src/vendor/golang.org/x/crypto/chacha20/chacha_generic.go:10:2: could not import crypto/cipher (can't find import: "crypto/cipher")
# vendor/golang.org/x/text/secure/bidirule
typecheck error: /usr/lib/go/src/vendor/golang.org/x/text/secure/bidirule/bidirule.go:12:2: could not import errors (can't find import: "errors")
# vendor/golang.org/x/crypto/cryptobyte
typecheck error: /usr/lib/go/src/vendor/golang.org/x/crypto/cryptobyte/asn1.go:8:16: could not import encoding/asn1 (can't find import: "encoding/asn1")
# vendor/golang.org/x/text/unicode/norm
typecheck error: /usr/lib/go/src/vendor/golang.org/x/text/unicode/norm/composition.go:7:8: could not import unicode/utf8 (can't find import: "unicode/utf8")
This is because we'd fall back to importer.Default, which only knows how
to find packages in $GOROOT/pkg. Those are missing for cross-builds,
unsurprisingly, as those built archives end up in the build cache.
After this change, we properly support importing std-vendored packages,
so we can get rid of the importer.Default workaround. And, by extension,
cross-builds now work as well.
Note that, in the added test script, the full build of the binary fails,
as there seems to be some sort of linker problem:
> garble build
[stderr]
# test/main
d9rqJyxo.uoqIiDs5: relocation target runtime.os9A16A3 not defined
We leave that as a TODO for now, as this change is subtle enough as it
is.
In the worst case scenario, when GOPRIVATE isn't set at all, we would
run these three commands:
* "go env GOPRIVATE", to fetch GOPRIVATE itself
* "go list -m", for GOPRIVATE's fallback
* "go version", to check the version of Go being used
Now that we support Go 1.16 and later, all these three can be obtained
via "go env -json":
$ go env -json GOPRIVATE GOMOD GOVERSION
{
"GOMOD": "/home/mvdan/src/garble/go.mod",
"GOPRIVATE": "",
"GOVERSION": "go1.16.3"
}
Note that we don't get the module path directly, but we can use the
x/mod/modfile Go API to parse it from the GOMOD file cheaply.
Notably, this also simplifies our Go version checking logic, as now we
get just the version string without the "go version" prefix and
"GOOS/GOARCH" suffix we don't care about.
This makes our code a bit more maintainable and robust. When running a
short incremental build, we can also see a small speed-up, as saving two
"go" invocations can save a few milliseconds:
name old time/op new time/op delta
Build/Cache-8 168ms ± 0% 166ms ± 1% -1.26% (p=0.009 n=6+6)
name old bin-B new bin-B delta
Build/Cache-8 6.36M ± 0% 6.36M ± 0% +0.12% (p=0.002 n=6+6)
name old sys-time/op new sys-time/op delta
Build/Cache-8 222ms ± 2% 219ms ± 3% ~ (p=0.589 n=6+6)
name old user-time/op new user-time/op delta
Build/Cache-8 857ms ± 1% 846ms ± 1% -1.31% (p=0.041 n=6+6)
In ce2c45440a, we simplified the code a bit and removed one call to
obfuscatedTypesPackage.
Unfortunately, we introduced a regression; if an exported function is
linknamed to another symbol name, and it's called from an importer
package, we would have a build failure now:
> garble build
[stderr]
# test/main
ZiOACuw7.go:1: undefined: ODC0xN52.BaDqbhkj
This is because the imported package would not hash the original name,
via its ignoreObjects logic. And, since the importer package has no
access to that knowledge, it would hash the same name, and fail to find
it in the final build.
The regression happened because we used to have a types.Scope Lookup
that saved us in this scenario. Add the test, and re-add the Lookup,
this time only for this particular scenario with function names.
Thanks to Andrew LeFevre for reporting and describing the test case.
While at it, replace more uses of "garbled" to "obfuscated".
Since it changes rapidly, especially during merge cycles, and we don't
want CI to surprisingly blow up in our faces from one day to another.
Pin this to a commit from yesterday which works, since some changes
merged today moved where the Go build version is recorded and broke
garble.
While at it, replace "git clone" with a wget of a source archive. This
is much, much faster, mainly because a tarball is significantly smaller.
We now download about 20MiB instead of over 350MiB.
One downside is that, without git, make.bash can't construct a devel
version on its own. For that reason, add a pretty basic manual version
via the VERSION file.
This means that we must not reject custom devel version strings. This is
a good thing anyway, because custom devel strings are already common
when building Go in custom ways. Those people tend to be advanced users,
such as CI, so fall back to assuming they know what they are doing and
don't error.
Plus, starting last week, devel versions in Go master now contain the
major Go version like in build tags, such as "go1.17-commit...", so we
will soon start relying on that instead of parsing dates:
$ go version
go version devel go1.17-a7e16abb22 Thu Apr 8 07:33:58 2021 +0000 linux/amd64
Two bugs were remaining which made the build with -literals of std fail.
First, we were ignoring too many objects in constant expressions,
including type names. This resulted in type names declared in
dependencies which were incorrectly not obfuscated in the current
package:
# go/constant
O1ku7TCe.go:1: undefined: alzLJ5Fd.Word
b0ieEGVQ.go:1: undefined: alzLJ5Fd.Word
LEpgYKdb.go:4: undefined: alzLJ5Fd.Word
FkhHJCfm.go:1: undefined: alzLJ5Fd.Word
This edge case is easy to reproduce, so a test case is added to
literals.txt.
The second issue is trickier; in some packages like os/user, we would
get syntax errors because of comments printed out of place:
../tip/os/user/getgrouplist_unix.go:35:130: syntax error: unexpected newline, expecting comma or )
This is a similar kind of error that we tried to fix with e2f06cce94. In
particular, it's fixed by also setting CallExpr.Rparen in withPos. We
also add many other missing Pos fields for good measure, even though
we're not sure they help just yet.
Unfortunately, all my attempts to minimize this into a reproducible
failure have failed. We can't just copy the failing file from os/user,
as it only builds on some OSs. It seems like it was the perfect mix of
cgo (which adds line directive comments) plus unlucky positioning of
literals.
For that last reason, as well as for ensuring that -literals works well
with a wide variety of software, we add a build of all of std with
-literals when not testing with -short. This is akin to what we do in
goprivate.txt, but with the -literals flag. This does make "go test"
more expensive, but also more thorough.
Fixes#285, hopefully for good this time.
The regular obfuscation process simply modifies some simple nodes, such
as identifiers and strings. In those cases, we modify the nodes
in-place, meaning that their positions remain the same. This hasn't
caused any problems.
Literal obfuscation is trickier. Since we replace one expression with an
entirely different one, we use cursor.Replace. The new expression is
entirely made up on the spot, so it lacks position information.
This was causing problems. For example, in the added test input:
> garble -literals build
[stderr]
# test/main
dgcm4t6w.go:3: misplaced compiler directive
dgcm4t6w.go:4: misplaced compiler directive
dgcm4t6w.go:3: misplaced compiler directive
dgcm4t6w.go:6: misplaced compiler directive
dgcm4t6w.go:7: misplaced compiler directive
dgcm4t6w.go:3: misplaced compiler directive
dgcm4t6w.go:9: misplaced compiler directive
dgcm4t6w.go:3: misplaced compiler directive
dgcm4t6w.go:3: too many errors
The build errors are because we'd move the compiler directives, which
makes the compiler unhappy as they must be directly followed by a
function declaration.
The root cause there seems to be that, since the replacement nodes lack
position information, go/printer would try to estimate its printing
position by adding to the last known position. Since -literals adds
code, this would result in the printer position increasing rapidly, and
potentially printing directive comments earlier than needed.
For now, making the replacement nodes have the same position as the
original node seems to stop go/printer from making this mistake.
It's possible that this workaround won't be bulletproof forever, but it
works well for now, and I don't see a simpler workaround right now.
It would be possible to use fancier mechanisms like go/ast.CommentMap or
dave/dst, but those are a significant amount of added complexity as well.
Fixes#285.
Reduces "go test -short" with a warm build cache from ~9s to ~4s. The
main offender was the use of "-a" in the "garble test" call; I think I
added that flag to rebuild all packages when debugging an error, and
forgot to remove it.
Just the exit status code is used. Printing an error would be confusing,
since we're printing the input too. Plus, the error is binary anyway -
either we changed something, or we did not.
One could make a case for only printing if we changed anything, and
showing an error otherwise. But that would mean buffering the entire
input, which could be problematic or slow when e.g. reversing large log
files.
Fix up a few TODOs, and simplify the way we handle comments.
We now add whitespace around inline /*line*/ directives, to ensure we
don't break programs. A test case is added too.
We now add line directives to call sites, not function declarations,
since those are what actually shows up in stack traces.
It's unclear if we care about any other lines inside functions at all.
This also fixes reversing with -literals, since that feature adds a
significant amount of code which shuffles line numbers around.
Finally, we extend the tests with types, methods, and anonymous
functions, and we make all of them work well.
Updates #5.
In 6898d61637, we switched from using action IDs from "go list
-toolexec=garble" to those from the original "go list". We still wanted
the obfuscation and hashing to change if the version of garble changes,
so we hashed that "original action ID" with garble's own content ID, and
called the new hash "garble action ID".
While working on a different patch, I noticed something weird: with the
new mechanism, adding or removing flags like -literals did not alter
those hashes, unlike the old method. This is because the old method used
ownContentID, which includes such bits of information, but the new
method does not.
Change that, and add a test that locks in the behavior we want. In
seed.txt, we check that a single function name gets hashed in particular
ways in different scenarios.
Note that we use a mix of "cmp" and "! bincmp", since the former has no
negated form.
While at it, the seed.txt test is revamped a bit. Now, we only run with
-literals once, as this test is mainly about -seed. We also declare seed
strings once, as environment variables, which makes it easier to track
what each step is doing.
In particular, the positions within function declarations, including the
positions of call sites to other functions.
Note that this isn't well tested just yet, particularly not with other
features like -literals. We can extend the tests and code over time.
This gets us the core basics.
The issue will be closed once the feature is documented for users, in a
follow-up PR.
Updates #5.
Position information was obfuscated with math/rand manually, which meant
that the resulting positions were pretty small like "x.go:34", but they
were also very hard to reverse due to their short length and difficulty
to reproduce.
We now hash them with hashWith and the package's GarbleActionID:
"main.go:203" hashed with 933ad1c700755b7c3a9913c55cade1 to "mwu1xuNz.go"
The input to the hash is the base filename and the byte offset of the
declaration within the file, meaning that it's unique within a package.
The output filename is long enough to allow easy reversal.
The line number is always 1, since the information needed for reversing
is contained entirely within the filename. It doesn't really matter if
we encode data in the filename or line number, but it's easier for us to
use a string.
For #5.
Historically, it was impossible to rename those funcs as the
implementation was in assembly files, and we only transformed Go code.
Now that transformAsm exists, it's only about fifty lines to do some
very basic parsing and rewriting of assembly files.
This fixes the obfuscated builds of multiple std packages, including a
few dependencies of net/http, since they included assembly funcs which
called pure Go functions. Those pure Go functions had their names
obfuscated, breaking the call sites in assembly.
Fixes#258.
Fixes#261.
With the many refactors building up to v0.1.0, we broke "garble test" as
we no longer dealt with test packages well.
Luckily, now that we can depend on TOOLEXEC_IMPORTPATH, we can support
the test command again, as we can always figure out what package we're
currently compiling, without having to track a "main" package.
Note that one major pitfall there is test packages, where
TOOLEXEC_IMPORTPATH does not agree with ImportPath from "go list -json".
However, we can still work around that with a bit of glue code, which is
also copiously documented.
The second change necessary is to consider test packages private
depending on whether their non-test package is private or not. This can
be done via the ForTest field in "go list -json".
The third change is to obfuscate "_testmain.go" files, which are the
code-generated main functions which actually run tests. We used to not
need to obfuscate them, since test function names are never obfuscated
and we used to not obfuscate import paths at compilation time. Now we do
rewrite import paths, so we must do that for "_testmain.go" too.
The fourth change is to re-enable test.txt, and expand it with more
sanity checks and edge cases.
Finally, document "garble test" again.
Fixes#241.
Daniel Martí
lu4p
Andrew LeFevre