Skip to content

Race condition on disableSliceFlagSeparator in flag.go #1670

Closed
Closed
Race condition on disableSliceFlagSeparator in flag.go#1670
Labels
area/v2relates to / is being considered for v2kind/bugdescribes or fixes a bugstatus/triagemaintainers still need to look into this
@zllovesuki

Description

@zllovesuki
zllovesuki
opened on Jan 28, 2023

My urfave/cli version is

github.com/urfave/cli/v2 v2.24.1

Checklist

  • Are you running the latest v2 release? The list of releases is here.
  • Did you check the manual for your release? The v2 manual is here
  • Did you perform a search about this problem? Here's the GitHub guide about searching.

Dependency Management

  • My project is using go modules.

Describe the bug

Running integration tests by passing fake command line arguments to .RunContext(). Allows test to use assertion instead of exec an binary.

To reproduce

  1. A function is used to create a clean *cli.Command every time:
func Generate() *cli.Command {
	return &cli.Command{
		Name:  "server",
// [...]
  1. A helper function is used to compile into an *cli.App:
func compileApp(cmd *cli.Command) (*cli.App, *observer.ObservedLogs) {
	observedZapCore, observedLogs := observer.New(zap.DebugLevel)
	observedLogger := zap.New(observedZapCore)
	return &cli.App{
		Name: "specter",
		Commands: []*cli.Command{
			cmd,
		},
		Before: func(ctx *cli.Context) error {
			ctx.App.Metadata["logger"] = observedLogger
			return nil
		},
		Metadata: make(map[string]interface{}),
	}, observedLogs
}
// [...]
  1. Then create servers dynamically in test:
	serverReturnCtx, serverStopped := context.WithCancel(ctx)
	defer serverStopped()

	t.Logf("Starting servers\n")

	serverLogs := make([]*observer.ObservedLogs, len(serverPorts))
	for i, args := range serverArgs {
		sApp, sLogs := compileApp(server.Generate())
		serverLogs[i] = sLogs
		args := args
		go func(app *cli.App) {
			if err := app.RunContext(ctx, args); err != nil {
				as.NoError(err)
			}
			serverStopped()
		}(sApp)
	}

	t.Logf("Waiting for servers to be started\n")
// [helper function not shown: checking if servers are started]
  1. Then start the test:
    GO_RUN_INTEGRATION=1 go test -v -timeout 30s ./integrations -v -race

Observed behavior

==================
WARNING: DATA RACE
Write at 0x000001bb3d53 by goroutine 11:
  github.com/urfave/cli/v2.(*App).Setup()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/app.go:270 +0x1764
  github.com/urfave/cli/v2.(*App).RunContext()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/app.go:317 +0x7a
  kon.nect.sh/specter/integrations.TestTunnel.func3()
      /home/rachel/code/specter/integrations/tunnel_test.go:164 +0x88
  kon.nect.sh/specter/integrations.TestTunnel.func13()
      /home/rachel/code/specter/integrations/tunnel_test.go:168 +0x47

Previous write at 0x000001bb3d53 by goroutine 10:
  github.com/urfave/cli/v2.(*App).Setup()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/app.go:270 +0x1764
  github.com/urfave/cli/v2.(*App).RunContext()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/app.go:317 +0x7a
  kon.nect.sh/specter/integrations.TestTunnel.func3()
      /home/rachel/code/specter/integrations/tunnel_test.go:164 +0x88
  kon.nect.sh/specter/integrations.TestTunnel.func13()
      /home/rachel/code/specter/integrations/tunnel_test.go:168 +0x47

Goroutine 11 (running) created at:
  kon.nect.sh/specter/integrations.TestTunnel()
      /home/rachel/code/specter/integrations/tunnel_test.go:163 +0x2689
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x47

Goroutine 10 (running) created at:
  kon.nect.sh/specter/integrations.TestTunnel()
      /home/rachel/code/specter/integrations/tunnel_test.go:163 +0x2689
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x47
==================
==================
WARNING: DATA RACE
Write at 0x000001b74e30 by goroutine 11:
  github.com/urfave/cli/v2.(*BoolFlag).Apply()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/flag_bool.go:107 +0x84
  github.com/urfave/cli/v2.flagSet()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/flag.go:174 +0x25b
  github.com/urfave/cli/v2.(*Command).newFlagSet()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:278 +0xc9
  github.com/urfave/cli/v2.(*Command).parseFlags()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:311 +0x5a
  github.com/urfave/cli/v2.(*Command).Run()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:152 +0x14c
  github.com/urfave/cli/v2.(*App).RunContext()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/app.go:333 +0x10e4
  kon.nect.sh/specter/integrations.TestTunnel.func3()
      /home/rachel/code/specter/integrations/tunnel_test.go:164 +0x88
  kon.nect.sh/specter/integrations.TestTunnel.func13()
      /home/rachel/code/specter/integrations/tunnel_test.go:168 +0x47

Previous write at 0x000001b74e30 by goroutine 10:
  github.com/urfave/cli/v2.(*BoolFlag).Apply()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/flag_bool.go:107 +0x84
  github.com/urfave/cli/v2.flagSet()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/flag.go:174 +0x25b
  github.com/urfave/cli/v2.(*Command).newFlagSet()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:278 +0xc9
  github.com/urfave/cli/v2.(*Command).parseFlags()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:311 +0x5a
  github.com/urfave/cli/v2.(*Command).Run()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:152 +0x14c
  github.com/urfave/cli/v2.(*App).RunContext()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/app.go:333 +0x10e4
  kon.nect.sh/specter/integrations.TestTunnel.func3()
      /home/rachel/code/specter/integrations/tunnel_test.go:164 +0x88
  kon.nect.sh/specter/integrations.TestTunnel.func13()
      /home/rachel/code/specter/integrations/tunnel_test.go:168 +0x47

Goroutine 11 (running) created at:
  kon.nect.sh/specter/integrations.TestTunnel()
      /home/rachel/code/specter/integrations/tunnel_test.go:163 +0x2689
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x47

Goroutine 10 (running) created at:
  kon.nect.sh/specter/integrations.TestTunnel()
      /home/rachel/code/specter/integrations/tunnel_test.go:163 +0x2689
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x47
==================
==================
WARNING: DATA RACE
Read at 0x000001b75b28 by goroutine 11:
  github.com/urfave/cli/v2.(*Command).setup()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:133 +0x8d6
  github.com/urfave/cli/v2.(*Command).Run()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:148 +0xa5
  github.com/urfave/cli/v2.(*Command).Run()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:264 +0x128b
  github.com/urfave/cli/v2.(*App).RunContext()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/app.go:333 +0x10e4
  kon.nect.sh/specter/integrations.TestTunnel.func3()
      /home/rachel/code/specter/integrations/tunnel_test.go:164 +0x88
  kon.nect.sh/specter/integrations.TestTunnel.func13()
      /home/rachel/code/specter/integrations/tunnel_test.go:168 +0x47

Previous write at 0x000001b75b28 by goroutine 12:
  github.com/urfave/cli/v2.(*Command).setup()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:134 +0x9be
  github.com/urfave/cli/v2.(*Command).Run()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:148 +0xa5
  github.com/urfave/cli/v2.(*Command).Run()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/command.go:264 +0x128b
  github.com/urfave/cli/v2.(*App).RunContext()
      /home/rachel/go/pkg/mod/github.com/urfave/cli/v2@v2.24.1/app.go:333 +0x10e4
  kon.nect.sh/specter/integrations.TestTunnel.func3()
      /home/rachel/code/specter/integrations/tunnel_test.go:164 +0x88
  kon.nect.sh/specter/integrations.TestTunnel.func13()
      /home/rachel/code/specter/integrations/tunnel_test.go:168 +0x47

Goroutine 11 (running) created at:
  kon.nect.sh/specter/integrations.TestTunnel()
      /home/rachel/code/specter/integrations/tunnel_test.go:163 +0x2689
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x47

Goroutine 12 (running) created at:
  kon.nect.sh/specter/integrations.TestTunnel()
      /home/rachel/code/specter/integrations/tunnel_test.go:163 +0x2689
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x47
==================

Expected behavior

No data race.

Additional context

This actually doesn't affect the test results (servers started correctly with the correct parameters). However,

var (
	defaultSliceFlagSeparator = ","
	disableSliceFlagSeparator = false
)

in flag.go are not protected from concurrent access.

Want to fix this yourself?

Gonna need some pointers

Run go version and paste its output here

go version go1.19.4 linux/amd64

Run go env and paste its output here

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/rachel/.cache/go-build"
GOENV="/home/rachel/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/rachel/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/rachel/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/rachel/code/specter/go.mod"
GOWORK=""
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 -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1222184028=/tmp/go-build -gno-record-gcc-switches"

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/v2relates to / is being considered for v2kind/bugdescribes or fixes a bugstatus/triagemaintainers still need to look into this

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions