From ae37e84606586613d7115eff6a871a898a0da0bb Mon Sep 17 00:00:00 2001 From: ltzmaxwell Date: Tue, 9 Apr 2024 19:58:51 +0800 Subject: [PATCH] fix: incorrect pointer value comparison (#1601) This is a fix to #1569 . Thanks to @thehowl for identifying this issue and providing a thorough and insightful analysis. Here is an further analysis based on #1569 : ```go package main func main() { { b := []byte("ABCDEFGHIJKL") a := b println(&a[0] == &a[0]) } } ``` this comparison would be false too. The root cause for this is the way pointer values is obtained and compared, e.g. ```go package main func main() { c := []byte{'A'} a := c println(&c[0]) println(&c[0] == &a[0]) } ``` in this code snippet, the address of the c[0], (&c[0]) is obtained from this: ```go ev := fillValueTV(store, &av.List[ii]) // by reference ``` that is a reference to the element of the underlying list of arrayValue, in case like this: ```go package main func main() { { b := []byte("ABCDEFGHIJKL") a := b println(&a[0] == &a[0]) } } ``` the address is obtained by ```go bv := &TypedValue{ // heap alloc, so need to compare value rather than pointer in isEql(), line 482 T: DataByteType, V: DataByteValue{ Base: av, Index: ii, ElemType: et, }, } ``` that is a new allocated *TV, which implies the value of it would not be same within the first and second & operation. So we should actually compare the concrete value rather than the pointers for this case. --- gnovm/pkg/gnolang/op_binary.go | 8 +++++++- gnovm/pkg/gnolang/values.go | 3 ++- gnovm/pkg/gnolang/values_string.go | 4 +++- gnovm/tests/files/a47.gno | 5 ++++- gnovm/tests/files/a47a.gno | 24 +++++++++++++++++++++++ gnovm/tests/files/a47b.gno | 13 +++++++++++++ gnovm/tests/files/a48.gno | 31 ++++++++++++++++++++++++++++++ gnovm/tests/files/a48a.gno | 12 ++++++++++++ gnovm/tests/files/a49.gno | 12 ++++++++++++ 9 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 gnovm/tests/files/a47a.gno create mode 100644 gnovm/tests/files/a47b.gno create mode 100644 gnovm/tests/files/a48.gno create mode 100644 gnovm/tests/files/a48a.gno create mode 100644 gnovm/tests/files/a49.gno diff --git a/gnovm/pkg/gnolang/op_binary.go b/gnovm/pkg/gnolang/op_binary.go index 9162a710d7d..99b56c18a06 100644 --- a/gnovm/pkg/gnolang/op_binary.go +++ b/gnovm/pkg/gnolang/op_binary.go @@ -475,7 +475,13 @@ func isEql(store Store, lv, rv *TypedValue) bool { rfv.GetClosure(store) } case PointerKind: - // TODO: assumes runtime instance normalization. + if lv.V != nil && rv.V != nil { + lpv := lv.V.(PointerValue) + rpv := rv.V.(PointerValue) + if lpv.TV.T == DataByteType && rpv.TV.T == DataByteType { + return *(lpv.TV) == *(rpv.TV) && lpv.Base == rpv.Base && lpv.Index == rpv.Index && lpv.Key == rpv.Key + } + } return lv.V == rv.V default: panic(fmt.Sprintf( diff --git a/gnovm/pkg/gnolang/values.go b/gnovm/pkg/gnolang/values.go index 85e6562eca6..ae5ac7fd40b 100644 --- a/gnovm/pkg/gnolang/values.go +++ b/gnovm/pkg/gnolang/values.go @@ -361,7 +361,7 @@ func (av *ArrayValue) GetPointerAtIndexInt2(store Store, ii int, et Type) Pointe Index: ii, } } - bv := &TypedValue{ // heap alloc + bv := &TypedValue{ // heap alloc, so need to compare value rather than pointer T: DataByteType, V: DataByteValue{ Base: av, @@ -369,6 +369,7 @@ func (av *ArrayValue) GetPointerAtIndexInt2(store Store, ii int, et Type) Pointe ElemType: et, }, } + return PointerValue{ TV: bv, Base: av, diff --git a/gnovm/pkg/gnolang/values_string.go b/gnovm/pkg/gnolang/values_string.go index 34187e32879..b334addc1c3 100644 --- a/gnovm/pkg/gnolang/values_string.go +++ b/gnovm/pkg/gnolang/values_string.go @@ -11,7 +11,7 @@ type protectedStringer interface { ProtectedString(*seenValues) string } -// This indicates the maximum ancticipated depth of the stack when printing a Value type. +// This indicates the maximum anticipated depth of the stack when printing a Value type. const defaultSeenValuesSize = 32 type seenValues struct { @@ -322,6 +322,8 @@ func (tv *TypedValue) ProtectedSprint(seen *seenValues, considerDeclaredType boo return fmt.Sprintf("%d", tv.GetUint()) case Uint8Type: return fmt.Sprintf("%d", tv.GetUint8()) + case DataByteType: + return fmt.Sprintf("%d", tv.GetDataByte()) case Uint16Type: return fmt.Sprintf("%d", tv.GetUint16()) case Uint32Type: diff --git a/gnovm/tests/files/a47.gno b/gnovm/tests/files/a47.gno index 2775547a3e8..1749968eac1 100644 --- a/gnovm/tests/files/a47.gno +++ b/gnovm/tests/files/a47.gno @@ -20,7 +20,9 @@ func main() { println(newArr[0].i, newArr[1].i) // The struct should have been copied, not referenced. - println(&sArr[2] != &newArr[1]) + println(&sArr[2] == &newArr[1]) + // share same underlying array, and same index + println(&sArr[1] == &newArr[1]) } // Output: @@ -29,4 +31,5 @@ func main() { // 2 3 3 // true // 2 3 +// false // true diff --git a/gnovm/tests/files/a47a.gno b/gnovm/tests/files/a47a.gno new file mode 100644 index 00000000000..a613482efda --- /dev/null +++ b/gnovm/tests/files/a47a.gno @@ -0,0 +1,24 @@ +package main + +type S struct { + i int +} + +func main() { + sArr := make([]S, 0, 4) + sArr = append(sArr, S{1}, S{2}, S{3}) + + newArr := append(sArr[:0], sArr[0:]...) + + // share same underlying array + println(&sArr[0] == &newArr[0]) + + println(&sArr[1] == &newArr[1]) + + println(&sArr[2] == &newArr[2]) +} + +// Output: +// true +// true +// true diff --git a/gnovm/tests/files/a47b.gno b/gnovm/tests/files/a47b.gno new file mode 100644 index 00000000000..89385a6f562 --- /dev/null +++ b/gnovm/tests/files/a47b.gno @@ -0,0 +1,13 @@ +package main + +func main() { + s1 := []int{1, 2} + i0 := &s1[0] + // new array allocated, so they have different base array + s1 = append(s1, 3) + ii0 := &s1[0] + println(i0 == ii0) +} + +// Output: +// false diff --git a/gnovm/tests/files/a48.gno b/gnovm/tests/files/a48.gno new file mode 100644 index 00000000000..be6e9c064ce --- /dev/null +++ b/gnovm/tests/files/a48.gno @@ -0,0 +1,31 @@ +package main + +func main() { + { + b := []byte("ABCDEFGHIJKL") + a := b + println(&b[0] == &a[0], b[0], a[0]) + + // modifying the underlying array modifies both a[0] and b[0], + // as it should + a[0] = 11 + println(a[0], b[0]) + } + + { + b := []byte{1, 2, 3} + a := b + println(&b[0] == &a[0], b[0], a[0]) + + // modifying the underlying array modifies both a[0] and b[0], + // as it should + a[0] = 11 + println(a[0], b[0]) + } +} + +// Output: +// true 65 65 +// 11 11 +// true 1 1 +// 11 11 diff --git a/gnovm/tests/files/a48a.gno b/gnovm/tests/files/a48a.gno new file mode 100644 index 00000000000..b17fdff515d --- /dev/null +++ b/gnovm/tests/files/a48a.gno @@ -0,0 +1,12 @@ +package main + +func main() { + { + b := []byte("ABCDEFGHIJKL") + a := b + println(&a[0] == &a[0]) + } +} + +// Output: +// true diff --git a/gnovm/tests/files/a49.gno b/gnovm/tests/files/a49.gno new file mode 100644 index 00000000000..ee39a484eb7 --- /dev/null +++ b/gnovm/tests/files/a49.gno @@ -0,0 +1,12 @@ +package main + +func main() { + c := []byte{'A'} + a := c + println(&c[0]) + println(&c[0] == &a[0]) +} + +// Output: +// &(65 uint8) +// true