Skip to content

Commit

Permalink
fix: panic in store.SprintStoreOps (#1036)
Browse files Browse the repository at this point in the history
2 ways to reproduce :

- Using a filetest:
```
 go run ./gnovm/cmd/gno test -verbose ./examples/gno.land/p/demo/tests/ -run file/z0
?       ./examples/gno.land/p/demo/tests/subtests 	[no test files]
=== RUN   file/z0_filetest.gno
panic: expected JSON object but got "20"

goroutine 1 [running]:
github.com/gnolang/gno/tm2/pkg/amino.(*Codec).MustMarshalJSON(...)
	/home/tom/src/gno/tm2/pkg/amino/amino.go:818
github.com/gnolang/gno/tm2/pkg/amino.MustMarshalJSON({0xda1d40?, 0xc0001305a0?})
	/home/tom/src/gno/tm2/pkg/amino/amino.go:140 +0x53
github.com/gnolang/gno/gnovm/pkg/gnolang.StoreOp.String({0x0, {0x1026bf0, 0xc0001305a0}, {0x0, 0x0}})
	/home/tom/src/gno/gnovm/pkg/gnolang/store.go:654 +0xb2
github.com/gnolang/gno/gnovm/pkg/gnolang.(*defaultStore).SprintStoreOps(0xc0000166c0)
	/home/tom/src/gno/gnovm/pkg/gnolang/store.go:687 +0xff
github.com/gnolang/gno/gnovm/tests.RunFileTest({0xc000357158, 0x11}, {0xc0003691a0, 0x2e}, {0xc0001499e8, 0x1, 0x1013280?})
	/home/tom/src/gno/gnovm/tests/file.go:325 +0x44e
main.gnoTestPkg({0xc000116500, 0x20}, {0x0?, 0x0, 0x1?}, {0xc00035b720, 0x1, 0xc000129af8?}, 0xc00034c8c0, 0xc0002948c0)
	/home/tom/src/gno/gnovm/cmd/gno/test.go:337 +0x876
main.execTest(0xc00034c8c0, {0xc00035b590, 0x1, 0x1}, 0xc000036220?)
	/home/tom/src/gno/gnovm/cmd/gno/test.go:208 +0xb67
main.newTestCmd.func1({0x0?, 0x0?}, {0xc00035b590?, 0xc00035f118?, 0x0?})
	/home/tom/src/gno/gnovm/cmd/gno/test.go:51 +0x32
github.com/gnolang/gno/tm2/pkg/commands.(*Command).Run(0x5?, {0x1018768?, 0xc00003a130?})
	/home/tom/src/gno/tm2/pkg/commands/command.go:233 +0x1ac
github.com/gnolang/gno/tm2/pkg/commands.(*Command).Run(0xc00035ee70?, {0x1018768?, 0xc00003a130?})
	/home/tom/src/gno/tm2/pkg/commands/command.go:237 +0x154
github.com/gnolang/gno/tm2/pkg/commands.(*Command).ParseAndRun(0x4059dc?, {0x1018768, 0xc00003a130}, {0xc0000361f0?, 0x401240?, 0x0?})
	/home/tom/src/gno/tm2/pkg/commands/command.go:118 +0x4f
main.main()
	/home/tom/src/gno/gnovm/cmd/gno/main.go:14 +0x75
exit status 2
```

- using a go unit test:
```
$  go test ./gnovm/pkg/gnolang/ -v -run Amino
=== RUN   TestAminoMustMarshalJSONPanics
"20"
--- FAIL: TestAminoMustMarshalJSONPanics (0.00s)
panic: expected JSON object but got "20" [recovered]
	panic: expected JSON object but got "20"

goroutine 6 [running]:
testing.tRunner.func1.2({0xc15460, 0xc0002d2fc0})
	/usr/lib/go/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
	/usr/lib/go/src/testing/testing.go:1529 +0x39f
panic({0xc15460, 0xc0002d2fc0})
	/usr/lib/go/src/runtime/panic.go:884 +0x213
github.com/gnolang/gno/tm2/pkg/amino.(*Codec).MustMarshalJSON(...)
	/home/tom/src/gno/tm2/pkg/amino/amino.go:818
github.com/gnolang/gno/tm2/pkg/amino.MustMarshalJSON({0xc59ec0?, 0xc0001c42d0?})
	/home/tom/src/gno/tm2/pkg/amino/amino.go:140 +0x53
github.com/gnolang/gno/gnovm/pkg/gnolang.TestAminoMustMarshalJSONPanics(0x0?)
	/home/tom/src/gno/gnovm/pkg/gnolang/values_test.go:20 +0x134
testing.tRunner(0xc000300000, 0xcd8f40)
	/usr/lib/go/src/testing/testing.go:1576 +0x10b
created by testing.(*T).Run
	/usr/lib/go/src/testing/testing.go:1629 +0x3ea
FAIL	github.com/gnolang/gno/gnovm/pkg/gnolang	0.008s
FAIL
```

Additional notes:
- when `// Realm:` instruction is present, it triggers a comparison
between the store and the content of the instruction.
- when the store content is serialized before the comparison, this panic
happens in the amino code
- this doesn't affect all `// Realm:` instruction, for instance
`examples/gno.land/p/demo/avl/z_0_filetest.gno` is still working well

<!-- please provide a detailed description of the changes made in this
pull request. -->

<details><summary>Contributors' checklist...</summary>

- [ ] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
  • Loading branch information
tbruyelle committed Sep 7, 2023
1 parent 4bd1789 commit 207d66d
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 20 deletions.
40 changes: 40 additions & 0 deletions gnovm/pkg/gnolang/package_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package gnolang

import (
"math/big"
"reflect"
"testing"

"github.com/jaekwon/testify/require"

"github.com/gnolang/gno/tm2/pkg/amino"
)

// Tries to reproduce the bug #1036 on all registered types
func TestAminoJSONRegisteredTypes(t *testing.T) {
for _, typ := range Package.Types {
// Instantiate registered type
x := reflect.New(typ.Type).Interface()

// Call MarshalAmino directly on 'x'
_, err := amino.MarshalJSON(x)
require.NoError(t, err, "marshal type %s", typ.Type.Name())

// Call MarshalAmino on a struct that embeds 'x' in a field of type any
xx := struct {
X any
}{X: x}
_, err = amino.MarshalJSON(xx)
require.NoError(t, err, "marshal type %s from struct", typ.Type.Name())
}

// Check unmarshaling (can't reuse package.Types because some internal values
// must be filled properly
bi := BigintValue{V: big.NewInt(1)}
bz := amino.MustMarshalJSON(bi)
amino.MustUnmarshalJSON(bz, &bi)
// Check unmarshaling with an embedding struct
x := struct{ X any }{X: bi}
bz = amino.MustMarshalJSON(x)
amino.MustUnmarshalJSON(bz, &x)
}
2 changes: 1 addition & 1 deletion tm2/pkg/amino/json_decode.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func (cdc *Codec) decodeReflectJSONInterface(bz []byte, iinfo *TypeInfo, rv refl
}

// Extract the value bytes.
if cinfo.IsJSONAnyValueType {
if cinfo.IsJSONAnyValueType || (cinfo.IsAminoMarshaler && cinfo.ReprType.IsJSONAnyValueType) {
bz = value
} else {
bz, err = deriveJSONObject(bz, typeURL)
Expand Down
36 changes: 17 additions & 19 deletions tm2/pkg/amino/json_encode.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ func (cdc *Codec) encodeReflectJSONInterface(w io.Writer, iinfo *TypeInfo, rv re
err = errors.New("JSON bytes cannot be empty")
return
}
if cinfo.IsJSONAnyValueType {
if cinfo.IsJSONAnyValueType || (cinfo.IsAminoMarshaler && cinfo.ReprType.IsJSONAnyValueType) {
// Sanity check
if value[0] == '{' || value[len(value)-1] == '}' {
err = errors.New("unexpected JSON object %s", value)
Expand Down Expand Up @@ -397,24 +397,22 @@ func isJSONAnyValueType(rt reflect.Type) bool {
// {@type,value}, the latter specifically
// {@type:"/google.protobuf.Any",value:{@type,value}).
return true
} else {
// Otherwise, it depends on the kind.
switch rt.Kind() {
case
reflect.Bool, reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32,
reflect.Int64, reflect.Uint, reflect.Uint8, reflect.Uint16,
reflect.Uint32, reflect.Uint64, reflect.Float32, reflect.Float64,
// Primitive types get special {@type,value} treatment. In
// binary form, most of these types would be encoded
// wrapped in an implicit struct, except for lists (both of
// bytes and of anything else), and for strings...
reflect.Array, reflect.Slice, reflect.String:
// ...which are all non-objects that must be encoded as
// {@type,value}.
return true
default:
return false
}
}
// Otherwise, it depends on the kind.
switch rt.Kind() {
case
reflect.Bool, reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32,
reflect.Int64, reflect.Uint, reflect.Uint8, reflect.Uint16,
reflect.Uint32, reflect.Uint64, reflect.Float32, reflect.Float64,
// Primitive types get special {@type,value} treatment. In
// binary form, most of these types would be encoded
// wrapped in an implicit struct, except for lists (both of
// bytes and of anything else), and for strings...
reflect.Array, reflect.Slice, reflect.String:
// ...which are all non-objects that must be encoded as
// {@type,value}.
return true
default:
return false
}
}

0 comments on commit 207d66d

Please sign in to comment.