Skip to content

Commit

Permalink
fix: incorrect pointer value comparison (#1601)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ltzmaxwell authored Apr 9, 2024
1 parent 3b95486 commit ae37e84
Show file tree
Hide file tree
Showing 9 changed files with 108 additions and 4 deletions.
8 changes: 7 additions & 1 deletion gnovm/pkg/gnolang/op_binary.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
3 changes: 2 additions & 1 deletion gnovm/pkg/gnolang/values.go
Original file line number Diff line number Diff line change
Expand Up @@ -361,14 +361,15 @@ 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,
Index: ii,
ElemType: et,
},
}

return PointerValue{
TV: bv,
Base: av,
Expand Down
4 changes: 3 additions & 1 deletion gnovm/pkg/gnolang/values_string.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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:
Expand Down
5 changes: 4 additions & 1 deletion gnovm/tests/files/a47.gno
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -29,4 +31,5 @@ func main() {
// 2 3 3
// true
// 2 3
// false
// true
24 changes: 24 additions & 0 deletions gnovm/tests/files/a47a.gno
Original file line number Diff line number Diff line change
@@ -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
13 changes: 13 additions & 0 deletions gnovm/tests/files/a47b.gno
Original file line number Diff line number Diff line change
@@ -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
31 changes: 31 additions & 0 deletions gnovm/tests/files/a48.gno
Original file line number Diff line number Diff line change
@@ -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
12 changes: 12 additions & 0 deletions gnovm/tests/files/a48a.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package main

func main() {
{
b := []byte("ABCDEFGHIJKL")
a := b
println(&a[0] == &a[0])
}
}

// Output:
// true
12 changes: 12 additions & 0 deletions gnovm/tests/files/a49.gno
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit ae37e84

Please sign in to comment.