Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plugin: Panic decoding json into shared type #21386

Closed
alhails opened this issue Aug 10, 2017 · 7 comments
Closed

plugin: Panic decoding json into shared type #21386

alhails opened this issue Aug 10, 2017 · 7 comments

Comments

@alhails
Copy link

alhails commented Aug 10, 2017

go version go1.9rc2 linux/amd64

GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/alastair/Code/go"
GORACE=""
GOROOT="/home/alastair/Code/golang/go"
GOTOOLDIR="/home/alastair/Code/golang/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build462207145=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

When multiple plugins contain the same type, this leads to errors decoding JSON into that type

Here's my simple example reproducing this behaviour.
Build the plugins using -buildmode=plugin.
I can send the full buildable project if required.

main.go

package main

import (
	"fmt"
	"plugin"
)

func main() {
	plugins := []string{"plugin1.so", "plugin2.so"}
	for _, pluginName := range plugins {
		plugin, err := plugin.Open(pluginName)
		if err != nil {
			fmt.Printf("Failed to load plugin (%s)\n", err.Error())
		}
		symbol, err := plugin.Lookup("Decode")
		if err != nil {
			fmt.Printf("Plugin not implemented correctly (%s)\n", err.Error())
		}
		f := symbol.(func(string))
		body := `{"success":true,"data":{"id":1,"name":"A"}}`
		f(body)
	}
}

plugin1.go

package main

import (
	"encoding/json"
	"fmt"
)

type orgUnitName struct {
	ID   int    `json:"id"`
	Name string `json:"name"`
}

type orgUnitNameResponse struct {
	Data *orgUnitName `json:"data"`
}

func Decode(body string) {
	orgUnit := orgUnitNameResponse{}
	err := json.Unmarshal([]byte(body), &orgUnit)
	if err != nil {
		fmt.Printf("Failed to decode (%s)", err.Error())
		return
	}
	fmt.Println("Plugin1", *orgUnit.Data)
}

plugin2.go

package main

import (
	"encoding/json"
	"fmt"
)

type orgUnitName struct {
	ID   int    `json:"id"`
	Name string `json:"name"`
}

type orgUnitNameResponse struct {
	Data *orgUnitName `json:"data"`
}

func Decode(body string) {
	orgUnit := orgUnitNameResponse{}
	err := json.Unmarshal([]byte(body), &orgUnit)
	if err != nil {
		fmt.Printf("Failed to decode (%s)", err.Error())
		return
	}
	fmt.Println("Plugin2", *orgUnit.Data)
}

What did you expect to see?

Expected the decoding to work successfully with no error

What did you see instead?

The standard library reports the following error...

panic: interface conversion: string is not error: missing method Error

goroutine 1 [running]:
encoding/json.(*decodeState).unmarshal.func1(0xc420059db0)
        /home/alastair/Code/golang/go/src/encoding/json/decode.go:176 +0x105
panic(0x548d60, 0xc4200920f0)
        /home/alastair/Code/golang/go/src/runtime/panic.go:491 +0x294
reflect.Value.assignTo(0x7fd353574a80, 0xc42008e260, 0x16, 0x7fd352e57a77, 0xb, 0x7fd353093a80, 0x0, 0x7fd353574a80, 0x16, 0xc42008e260)
        /home/alastair/Code/golang/go/src/reflect/value.go:2186 +0x3ac
reflect.Value.Set(0x7fd353093a80, 0xc420108e68, 0x196, 0x7fd353574a80, 0xc42008e260, 0x16)
        /home/alastair/Code/golang/go/src/reflect/value.go:1351 +0xa6
encoding/json.(*decodeState).indirect(0xc4200dd7a0, 0x7fd353093a80, 0xc420108e68, 0x196, 0x0, 0xc4200ea090, 0x2b, 0x2b, 0xc420059858, 0x7fd3532ac3ac, ...)
        /home/alastair/Code/golang/go/src/encoding/json/decode.go:469 +0x386
encoding/json.(*decodeState).object(0xc4200dd7a0, 0x7fd353093a80, 0xc420108e68, 0x196)
        /home/alastair/Code/golang/go/src/encoding/json/decode.go:599 +0x71
encoding/json.(*decodeState).value(0xc4200dd7a0, 0x7fd353093a80, 0xc420108e68, 0x196)
        /home/alastair/Code/golang/go/src/encoding/json/decode.go:406 +0x2f1
encoding/json.(*decodeState).object(0xc4200dd7a0, 0x7fd353093ac0, 0xc420108e68, 0x16)
        /home/alastair/Code/golang/go/src/encoding/json/decode.go:737 +0x12bf
encoding/json.(*decodeState).value(0xc4200dd7a0, 0x7fd353093ac0, 0xc420108e68, 0x16)
        /home/alastair/Code/golang/go/src/encoding/json/decode.go:406 +0x2f1
encoding/json.(*decodeState).unmarshal(0xc4200dd7a0, 0x7fd353093ac0, 0xc420108e68, 0x0, 0x0)
        /home/alastair/Code/golang/go/src/encoding/json/decode.go:188 +0x21c
encoding/json.Unmarshal(0xc4200ea060, 0x2b, 0x30, 0x7fd353093ac0, 0xc420108e68, 0x30, 0x0)
        /home/alastair/Code/golang/go/src/encoding/json/decode.go:107 +0x15a
plugin_decode/plugin2.Decode(0x574211, 0x2b)
        /home/alastair/Code/go/src/plugin_decode/plugin2/plugin2.go:19 +0xaa
main.main()
        /home/alastair/Code/go/src/plugin_decode/main.go:21 +0x79

This error actually results from a failure to recover from the original panic - so the original error details are hidden.
This happens here since the panic is raised with a string rather than an Error...

err = r.(error)

After hacking some changes here, I discovered the original error as ...

reflect.Set: value of type *main.orgUnitName is not assignable to type *main.orgUnitName

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Aug 12, 2017
@ianlancetaylor
Copy link
Contributor

CC @crawshaw

@alhails alhails changed the title plugin: Exception decoding json into shared type plugin: Error decoding json into shared type Aug 18, 2017
@alhails alhails changed the title plugin: Error decoding json into shared type plugin: Panic decoding json into shared type Aug 18, 2017
@crawshaw
Copy link
Member

crawshaw commented Sep 1, 2017

Great bug report. Thanks.

Looks like the plugin name prefix is not being used for namedata symbols, leading to a collision between the two independent types:

$ objdump -t plugin1.so | grep orgUnitName
00000000000c7610 l       __TEXT,__text	type..hash.plugin1.orgUnitName
00000000000c7690 l       __TEXT,__text	type..eq.plugin1.orgUnitName
00000000000ca1e0 l       __DATA,__rodata	type..eqfunc.plugin1.orgUnitName
00000000000ca590 l       __DATA,__rodata	type..hashfunc.plugin1.orgUnitName
00000000000ce080 l       __DATA,__rodata	type..alg.plugin1.orgUnitName
00000000000d126c l       __DATA,__rodata	type..namedata.*main.orgUnitName-        <-- oops
00000000000d356a l       __DATA,__rodata	type..namedata.*main.orgUnitNameResponse-
00000000000d6ea0 g       __DATA,__rodata	_type.*plugin1.orgUnitName
00000000000d6ee0 g       __DATA,__rodata	_type.*plugin1.orgUnitNameResponse
00000000000ec400 g       __DATA,__rodata	_type.plugin1.orgUnitName
00000000000ea000 g       __DATA,__rodata	_type.plugin1.orgUnitNameResponse

Should be an easy fix, let me give it a go.

@crawshaw
Copy link
Member

crawshaw commented Sep 1, 2017

I think the namedata symbol was a red herring, something else is causing the reflect package to get the wrong *rtype. A simplified test case:

plugin1.go

type sameNameReusedInPlugins struct {
        X string
}

type sameNameHolder struct {
        F *sameNameReusedInPlugins
}

var SameName sameNameHolder

plugin2.go

type sameNameReusedInPlugins struct {
        X string
}

type sameNameHolder struct {
        F *sameNameReusedInPlugins
}

func UnexportedNameReuse() {
        h := sameNameHolder{}
        v := reflect.ValueOf(&h).Elem().Field(0)
        println("UnexportedNameReuse field type: ", v.Type())
        v.Set(reflect.New(v.Type().Elem()))
}

Load plugin1, then plugin2, and call UnexportedNameReuse().

@crawshaw
Copy link
Member

crawshaw commented Sep 1, 2017

The problem here is the pkgpath value inside the type.plugin1.sameNameReusedInPlugins symbol. In both the plugin1 and plugin2 types it is "main", which means the runtime typeOff logic declares them identical types and starts always picking the type from the earlier module.

This is easy to fix. The pkgpath is set to "main" by the compiler, because cmd/go is passing "-p main" when compiling the package. If it left it off, or passed the correct value when building the plugin, the two types then have useful pkgpath values and the error is fixed.

@gopherbot
Copy link

Change https://golang.org/cl/60910 mentions this issue: cmd/go: pass plugin package name to compile -p

@alhails
Copy link
Author

alhails commented Sep 1, 2017

@crawshaw Thanks for the fix.
That should fix the underlying issue, but are you also considering changing the logic in

err = r.(error)

I'm happy to submit a pull request for this if you think it would be a good change - i.e. changing the logic to check the type of recovered panic and if it's a string (rather than error) then create new error.

@crawshaw
Copy link
Member

crawshaw commented Sep 1, 2017

@alhails oddly, I saw a different error message to you:

$ go run main.go 
panic: reflect.Set: value of type *main.orgUnitName is not assignable to type *main.orgUnitName [recovered]
	panic: interface conversion: string is not error: missing method Error

goroutine 1 [running]:
encoding/json.(*decodeState).unmarshal.func1(0xc420053d80)
	/Users/crawshaw/go/src/encoding/json/decode.go:175 +0x8f
panic(0x40c6b80, 0xc4200102e0)
	/Users/crawshaw/go/src/runtime/panic.go:492 +0x273
reflect.Value.assignTo(0x50d6ea0, 0xc42000c1e0, 0x16, 0x534a764, 0xb, 0x531cea0, 0x0, 0x50d6ea0, 0x16, 0xc42000c1e0)
	/Users/crawshaw/go/src/reflect/value.go:2185 +0x3a0
reflect.Value.Set(0x531cea0, 0xc4200e8d88, 0x196, 0x50d6ea0, 0xc42000c1e0, 0x16)
	/Users/crawshaw/go/src/reflect/value.go:1350 +0xa4
encoding/json.(*decodeState).indirect(0xc4200cfb00, 0x531cea0, 0xc4200e8d88, 0x196, 0x0, 0xc42001c0c0, 0x2b, 0x2b, 0xc420053828, 0x5284b93, ...)
	/Users/crawshaw/go/src/encoding/json/decode.go:468 +0x340
encoding/json.(*decodeState).object(0xc4200cfb00, 0x531cea0, 0xc4200e8d88, 0x196)
	/Users/crawshaw/go/src/encoding/json/decode.go:598 +0x6f
encoding/json.(*decodeState).value(0xc4200cfb00, 0x531cea0, 0xc4200e8d88, 0x196)
	/Users/crawshaw/go/src/encoding/json/decode.go:405 +0x2e0
encoding/json.(*decodeState).object(0xc4200cfb00, 0x531cee0, 0xc4200e8d88, 0x16)
	/Users/crawshaw/go/src/encoding/json/decode.go:736 +0x120f
encoding/json.(*decodeState).value(0xc4200cfb00, 0x531cee0, 0xc4200e8d88, 0x16)
	/Users/crawshaw/go/src/encoding/json/decode.go:405 +0x2e0
encoding/json.(*decodeState).unmarshal(0xc4200cfb00, 0x531cee0, 0xc4200e8d88, 0x0, 0x0)
	/Users/crawshaw/go/src/encoding/json/decode.go:187 +0x20d
encoding/json.Unmarshal(0xc42001c090, 0x2b, 0x30, 0x531cee0, 0xc4200e8d88, 0x30, 0x40a95b2)
	/Users/crawshaw/go/src/encoding/json/decode.go:107 +0x158
plugin2.Decode(0x40f1829, 0x2b)
	/Users/crawshaw/src/plugin2/plugin2.go:19 +0x9e
main.main()
	/Users/crawshaw/main.go:23 +0x210
exit status 2
$

This has all the information I need in it, so I'm happy the way it is. I don't know why you see something different. Is this a recent change? Mind syncing and seeing if your error is the same?

@golang golang locked and limited conversation to collaborators Sep 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants