Skip to content

Commit 035db07

Browse files
committed
cmd/go,cmd/link: prefer external linking when strange cgo flags seen
This patch changes the Go command to examine the set of compiler flags feeding into the C compiler when packages that use cgo are built. If any of a specific set of strange/dangerous flags are in use, then the Go command generates a token file ("preferlinkext") and embeds it into the compiled package's archive. When the Go linker reads the archives of the packages feeding into the link and detects a "preferlinkext" token, it will then use external linking for the program by default (although this default can be overridden with an explicit "-linkmode" flag). The intent here is to avoid having to teach the Go linker's host object reader to grok/understand the various odd symbols/sections/types that can result from boutique flag use, but rather to just boot the objects in question over to the C linker instead. Updates #58619. Updates #58620. Updates #58848. Change-Id: I56382dd305de8dac3841a7a7e664277826061eaa Reviewed-on: https://go-review.googlesource.com/c/go/+/475375 Reviewed-by: Cherry Mui <[email protected]> Reviewed-by: Bryan Mills <[email protected]> Run-TryBot: Than McIntosh <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
1 parent b37c060 commit 035db07

8 files changed

Lines changed: 294 additions & 15 deletions

File tree

src/cmd/go/internal/work/exec.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3146,6 +3146,36 @@ func (b *Builder) cgo(a *Action, cgoExe, objdir string, pcCFLAGS, pcLDFLAGS, cgo
31463146
}
31473147
}
31483148

3149+
// Scrutinize CFLAGS and related for flags that might cause
3150+
// problems if we are using internal linking (for example, use of
3151+
// plugins, LTO, etc) by calling a helper routine that builds on
3152+
// the existing CGO flags allow-lists. If we see anything
3153+
// suspicious, emit a special token file "preferlinkext" (known to
3154+
// the linker) in the object file to signal the that it should not
3155+
// try to link internally and should revert to external linking.
3156+
// The token we pass is a suggestion, not a mandate; if a user is
3157+
// explicitly asking for a specific linkmode via the "-linkmode"
3158+
// flag, the token will be ignored. NB: in theory we could ditch
3159+
// the token approach and just pass a flag to the linker when we
3160+
// eventually invoke it, and the linker flag could then be
3161+
// documented (although coming up with a simple explanation of the
3162+
// flag might be challenging). For more context see issues #58619,
3163+
// #58620, and #58848.
3164+
flagSources := []string{"CGO_CFLAGS", "CGO_CXXFLAGS", "CGO_FFLAGS"}
3165+
flagLists := [][]string{cgoCFLAGS, cgoCXXFLAGS, cgoFFLAGS}
3166+
if flagsNotCompatibleWithInternalLinking(flagSources, flagLists) {
3167+
tokenFile := objdir + "preferlinkext"
3168+
if cfg.BuildN || cfg.BuildX {
3169+
b.Showcmd("", "echo > %s", tokenFile)
3170+
}
3171+
if !cfg.BuildN {
3172+
if err := os.WriteFile(tokenFile, nil, 0666); err != nil {
3173+
return nil, nil, err
3174+
}
3175+
}
3176+
outObj = append(outObj, tokenFile)
3177+
}
3178+
31493179
if cfg.BuildMSan {
31503180
cgoCFLAGS = append([]string{"-fsanitize=memory"}, cgoCFLAGS...)
31513181
cgoLDFLAGS = append([]string{"-fsanitize=memory"}, cgoLDFLAGS...)
@@ -3395,6 +3425,24 @@ func (b *Builder) cgo(a *Action, cgoExe, objdir string, pcCFLAGS, pcLDFLAGS, cgo
33953425
return outGo, outObj, nil
33963426
}
33973427

3428+
// flagsNotCompatibleWithInternalLinking scans the list of cgo
3429+
// compiler flags (C/C++/Fortran) looking for flags that might cause
3430+
// problems if the build in question uses internal linking. The
3431+
// primary culprits are use of plugins or use of LTO, but we err on
3432+
// the side of caution, supporting only those flags that are on the
3433+
// allow-list for safe flags from security perspective. Return is TRUE
3434+
// if a sensitive flag is found, FALSE otherwise.
3435+
func flagsNotCompatibleWithInternalLinking(sourceList []string, flagListList [][]string) bool {
3436+
for i := range sourceList {
3437+
sn := sourceList[i]
3438+
fll := flagListList[i]
3439+
if err := checkCompilerFlagsForInternalLink(sn, sn, fll); err != nil {
3440+
return true
3441+
}
3442+
}
3443+
return false
3444+
}
3445+
33983446
// dynimport creates a Go source file named importGo containing
33993447
// //go:cgo_import_dynamic directives for each symbol or library
34003448
// dynamically imported by the object files outObj.

src/cmd/go/internal/work/security.go

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -230,32 +230,55 @@ var validLinkerFlagsWithNextArg = []string{
230230
}
231231

232232
func checkCompilerFlags(name, source string, list []string) error {
233-
return checkFlags(name, source, list, validCompilerFlags, validCompilerFlagsWithNextArg)
233+
checkOverrides := true
234+
return checkFlags(name, source, list, validCompilerFlags, validCompilerFlagsWithNextArg, checkOverrides)
234235
}
235236

236237
func checkLinkerFlags(name, source string, list []string) error {
237-
return checkFlags(name, source, list, validLinkerFlags, validLinkerFlagsWithNextArg)
238+
checkOverrides := true
239+
return checkFlags(name, source, list, validLinkerFlags, validLinkerFlagsWithNextArg, checkOverrides)
238240
}
239241

240-
func checkFlags(name, source string, list []string, valid []*lazyregexp.Regexp, validNext []string) error {
242+
// checkCompilerFlagsForInternalLink returns an error if 'list'
243+
// contains a flag or flags that may not be fully supported by
244+
// internal linking (meaning that we should punt the link to the
245+
// external linker).
246+
func checkCompilerFlagsForInternalLink(name, source string, list []string) error {
247+
checkOverrides := false
248+
if err := checkFlags(name, source, list, validCompilerFlags, validCompilerFlagsWithNextArg, checkOverrides); err != nil {
249+
return err
250+
}
251+
// Currently the only flag on the allow list that causes problems
252+
// for the linker is "-flto"; check for it manually here.
253+
for _, fl := range list {
254+
if strings.HasPrefix(fl, "-flto") {
255+
return fmt.Errorf("flag %q triggers external linking", fl)
256+
}
257+
}
258+
return nil
259+
}
260+
261+
func checkFlags(name, source string, list []string, valid []*lazyregexp.Regexp, validNext []string, checkOverrides bool) error {
241262
// Let users override rules with $CGO_CFLAGS_ALLOW, $CGO_CFLAGS_DISALLOW, etc.
242263
var (
243264
allow *regexp.Regexp
244265
disallow *regexp.Regexp
245266
)
246-
if env := cfg.Getenv("CGO_" + name + "_ALLOW"); env != "" {
247-
r, err := regexp.Compile(env)
248-
if err != nil {
249-
return fmt.Errorf("parsing $CGO_%s_ALLOW: %v", name, err)
267+
if checkOverrides {
268+
if env := cfg.Getenv("CGO_" + name + "_ALLOW"); env != "" {
269+
r, err := regexp.Compile(env)
270+
if err != nil {
271+
return fmt.Errorf("parsing $CGO_%s_ALLOW: %v", name, err)
272+
}
273+
allow = r
250274
}
251-
allow = r
252-
}
253-
if env := cfg.Getenv("CGO_" + name + "_DISALLOW"); env != "" {
254-
r, err := regexp.Compile(env)
255-
if err != nil {
256-
return fmt.Errorf("parsing $CGO_%s_DISALLOW: %v", name, err)
275+
if env := cfg.Getenv("CGO_" + name + "_DISALLOW"); env != "" {
276+
r, err := regexp.Compile(env)
277+
if err != nil {
278+
return fmt.Errorf("parsing $CGO_%s_DISALLOW: %v", name, err)
279+
}
280+
disallow = r
257281
}
258-
disallow = r
259282
}
260283

261284
Args:

src/cmd/go/internal/work/security_test.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package work
66

77
import (
88
"os"
9+
"strings"
910
"testing"
1011
)
1112

@@ -28,6 +29,7 @@ var goodCompilerFlags = [][]string{
2829
{"-Wp,-Ufoo"},
2930
{"-Wp,-Dfoo1"},
3031
{"-Wp,-Ufoo1"},
32+
{"-flto"},
3133
{"-fobjc-arc"},
3234
{"-fno-objc-arc"},
3335
{"-fomit-frame-pointer"},
@@ -278,3 +280,34 @@ func TestCheckFlagAllowDisallow(t *testing.T) {
278280
t.Fatalf("missing error for -fplugin=lint.so: %v", err)
279281
}
280282
}
283+
284+
func TestCheckCompilerFlagsForInternalLink(t *testing.T) {
285+
// Any "bad" compiler flag should trigger external linking.
286+
for _, f := range badCompilerFlags {
287+
if err := checkCompilerFlagsForInternalLink("test", "test", f); err == nil {
288+
t.Errorf("missing error for %q", f)
289+
}
290+
}
291+
292+
// All "good" compiler flags should not trigger external linking,
293+
// except for anything that begins with "-flto".
294+
for _, f := range goodCompilerFlags {
295+
foundLTO := false
296+
for _, s := range f {
297+
if strings.Contains(s, "-flto") {
298+
foundLTO = true
299+
}
300+
}
301+
if err := checkCompilerFlagsForInternalLink("test", "test", f); err != nil {
302+
// expect error for -flto
303+
if !foundLTO {
304+
t.Errorf("unexpected error for %q: %v", f, err)
305+
}
306+
} else {
307+
// expect no error for everything else
308+
if foundLTO {
309+
t.Errorf("missing error for %q: %v", f, err)
310+
}
311+
}
312+
}
313+
}

src/cmd/go/scriptconds_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ func scriptConditions() map[string]script.Cond {
4949
add("link", lazyBool("testenv.HasLink()", testenv.HasLink))
5050
add("mismatched-goroot", script.Condition("test's GOROOT_FINAL does not match the real GOROOT", isMismatchedGoroot))
5151
add("msan", sysCondition("-msan", platform.MSanSupported, true))
52+
add("cgolinkext", script.BoolCondition("platform requires external linking for cgo", platform.MustLinkExternal(cfg.Goos, cfg.Goarch, true)))
5253
add("net", lazyBool("testenv.HasExternalNetwork()", testenv.HasExternalNetwork))
5354
add("race", sysCondition("-race", platform.RaceDetectorSupported, true))
5455
add("symlink", lazyBool("testenv.HasSymlink()", testenv.HasSymlink))

src/cmd/go/testdata/script/README

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,8 @@ The available conditions are:
382382
$WORK filesystem is case-sensitive
383383
[cgo]
384384
host CGO_ENABLED
385+
[cgolinkext]
386+
platform requires external linking for cgo
385387
[compiler:*]
386388
runtime.Compiler == <suffix>
387389
[cross]
Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
# Test case to verify that when we have a package that uses CGO in
2+
# combination with selected "unusual" flags (involving plugins, LTO)
3+
# that we force external linking. See related
4+
# issues 58619, 58620, and 58848.
5+
6+
[compiler:gccgo] skip # only external linking for gccgo
7+
8+
# This test requires external linking, so use cgo as a proxy
9+
[!cgo] skip
10+
11+
# Here we build three program: one with explicit CGO use, one with no
12+
# CGO use, and one that uses a stdlib package ("runtime/cgo") that has
13+
# CGO in it. It used to be that only the explicit use of CGO would
14+
# trigger external linking, and that the program that only used
15+
# "runtime/cgo" would always be handled with internal linking. This caused
16+
# issues when users included odd/unusual flags (ex: -fplugin, -flto)
17+
# in CGO_CFLAGS, causing the Go linker to have to read and interpret
18+
# non-standard host objects.
19+
#
20+
# As of 1.21 we continue to use internal linking for programs whose
21+
# CGO use comes ony from stdlib packages in the absence of any flag
22+
# funny business, however if the Go command sees flags that may be suspicious,
23+
# it signals the Go linker to invoke the external linker.
24+
25+
# The next few tests run builds passing "-n" to the Go command, then
26+
# checking the output to see if the Go command is trying to pass a
27+
# "preferlinkext" token to the linker to request external linking.
28+
29+
#-----------------------
30+
31+
# Use a fresh GOCACHE for these next steps, so as to have the real
32+
# actions for the runtime/cgo package appear in the "-n -x" output.
33+
env GOCACHE=$WORK/gocache
34+
mkdir $GOCACHE
35+
36+
# First build: there is no CGO in use, so no token should be present regardless
37+
# of weird CGO flags.
38+
go build -x -n -o dummy.exe ./noUseOfCgo
39+
! stderr preferlinkext
40+
env CGO_CFLAGS=-flto
41+
go build -x -n -o dummy.exe ./noUseOfCgo
42+
! stderr preferlinkext
43+
env CGO_CFLAGS=
44+
45+
# Second build uses CGO, so we expect to see the token present in the
46+
# -n output only when strange flags are used.
47+
go build -x -n -o dummy.exe ./usesInternalCgo
48+
! stderr preferlinkext
49+
env CGO_CFLAGS=-flto
50+
go build -x -n -o dummy.exe ./usesInternalCgo
51+
stderr preferlinkext
52+
env CGO_CFLAGS=-fplugin
53+
go build -x -n -o dummy.exe ./usesInternalCgo
54+
stderr preferlinkext
55+
env CGO_CFLAGS=-fprofile-instr-generate
56+
go build -x -n -o dummy.exe ./usesInternalCgo
57+
stderr preferlinkext
58+
env CGO_CFLAGS=
59+
60+
[short] skip
61+
62+
# In the remaining tests below we do actual builds (without -n) to
63+
# verify that the Go linker is going the right thing in addition to the
64+
# Go command. Here the idea is to pass "-tmpdir" to the linker, then
65+
# check after the link is done for the presence of the file
66+
# <tmpdir>/go.o, which the Go linker creates prior to kicking off the
67+
# external linker.
68+
69+
mkdir tmp1
70+
mkdir tmp2
71+
mkdir tmp3
72+
mkdir tmp4
73+
mkdir tmp5
74+
75+
# First build: no external linking expected
76+
go build -ldflags=-tmpdir=tmp1 -o $devnull ./noUseOfCgo &
77+
78+
# Second build: using only "runtime/cgo", expect internal linking.
79+
go build -ldflags=-tmpdir=tmp2 -o $devnull ./usesInternalCgo &
80+
81+
# Third build: program uses only "runtime/cgo", so we would normally
82+
# expect internal linking, except that cflags contain suspicious entries
83+
# (in this case, a flag that does not appear on the allow list).
84+
env CGO_CFLAGS=-fmerge-all-constants
85+
env CGO_LDFLAGS=-fmerge-all-constants
86+
go build -ldflags=-tmpdir=tmp3 -o $devnull ./usesInternalCgo &
87+
env CGO_CFLAGS=
88+
env CGO_LDFLAGS=
89+
90+
# Fourth build: explicit CGO, expect external linking.
91+
go build -ldflags=-tmpdir=tmp4 -o $devnull ./usesExplicitCgo &
92+
93+
# Fifth build: explicit CGO, but we specifically asked for internal linking
94+
# via a flag, so using internal linking it is.
95+
[cgolinkext] go list ./usesInternalCgo
96+
[!cgolinkext] go build '-ldflags=-tmpdir=tmp5 -linkmode=internal' -o $devnull ./usesInternalCgo &
97+
98+
wait
99+
100+
# Check first build: no external linking expected
101+
! exists tmp1/go.o
102+
103+
# Check second build: using only "runtime/cgo", expect internal linking.
104+
[!cgolinkext] ! exists tmp2/go.o
105+
[cgolinkext] exists tmp2/go.o
106+
107+
# Check third build: has suspicious flag.
108+
exists tmp3/go.o
109+
110+
# Fourth build: explicit CGO, expect external linking.
111+
exists tmp4/go.o
112+
113+
# Fifth build: explicit CGO, -linkmode=internal.
114+
! exists tmp5/go.o
115+
116+
-- go.mod --
117+
118+
module cgo.example
119+
120+
go 1.20
121+
122+
-- noUseOfCgo/main.go --
123+
124+
package main
125+
126+
func main() {
127+
println("clean as a whistle")
128+
}
129+
130+
-- usesInternalCgo/main.go --
131+
132+
package main
133+
134+
import (
135+
"runtime/cgo"
136+
)
137+
138+
func main() {
139+
q := "hello"
140+
h := cgo.NewHandle(q)
141+
h.Delete()
142+
}
143+
144+
-- usesExplicitCgo/main.go --
145+
146+
package main
147+
148+
/*
149+
int meaningOfLife() { return 42; }
150+
*/
151+
import "C"
152+
153+
func main() {
154+
println(C.meaningOfLife())
155+
}

src/cmd/link/internal/ld/config.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,11 @@ func determineLinkMode(ctxt *Link) {
198198
ctxt.LinkMode = LinkExternal
199199
via = "via GO_EXTLINK_ENABLED "
200200
default:
201-
if extNeeded || (iscgo && externalobj) {
201+
preferExternal := len(preferlinkext) != 0
202+
if preferExternal && ctxt.Debugvlog > 0 {
203+
ctxt.Logf("external linking prefer list is %v\n", preferlinkext)
204+
}
205+
if extNeeded || (iscgo && (externalobj || preferExternal)) {
202206
ctxt.LinkMode = LinkExternal
203207
} else {
204208
ctxt.LinkMode = LinkInternal

src/cmd/link/internal/ld/lib.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,12 @@ var (
341341
// any of these objects, we must link externally. Issue 52863.
342342
dynimportfail []string
343343

344+
// preferlinkext is a list of packages for which the Go command
345+
// noticed use of peculiar C flags. If we see any of these,
346+
// default to linking externally unless overridden by the
347+
// user. See issues #58619, #58620, and #58848.
348+
preferlinkext []string
349+
344350
// unknownObjFormat is set to true if we see an object whose
345351
// format we don't recognize.
346352
unknownObjFormat = false
@@ -1086,6 +1092,13 @@ func loadobjfile(ctxt *Link, lib *sym.Library) {
10861092
if arhdr.name == "dynimportfail" {
10871093
dynimportfail = append(dynimportfail, lib.Pkg)
10881094
}
1095+
if arhdr.name == "preferlinkext" {
1096+
// Ignore this directive if -linkmode has been
1097+
// set explicitly.
1098+
if ctxt.LinkMode == LinkAuto {
1099+
preferlinkext = append(preferlinkext, lib.Pkg)
1100+
}
1101+
}
10891102

10901103
// Skip other special (non-object-file) sections that
10911104
// build tools may have added. Such sections must have

0 commit comments

Comments
 (0)