cmd/cgo: reject attempts to declare methods on C types

This change causes cgo to emit an error (with the same
message as the compiler) when it encounters a declaration
of a method whose receiver type is C.T or *C.T.

Conceptually, C is another package, but unfortunately
the desugaring of C.T is a type within the same package,
causing the previous behavior to accept invalid input.

It is likely that at least some users are intentionally
exploiting this behavior, so this may break their build.
We should mention it in the release notes.

Fixes #57926

Change-Id: I513cffb7e13bc93d08a07b7e61301ac1762fd42d
Reviewed-on: https://go-review.googlesource.com/c/go/+/490819
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>

diff --git a/src/cmd/cgo/ast.go b/src/cmd/cgo/ast.go
index 81060c67edd79291f8b13cf45cf5ed0bb664625c..6a1cf387208e5e15f5f532a118c5b853d1dd25b9 100644 (file)
--- a/src/cmd/cgo/ast.go
+++ b/src/cmd/cgo/ast.go
@@ -9,6 +9,7 @@ package main
 import (
        "fmt"
        "go/ast"
+       "go/format"
        "go/parser"
        "go/scanner"
        "go/token"
@@ -62,29 +63,48 @@ func (f *File) ParseGo(abspath string, src []byte) {
        // In ast1, find the import "C" line and get any extra C preamble.
        sawC := false
        for _, decl := range ast1.Decls {
-               d, ok := decl.(*ast.GenDecl)
-               if !ok {
-                       continue
-               }
-               for _, spec := range d.Specs {
-                       s, ok := spec.(*ast.ImportSpec)
-                       if !ok || s.Path.Value != `"C"` {
-                               continue
-                       }
-                       sawC = true
-                       if s.Name != nil {
-                               error_(s.Path.Pos(), `cannot rename import "C"`)
-                       }
-                       cg := s.Doc
-                       if cg == nil && len(d.Specs) == 1 {
-                               cg = d.Doc
+               switch decl := decl.(type) {
+               case *ast.GenDecl:
+                       for _, spec := range decl.Specs {
+                               s, ok := spec.(*ast.ImportSpec)
+                               if !ok || s.Path.Value != `"C"` {
+                                       continue
+                               }
+                               sawC = true
+                               if s.Name != nil {
+                                       error_(s.Path.Pos(), `cannot rename import "C"`)
+                               }
+                               cg := s.Doc
+                               if cg == nil && len(decl.Specs) == 1 {
+                                       cg = decl.Doc
+                               }
+                               if cg != nil {
+                                       f.Preamble += fmt.Sprintf("#line %d %q\n", sourceLine(cg), abspath)
+                                       f.Preamble += commentText(cg) + "\n"
+                                       f.Preamble += "#line 1 \"cgo-generated-wrapper\"\n"
+                               }
                        }
-                       if cg != nil {
-                               f.Preamble += fmt.Sprintf("#line %d %q\n", sourceLine(cg), abspath)
-                               f.Preamble += commentText(cg) + "\n"
-                               f.Preamble += "#line 1 \"cgo-generated-wrapper\"\n"
+
+               case *ast.FuncDecl:
+                       // Also, reject attempts to declare methods on C.T or *C.T.
+                       // (The generated code would otherwise accept this
+                       // invalid input; see issue #57926.)
+                       if decl.Recv != nil && len(decl.Recv.List) > 0 {
+                               recvType := decl.Recv.List[0].Type
+                               if recvType != nil {
+                                       t := recvType
+                                       if star, ok := unparen(t).(*ast.StarExpr); ok {
+                                               t = star.X
+                                       }
+                                       if sel, ok := unparen(t).(*ast.SelectorExpr); ok {
+                                               var buf strings.Builder
+                                               format.Node(&buf, fset, recvType)
+                                               error_(sel.Pos(), `cannot define new methods on non-local type %s`, &buf)
+                                       }
+                               }
                        }
                }
+
        }
        if !sawC {
                error_(ast1.Package, `cannot find import "C"`)
@@ -542,3 +562,11 @@ func (f *File) walk(x interface{}, context astContext, visit func(*File, interfa
                }
        }
 }
+
+// If x is of the form (T), unparen returns unparen(T), otherwise it returns x.
+func unparen(x ast.Expr) ast.Expr {
+       if p, isParen := x.(*ast.ParenExpr); isParen {
+               x = unparen(p.X)
+       }
+       return x
+}

diff --git a/src/cmd/go/testdata/script/cgo_badmethod_issue57926.txt b/src/cmd/go/testdata/script/cgo_badmethod_issue57926.txt
new file mode 100644 (file)
index 0000000..81ef850
--- /dev/null
+++ b/src/cmd/go/testdata/script/cgo_badmethod_issue57926.txt
@@ -0,0 +1,31 @@
+[short] skip
+[!cgo] skip
+
+# Test that cgo rejects attempts to declare methods
+# on the types C.T or *C.T; see issue #57926.
+
+! go build
+stderr 'cannot define new methods on non-local type C.T'
+stderr 'cannot define new methods on non-local type \*C.T'
+! stderr 'Alias'
+
+-- go.mod --
+module example.com
+go 1.12
+
+-- a.go --
+package a
+
+/*
+typedef int T;
+*/
+import "C"
+
+func (C.T) f() {}
+func (recv *C.T) g() {}
+
+// The check is more education than enforcement,
+// and is easily defeated using a type alias.
+type Alias = C.T
+func (Alias) h() {}
+func (*Alias) i() {}