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

Instruction to reproduce weird bug on append #1170

Closed
wants to merge 1 commit into from

Conversation

tbruyelle
Copy link
Contributor

@tbruyelle tbruyelle commented Sep 25, 2023

start a node, then execute append_bug.sh <YOURKEY> or copy paste the commands one by one.

The realm that has been added to reproduce exposes 2 functions,Append and Pop, and the script does the following:

Append 1
Append 2
Append 3
Render -> 1,2,3 -> OK
Pop
Render -> 1,2 -> WRONG, should be 2,3 !!
Append 42
Render -> 1,2,3 -> WTF Where is 42 ???

@tbruyelle tbruyelle requested a review from a team as a code owner September 25, 2023 16:40
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Sep 25, 2023
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (5a7d005) 47.00% compared to head (3f1a8a1) 47.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1170   +/-   ##
=======================================
  Coverage   47.00%   47.00%           
=======================================
  Files         365      365           
  Lines       61156    61156           
=======================================
+ Hits        28744    28746    +2     
+ Misses      30058    30056    -2     
  Partials     2354     2354           
Flag Coverage Δ
gno.land-_test.gnokey 0.00% <ø> (ø)
gno.land-_test.gnoland 88.14% <ø> (ø)
gno.land-_test.pkgs 27.88% <ø> (ø)
gnovm-_test.cmd 45.89% <ø> (ø)
gnovm-_test.gnolang.pkg0 17.98% <ø> (ø)
gnovm-_test.gnolang.pkg1 8.21% <ø> (ø)
gnovm-_test.gnolang.pkg2 9.87% <ø> (ø)
gnovm-_test.gnolang.realm 41.68% <ø> (ø)
gnovm-_test.gnolang.stdlibs 53.53% <ø> (ø)
gnovm-_test.pkg 25.94% <ø> (ø)
tm2-_test.flappy ∅ <ø> (∅)
tm2-_test.pkg.amino 58.32% <ø> (ø)
tm2-_test.pkg.bft 63.59% <ø> (+0.01%) ⬆️
tm2-_test.pkg.others 59.21% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moul
Copy link
Member

moul commented Sep 25, 2023

Another candidate for #1117

Edit: here is an initial txtar: ef50172, in #1171.

@tbruyelle
Copy link
Contributor Author

Closing in favor of #1305

@tbruyelle tbruyelle closed this Oct 27, 2023
tbruyelle added a commit to tbruyelle/gno that referenced this pull request Nov 1, 2023
So I think we can state that the bugs we have in gnolang#960, gnolang#1167 and gnolang#1170,
are all related to slice storage when its capacity is different than its
length.

@deelawn found a great way to overcome that bug, but the bug is still
there, somewhere in the VM code I think. I spent the last couple of days
trying to find it, unfortunately without success.

That said, I found a workaround, that could be also applied: when a
slice is stored, ignore any capacity different than the slice's length.

I think this is a good workaround because its one-line and because we
don't really care about storing slice with capacity higher than their
length (unless I'm missing something).
jaekwon added a commit that referenced this pull request Jan 4, 2024
Addresses #1167,  #960, and #1170 

Consider the following situation:
- A slice of structs exists with a length of zero and a capacity of one
- A new struct literal is appended to the slice
- The code panics because the newly allocated struct literal was never
marked as "new"

``` go
package append

import (
	"gno.land/p/demo/ufmt"
)

type T struct{ i int }

var a []T

func init() {
        a = make([]T, 0, 1)
}

func Append(i int) {
	a = append(a, T{i: i})
}

```

Invoking the `Append` function will cause a panic.

The solution is to traverse each of the array elements after slice
append assignment to make sure any new or updated elements are marked as
such.

This PR also includes a change to ensure that marking an object as dirty
and managing references to the object are mutually exclusive. I think
this is correct but am not sure.

The changes include txtar test cases that incorporate the issue
described by @tbruyelle in #1170

---------

Co-authored-by: jaekwon <jae@tendermint.com>
gfanton pushed a commit to moul/gno that referenced this pull request Jan 18, 2024
…lang#1305)

Addresses gnolang#1167,  gnolang#960, and gnolang#1170 

Consider the following situation:
- A slice of structs exists with a length of zero and a capacity of one
- A new struct literal is appended to the slice
- The code panics because the newly allocated struct literal was never
marked as "new"

``` go
package append

import (
	"gno.land/p/demo/ufmt"
)

type T struct{ i int }

var a []T

func init() {
        a = make([]T, 0, 1)
}

func Append(i int) {
	a = append(a, T{i: i})
}

```

Invoking the `Append` function will cause a panic.

The solution is to traverse each of the array elements after slice
append assignment to make sure any new or updated elements are marked as
such.

This PR also includes a change to ensure that marking an object as dirty
and managing references to the object are mutually exclusive. I think
this is correct but am not sure.

The changes include txtar test cases that incorporate the issue
described by @tbruyelle in gnolang#1170

---------

Co-authored-by: jaekwon <jae@tendermint.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: 🚀 Needed for Launch
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants