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

cmd/compile: short-circuiting interface-to-concrete comparisons misses panics #32187

Closed
skinass opened this issue May 22, 2019 · 6 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@skinass
Copy link

skinass commented May 22, 2019

What version of Go are you using (go version)?

$ go version
go version go1.11.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/a.sulaev/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/a.sulaev/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.4/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.4/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/wb/q3d56j697hs6zfvjgt3n88bc0000gp/T/go-build625061947=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Here is the code + play.golang.org link on it :

package main

import (
	"errors"
)

type SomeType struct{}

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

func main() {
	var x error
	x = errors.New("aa")
	switch x {
	case x.(*SomeType):
		println("*SomeType")
	case x:
		println("x")
	}

	// println(x.(*SomeType))
}

What did you expect to see?

I was expecting to see panic at the line case x.(*SomeType): because of the bad type casting.

For example if i uncomment the line // println(x.(*SomeType)) (which casts x the same way as in case) i get the panic

What did you see instead?

This code successfully compiles and prints "x" when running.

@skinass
Copy link
Author

skinass commented May 22, 2019

UPD:

package main

import (
	"errors"
)

type SomeType struct{}

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

func main() {
	var x error
	x = errors.New("aa")
	switch x {
	case x.(SomeType):
		println("SomeType")
	case x:
		println("x")
	}
}

if i cast to x.(SomeType) in place of x.(*SomeType) i get the panic

@randall77
Copy link
Contributor

Definitely weird. That should panic.

I suspect something in the compiler is getting mixed up between expression switches (which this is) and type switches.

Is this derived from some real code? It looks bizarre to me. The reason I ask is because it affects priority - if this happens in the real world we should probably get a fix in for 1.13; if not it can wait for 1.14.

Go gets this right up to and including 1.4. The bug starts in 1.5.
gccgo gets this right.

@skinass
Copy link
Author

skinass commented May 22, 2019

Definitely weird. That should panic.

I suspect something in the compiler is getting mixed up between expression switches (which this is) and type switches.

Is this derived from some real code? It looks bizarre to me. The reason I ask is because it affects priority - if this happens in the real world we should probably get a fix in for 1.13; if not it can wait for 1.14.

Go gets this right up to and including 1.4. The bug starts in 1.5.
gccgo gets this right.

I got something similar while reviewing a pull request where the code works fine.
And i started wondering why this doesn't panic. I dont think we are going to use this "trick" in production, but it might happen to someone else.

@randall77
Copy link
Contributor

It just seems weird to compare an interface against a concrete type that's type-casted from an interface. You might as well compare them directly. In other words, if x and y are interfaces, then x == y.(T) is the same as x == y, except that the former would panic if y was not a T.

@bcmills bcmills changed the title Tricky type cast inside switch/case block cmd/compile: invalid type assertion in switch case fails to panic May 22, 2019
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 22, 2019
@bcmills bcmills added this to the Go1.14 milestone May 22, 2019
@mdempsky
Copy link
Member

Simpler repros:

package main

func main() {
    var x interface{}
    println(x == x.(*int)) // should panic, but prints false

    var p *int
    println(x == *p)  // should panic, but prints false

    var s []string
    println(x == s[10])  // should panic, but prints false
}

The problem is for interface-to-concrete comparisons, we're short circuiting on the interface value's dynamic type before evaluating the concrete expression for side effects.

The problem doesn't happen for x.(SomeType) because order.go calls copyExpr on non-direct interface types (i.e., non-pointer types):

case ODOTTYPE, ODOTTYPE2:
n.Left = o.expr(n.Left, nil)
// TODO(rsc): The isfat is for consistency with componentgen and walkexpr.
// It needs to be removed in all three places.
// That would allow inlining x.(struct{*int}) the same as x.(*int).
if !isdirectiface(n.Type) || isfat(n.Type) || instrumenting {
n = o.copyExpr(n, n.Type, true)
}

@mdempsky mdempsky changed the title cmd/compile: invalid type assertion in switch case fails to panic cmd/compile: short-circuiting interface-to-concrete comparisons misses panics May 23, 2019
@gopherbot
Copy link

Change https://golang.org/cl/178817 mentions this issue: cmd/compile: ensure interface-to-concrete comparison panics when it should

tomocy pushed a commit to tomocy/go that referenced this issue Sep 1, 2019
…hould

In interface-to-concrete comparisons, we are short circuiting on the interface
value's dynamic type before evaluating the concrete expression for side effects,
causing concrete expression won't panic at runtime, while it should.

To fix it, evaluating the RHS of comparison before we do the short-circuit.

We also want to prioritize panics in the LHS over the RHS, so evaluating
the LHS too.

Fixes golang#32187

Change-Id: I15b58a523491b7fd1856b8fdb9ba0cba5d11ebb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/178817
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
…hould

In interface-to-concrete comparisons, we are short circuiting on the interface
value's dynamic type before evaluating the concrete expression for side effects,
causing concrete expression won't panic at runtime, while it should.

To fix it, evaluating the RHS of comparison before we do the short-circuit.

We also want to prioritize panics in the LHS over the RHS, so evaluating
the LHS too.

Fixes golang#32187

Change-Id: I15b58a523491b7fd1856b8fdb9ba0cba5d11ebb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/178817
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@golang golang locked and limited conversation to collaborators Aug 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants