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

type assertion on recovered nils panics #1650

Closed
thehowl opened this issue Feb 9, 2024 · 2 comments · Fixed by #1689
Closed

type assertion on recovered nils panics #1650

thehowl opened this issue Feb 9, 2024 · 2 comments · Fixed by #1689
Assignees
Labels
🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related

Comments

@thehowl
Copy link
Member

thehowl commented Feb 9, 2024

package main

type ex int

func (ex) Error() string { return "" }

func main() {
	defer func() {
		e := recover()
		if _, ok := e.(ex); ok {
			println("wat")
		} else {
			println("ok")
		}
	}()
}
Output and stacktrace

panic running expression main(): runtime error: invalid memory address or nil pointer dereference
Machine:
    CheckTypes: false
	Op: [OpHalt OpReturnFromBlock OpReturnCallDefers OpBody OpPopBlock OpIfCond OpEval OpDefine]
	Values: (len: 3)
          #2 (typeval{main.ex (0xc000119440)} type{})
          #1 (undefined)
          #0 (main func()())
	Exprs:
          #0 ok<VPBlock(1,0)>
	Stmts:
          #1 _<VPBlock(0,0)>, ok<VPBlock(1,0)> := e<VPBlock(2,0)>.(ex<VPBlock(5,0)>)
          #0 bodyStmt[0/0/2]=(end)
	Blocks:
          @(1) Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc00035e960,Source:if _<VPBlock(0,0)>, ok<VPBlock(1...,Parent:0xc00035e5a0)
            ok: (undefined)
 (s vals) @(1) Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc000266d20,Source:if _<VPBlock(0,0)>, ok<VPBlock(1...,Parent:0xc000344020)
            ok: (false bool)
 (s typs) @(1) [bool]
          @(2) Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc00035e5a0,Source:func func(){ e<VPBlock(1,0)> := ...,Parent:0xc00035e3c0)
            e: (undefined)
 (s vals) @(2) Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc000344020,Source:func func(){ e<VPBlock(1,0)> := ...,Parent:0xc000335420)
            e: (undefined)
 (s typs) @(2) [interface{}]
          @(3) Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc00035e3c0,Source:func main() { defer func func(){...,Parent:0xc00035e000)
 (s vals) @(3) Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc000335420,Source:func main() { defer func func(){...,Parent:0xc0002cf820)
 (s typs) @(3) []
          @(4) Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc00035e000,Source:file{ package main; type ex (con...,Parent:0xc0002ca5a0)
 (s vals) @(4) Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc0002cf820,Source:file{ package main; type ex (con...,Parent:0xc0002cfb20)
 (s typs) @(4) []
          @(5) main
	Blocks (other):
          #2 Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc00035e5a0,Source:func func(){ e<VPBlock(1,0)> := ...,Parent:0xc00035e3c0)
            e: (undefined)
 (static) #2 Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc000344020,Source:func func(){ e<VPBlock(1,0)> := ...,Parent:0xc000335420)
            e: (undefined)
          #1 Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc00035e3c0,Source:func main() { defer func func(){...,Parent:0xc00035e000)
 (static) #1 Block(ID:0000000000000000000000000000000000000000:0,Addr:0xc000335420,Source:func main() { defer func func(){...,Parent:0xc0002cf820)
	Frames:
          #0 [FRAME FUNC:main RECV:(undefined) (0 args) 1/0/0/0/1 LASTPKG:main LASTRLM:Realm(nil)]
	Realm:
	  
	Exceptions:
	  []
	  
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0xa1ac29]

goroutine 1 [running]:
main.runExpr.func1()
	/home/howl/oc/gno/gnovm/cmd/gno/run.go:159 +0x125
panic({0xbcb520?, 0x135cc90?})
	/usr/lib/go/src/runtime/panic.go:914 +0x21f
github.com/gnolang/gno/gnovm/pkg/gnolang.(*Machine).doOpTypeAssert2(0xc000166fc0)
	/home/howl/oc/gno/gnovm/pkg/gnolang/op_expressions.go:318 +0x5a9
github.com/gnolang/gno/gnovm/pkg/gnolang.(*Machine).Run(0xc000166fc0)
	/home/howl/oc/gno/gnovm/pkg/gnolang/machine.go:1265 +0xd8e
github.com/gnolang/gno/gnovm/pkg/gnolang.(*Machine).Eval(0xc000166fc0, {0xe7fb18, 0xc000119860})
	/home/howl/oc/gno/gnovm/pkg/gnolang/machine.go:701 +0x579
main.runExpr(0xc000166fc0, {0xc9823d?, 0x1?})
	/home/howl/oc/gno/gnovm/cmd/gno/run.go:166 +0x8d
main.execRun(0xc000291770, {0xc0002e9fc0, 0x1, 0x1}, {0xe82030, 0xc0002f6d70})
	/home/howl/oc/gno/gnovm/cmd/gno/run.go:108 +0x3b7
main.newRunCmd.func1({0x0?, 0xc0000341a0?}, {0xc0002e9fc0?, 0x0?, 0x0?})
	/home/howl/oc/gno/gnovm/cmd/gno/run.go:35 +0x30
github.com/gnolang/gno/tm2/pkg/commands.(*Command).Run(0xc0002f4fd0?, {0xe79468?, 0x13b1200?})
	/home/howl/oc/gno/tm2/pkg/commands/command.go:247 +0x19d
github.com/gnolang/gno/tm2/pkg/commands.(*Command).Run(0xc0002f4790?, {0xe79468?, 0x13b1200?})
	/home/howl/oc/gno/tm2/pkg/commands/command.go:251 +0x149
github.com/gnolang/gno/tm2/pkg/commands.(*Command).ParseAndRun(0xc0002f4b00?, {0xe79468, 0x13b1200}, {0xc000034190?, 0xc0002f4bb0?, 0xc0002f4c60?})
	/home/howl/oc/gno/tm2/pkg/commands/command.go:132 +0x49
github.com/gnolang/gno/tm2/pkg/commands.(*Command).Execute(0xe82030?, {0xe79468?, 0x13b1200?}, {0xc000034190?, 0x13276c8?, 0xc0000061a0?})
	/home/howl/oc/gno/tm2/pkg/commands/command.go:114 +0x29
main.main()
	/home/howl/oc/gno/gnovm/cmd/gno/main.go:13 +0x5f

Note: this is not about asserting on nil in general; I suspect there's as always the internal pesky difference between an "initialised" and an "uninitialised" value. This code works:

package main

type ex int

func (ex) Error() string { return "" }

func main() {
	var i interface{}
	if _, ok := i.(ex); ok {
		println("wat")
	} else {
		println("ok")
	}
}
@thehowl thehowl added 🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related labels Feb 9, 2024
@thehowl thehowl added this to the 🚀 main.gno.land (required) milestone Feb 9, 2024
@thehowl thehowl changed the title type assertion on recovered nils is not possible type assertion on recovered nils panics Feb 9, 2024
@thehowl
Copy link
Member Author

thehowl commented Feb 9, 2024

note: this happened while attempting to use testing.Recover; so when fixing this, a regression test for testing.Recover should be added too.

@deelawn deelawn self-assigned this Feb 12, 2024
@deelawn
Copy link
Contributor

deelawn commented Feb 13, 2024

I was working on a fix for this and may have one, but encountered a separate issue while testing -- #1656. It would be good to get this fixed first so that any tests added along with this fix are certain to behave correctly.

@zivkovicmilos zivkovicmilos moved this from Backlog to Todo in 🧙‍♂️gno.land core team Apr 3, 2024
@deelawn deelawn moved this from Todo to In Review in 🧙‍♂️gno.land core team Apr 8, 2024
@zivkovicmilos zivkovicmilos moved this from In Review to In Progress in 🧙‍♂️gno.land core team Apr 17, 2024
@deelawn deelawn moved this from In Progress to In Review in 🧙‍♂️gno.land core team Apr 24, 2024
@zivkovicmilos zivkovicmilos moved this from In Review to In Progress in 🧙‍♂️gno.land core team May 7, 2024
deelawn added a commit that referenced this issue May 21, 2024
<!-- please provide a detailed description of the changes made in this
pull request. -->
Fixes #1650 

This PR was originally intended to fix the case described in issue
#1650, but it soon became apparent that there were other subtle
inconsistencies between type assertions in go vs gno. The changes
outlined here attempt to fix the issues and the new file tests ensure
correctness. The summary below provides details as to the cases that
were fixed for type assertions, both 1 and 2 -- 1 and 2 referring to the
type assertion op codes, `OpTypeAssert1` and `OpTypeAssert2`. The former
handles type assertions with one LHS argument that panic on assertion
failure and the latter handles those with two LHS arguments that assigns
a boolean value dependent on success or failure.

### Summary of type assertion changes
- fail when the value being asserted has a nil type (i.e. the result of
`recover()` when no panic has occurred). This applies for both cases
where the type being asserted to is an interface or concrete type.
- fail when asserting to an interface type and the underlying type of
the value being asserted is also an interface type (non-concrete type)

Also, is this an accurate assumption?

https://github.com/gnolang/gno/blob/7d4e8e645ca1d2e89f5a8f2e85470e66b3253b33/gnovm/pkg/gnolang/op_expressions.go#L261

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

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [x] 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>
@github-project-automation github-project-automation bot moved this from In Progress to Done in 🧙‍♂️gno.land core team May 21, 2024
leohhhn pushed a commit to leohhhn/gno that referenced this issue May 21, 2024
<!-- please provide a detailed description of the changes made in this
pull request. -->
Fixes gnolang#1650 

This PR was originally intended to fix the case described in issue
gnolang#1650, but it soon became apparent that there were other subtle
inconsistencies between type assertions in go vs gno. The changes
outlined here attempt to fix the issues and the new file tests ensure
correctness. The summary below provides details as to the cases that
were fixed for type assertions, both 1 and 2 -- 1 and 2 referring to the
type assertion op codes, `OpTypeAssert1` and `OpTypeAssert2`. The former
handles type assertions with one LHS argument that panic on assertion
failure and the latter handles those with two LHS arguments that assigns
a boolean value dependent on success or failure.

### Summary of type assertion changes
- fail when the value being asserted has a nil type (i.e. the result of
`recover()` when no panic has occurred). This applies for both cases
where the type being asserted to is an interface or concrete type.
- fail when asserting to an interface type and the underlying type of
the value being asserted is also an interface type (non-concrete type)

Also, is this an accurate assumption?

https://github.com/gnolang/gno/blob/7d4e8e645ca1d2e89f5a8f2e85470e66b3253b33/gnovm/pkg/gnolang/op_expressions.go#L261

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

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [x] 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Development

Successfully merging a pull request may close this issue.

3 participants