Skip to content

runtime: incorrect unwind in panicwrap #7491

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

Closed
rsc opened this issue Mar 7, 2014 · 5 comments
Closed

runtime: incorrect unwind in panicwrap #7491

rsc opened this issue Mar 7, 2014 · 5 comments
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Mar 7, 2014

the panicwrap code does not clean up properly on panic (ironically).

http://play.golang.org/p/7bR-7ZA1VU

package main

import "runtime"

func main() {
    test11()
}

type Tiny struct{}

func (Tiny) M() {
    panic("asdf")
}

// calling i.M increments panicwrap, but then i.M panics, and panicwrap
// is never restored. because panicwrap is now wrong, the recover
// in T3 after the call to badstate does not see the panic that it should.
func badstate() {
    defer func() {
        println(recover())
    }()
    var i I = Tiny{}
    i.M()
}

// -- code below this line copied from test/recover.go except marked line--

// tiny receiver, so basic wrapper in i.M()
type T3 struct{}

func (T3) M() {
    badstate() // THIS IS NEW!
    mustRecoverBody(doubleRecover(), recover(), recover(), 11)
}

func test11() {
    var i I = T3{}
    defer i.M()
    panic(11)
}

type I interface {
    M()
}

func mustRecoverBody(v1, v2, v3, x interface{}) {
    v := v1
    if v != nil {
        println("spurious recover", v)
        die()
    }
    v = v2
    if v == nil {
        println("missing recover")
        die() // panic is useless here
    }
    if v != x {
        println("wrong value", v, x)
        die()
    }

    // the value should be gone now regardless
    v = v3
    if v != nil {
        println("recover didn't recover")
        die()
    }
}

func doubleRecover() interface{} {
    return recover()
}

func mustRecover(x interface{}) {
    mustRecoverBody(doubleRecover(), recover(), recover(), x)
}

func die() {
    runtime.Breakpoint() // can't depend on panic
}
@dvyukov
Copy link
Member

dvyukov commented Mar 7, 2014

Comment 1:

I am not sure whether it's the same bug, but I've just discovered and fixed memory
corruption and memory leak in recursive panic handling. I will send a fix soon.
Reproducer is:
func main() {
    func() {
        defer func() {
            fmt.Println(recover())
        }()
        var x [8192]byte
        func(x [8192]byte) {
            defer func() {
                if err := recover(); err != nil {
                    panic("1: " + err.(string))
                }
            }()
            func(x [8192]byte) {
                defer func() {
                    if err := recover(); err != nil {
                        panic("2: " + err.(string))
                    }
                }()
                panic("bad")
            }(x)
        }(x)
    }()
    panic("again")
}

@dvyukov
Copy link
Member

dvyukov commented Mar 7, 2014

Comment 2:

maybe it's a different one, I do not see how panicwrap is involved in my bug

@rsc
Copy link
Contributor Author

rsc commented May 9, 2014

Comment 3:

This is too uncommon a case to try to fix for Go 1.3 at this point. It has literally
never come up in practice.

Labels changed: added release-go1.4, removed release-go1.3.

@gopherbot
Copy link
Contributor

Comment 4:

CL https://golang.org/cl/135490044 mentions this issue.

@rsc
Copy link
Contributor Author

rsc commented Sep 6, 2014

Comment 5:

This issue was closed by revision 8473695.

Status changed to Fixed.

@rsc rsc added fixed labels Sep 6, 2014
@rsc rsc added this to the Go1.4 milestone Apr 14, 2015
@rsc rsc removed the release-go1.4 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
The gp->panicwrap adjustment is just fatally flawed.
Now that there is a Panic.argp field, update that instead.
That can be done on entry only, so that unwinding doesn't
need to worry about undoing anything. The wrappers
emit a few more instructions in the prologue but everything
else in the system gets much simpler.

It also fixes (without trying) a broken test I never checked in.

Fixes golang#7491.

LGTM=khr
R=khr
CC=dvyukov, golang-codereviews, iant, r
https://golang.org/cl/135490044
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
The gp->panicwrap adjustment is just fatally flawed.
Now that there is a Panic.argp field, update that instead.
That can be done on entry only, so that unwinding doesn't
need to worry about undoing anything. The wrappers
emit a few more instructions in the prologue but everything
else in the system gets much simpler.

It also fixes (without trying) a broken test I never checked in.

Fixes golang#7491.

LGTM=khr
R=khr
CC=dvyukov, golang-codereviews, iant, r
https://golang.org/cl/135490044
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants