default to GOGARBLE=*, stop using GOPRIVATE #616
Conversation
We can drop the code that kicked in when GOGARBLE was empty. We can also add the value in addGarbleToHash unconditionally, as we never allow it to be empty. In the tests, remove all GOGARBLE lines where it just meant "obfuscate everything" or "obfuscate the entire main module". cgo.txtar had "obfuscate everything" as a separate step, so remove it entirely. linkname.txtar started failing because the imported package did not import strings, so listPackage errored out. This wasn't a problem when strings itself wasn't obfuscated, as transformLinkname silently left strings.IndexByte untouched. It is a problem when IndexByte does get obfuscated. Make that kind of listPackage error visible, and fix it. reflect.txtar started failing with "unreachable method" runtime throws. It's not clear to me why; it appears that GOGARBLE=* makes the linker think that ExportedMethodName is suddenly unreachable. Work around the problem by making the method explicitly reachable, and leave a TODO as a reminder to investigate. Finally, gogarble.txtar no longer needs to test for GOPRIVATE. The rest of the test is left the same, as we still want the various values for GOGARBLE to continue to work just like before. Fixes burrowers#594.
|
This is a big change, so I'd really like two reviews :) |
|
This should also help |
|
grr, gotip still finds that method unreachable. no more time to investigate right now. |
|
Figured it out. The compiler and linker work together to track when reflection is used, because when it is, the toolchain deletes less "dead code" to keep APIs like MethodByName working. However, since we obfuscate the reflect package, then the toolchain thinks that reflection isn't being used. Which results in code used via reflection getting eliminated. Oops. This is a bug in master already; we just didn't notice because the reflect test didn't run with |
|
From the linker's |
We were obfuscating reflect's package path and its declared names,
but the toolchain wants to detect the presence of method reflection
to turn down the aggressiveness of dead code elimination.
Given that the obfuscation broke the detection,
we could easily end up in crashes when making reflect calls:
fatal error: unreachable method called. linker bug?
goroutine 1 [running]:
runtime.throw({0x50c9b3?, 0x2?})
runtime/panic.go:1047 +0x5d fp=0xc000063660 sp=0xc000063630 pc=0x43245d
runtime.unreachableMethod()
runtime/iface.go:532 +0x25 fp=0xc000063680 sp=0xc000063660 pc=0x40a845
runtime.call16(0xc00010a360, 0xc00000e0a8, 0x0, 0x0, 0x0, 0x8, 0xc000063bb0)
runtime/wcS9OpRFL:728 +0x49 fp=0xc0000636a0 sp=0xc000063680 pc=0x45eae9
runtime.reflectcall(0xc00001c120?, 0x1?, 0x1?, 0x18110?, 0xc0?, 0x1?, 0x1?)
<autogenerated>:1 +0x3c fp=0xc0000636e0 sp=0xc0000636a0 pc=0x462e9c
Avoid obfuscating the three names which cause problems: "reflect",
"Method", and "MethodByName".
While here, we also teach obfuscatedImportPath to skip "runtime",
as I also saw that the toolchain detects it for many reasons.
That wasn't a problem yet, as we do not obfuscate the runtime,
but it was likely going to become a problem in the future.
|
Sent the fix as a separate commit as it was a separate bug to be solved. |
|
@capnspacehook @pagran if either of you can review as well, I can do a release next week :) Otherwise it might jump to January, as I'll be off during the holidays. |
|
Running out of time to do a release before the holidays, so I'm merging with one review. Always happy to get more reviews post-merge. |
(see commit message)
Fixes #594.