Support Go plugins

[alexmaloteaux]

When a main go binary and a plugin are build through gradle with a common private import, i end up having an error message when loading the plugin :

plugin was built with a different version of package xxx.

Is it possible somehow to reuse previous build or im doing something wrong ?

[mvdan]

Can you share the precise steps that you followed? It doesn't matter if the code is private, I mainly want to see the commands you ran.

We already have a test using the plugin package here, so in principle it should work.

[alexmaloteaux]

i think the issue is there when both the loader and the plugin use a same local import

for instance :

myproject/cmd/loader/main.go
myproject/plugin/main.go
myproject/common/tools

if both main.go import tools then it fails with plugin was built with a different version of package myproject/common/tools

[mvdan]

Can you share the exact commands that you ran, please?

[alexmaloteaux]

Im using rev c9bc7ba as im facing #82 too

but the issue triggers just with garble -literals main.go and garble -literals --buildmode=plugin plugin/main.go i think.

It fails at this place in the runtime : https://github.com/golang/go/blob/master/src/runtime/plugin.go#L52
because the function/member.. names are used in the hash and both have different random name

changing the hashWith function like this allows me to load the plugin :

 func hashWith(salt, value string) string {
        const length = 8

+
+      //realsalt := salt
+      realsalt := "TESTSTATIC"
        d := sha256.New()
-       io.WriteString(d, salt)
+       io.WriteString(d, realsalt)

A real fix would require a new flag/parameter i think to provide a custom salt so i dont know if it will be included just for this specific case. Also not related but im personally dropping the use of plugins in my project as it leads to more issue then benefit so far.

A quick fix could be also to not use the salt if the seed as been provided

[mvdan]

#82 is fixed now, so please try master again.

but the issue triggers just with garble -literals main.go and garble -literals --buildmode=plugin plugin/main.go i think.

Sorry, but that isn't enough for me to reproduce the issue. I tried to modify our existing plugin test as follows, but couldn't get the build or execution to fail:

diff --git a/testdata/scripts/plugin.txt b/testdata/scripts/plugin.txt
index 9271f48..631a6d2 100644
--- a/testdata/scripts/plugin.txt
+++ b/testdata/scripts/plugin.txt
@@ -1,14 +1,12 @@
 [windows] skip 'Go plugins are not supported on Windows'
 
-garble build -buildmode=plugin ./plugin
+garble -literals build -buildmode=plugin ./plugin
 binsubstr plugin.so 'PublicVar' 'PublicFunc'
 ! binsubstr plugin.so 'privateFunc'
 
-# Note that we need -trimpath; see the caveat section in the README.
-go build -trimpath
+garble -literals build
 exec ./main
 cmp stderr main.stderr
-binsubstr main$exe 'PublicVar' 'PublicFunc'
 ! binsubstr plugin.so 'privateFunc'
 
 [short] stop # no need to verify this with -short

Perhaps provide a small self-contained git repository where we can run a tiny script to reproduce the error, assuming that your source code isn't open source.

[mvdan]

Err, if I run the test as of master with an empty build cache (thus forcing everything to build from scratch), I do get an error:

$ rm -rf /tmp/bad; GOCACHE=/tmp/bad go test -v -run Script/plugin
=== CONT  TestScripts/plugin
[...]
        > go build -trimpath
        > exec ./main
        [stderr]
        panic: plugin.Open("plugin"): plugin was built with a different version of package runtime/internal/sys

Why did I not see this error before? I'm confused.

[mvdan]

We've simply dropped support for Go plugins for the time being; it seems like they require quite a bit more work, and they're kind of low priority for our use cases right now.

[alexmaloteaux]

When the main binary is loading the plugin there is a check between all imports main binary hash and plugin hash.

if one of the hash is different then you get that error : plugin was built with a different version of package

In garble case we get the error because hashwith function generate different name on each build.

One easy fix would be to disable io.WriteString(d, salt) within hashwith when a seed is provided.

If you agree i can provide a PR

[mvdan]

In garble case we get the error because hashwith function generate different name on each build.

I don't get this. Each garble build should be entirely reproducible, so a specific package should always be built exactly the same and with the same salt. Unless you specify different -seed values each time, or -seed=random.

One easy fix would be to disable io.WriteString(d, salt) within hashwith when a seed is provided.

This would be a bad idea in general, because hashing without a salt would be pretty easy to reverse engineer.

[alexmaloteaux]

I don't get this. Each garble build should be entirely reproducible, so a specific package should always be built exactly the same and with the same salt. Unless you specify different -seed values each time, or -seed=random.

Maybe something in readBuildIDs end up outputing different salt when compiling a standard binary or a plugin.

One easy fix would be to disable io.WriteString(d, salt) within hashwith when a seed is provided.

This would be a bad idea in general, because hashing without a salt would be pretty easy to reverse engineer.

One could always randomize the seed value between build in the make file or whatever but keep it the same between plugin and loading binary during same build.

[mvdan]

Maybe something in readBuildIDs end up outputing different salt when compiling a standard binary or a plugin.

Could be. I personally didn't spend all that much time trying to get plugins to work properly.

One could always randomize the seed value between build in the make file or whatever but keep it the same between plugin and loading binary during same build.

You could do that, but the hashing would be significantly weaker still. Right now we have a seed for an entire build (from the -seed flag, empty by default), and a salt that's different for every package (thanks to the unique build ID used for caching). Thanks to both of those being in the hash, a name "foo" will be hashed differently in two different packages, and will also change if the package source code changes.

Removing either of those loses us the respective property. If we remove seed, there's no way to insert a custom seed. If we remove salt, all packages will hash the same, so it's far easier to reverse-engineer the process for all packages at once.

[alexmaloteaux]

hi plugins are working actually almost out of the box,
i tested again today and all i had to do is to

1. add _ "runtime/cgo" to the import
2. add //export funcname before func declaration due to functions now being hashed in main package

[mvdan]

Obfuscation of the standard library has vastly improved in the past two years, so it's not surprising that we are getting closer.

add _ "runtime/cgo" to the import

I'm surprised this is the case; what fails without the underscore import? plugin does import runtime/cgo, but I don't think that should matter.

add //export funcname before func declaration due to functions now being hashed in main package

That one shouldn't be too hard to fix, if we add 474a919 back when the build mode is "plugin".

[alexmaloteaux]

I'm surprised this is the case; what fails without the underscore import? plugin does import runtime/cgo, but I don't think that should matter.

i had a small test case for a while with that line added and a comment it was needed, sorry i just tested yesterday quickly as im not using plugin since some times. i tested without and it works out of the box , so only the //export part is needed at this point.

Didnt check exported variable however