From c342872276d4a3d5f662684115e282cbb20bf227 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 5 Feb 2021 13:02:47 -0800 Subject: [PATCH 1/3] libct/config: fix a data race As reported by go test -race ./libcontainer/configs: === RUN TestCommandHookRunTimeout ================== WARNING: DATA RACE Read at 0x00c000202230 by goroutine 23: os/exec.(*Cmd).Wait() /usr/lib/golang/src/os/exec/exec.go:502 +0x91 github.com/opencontainers/runc/libcontainer/configs.Command.Run() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/configs/config.go:390 +0x58c github.com/opencontainers/runc/libcontainer/configs_test.TestCommandHookRunTimeout() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/configs/config_test.go:223 +0x3ed testing.tRunner() /usr/lib/golang/src/testing/testing.go:1123 +0x202 Previous write at 0x00c000202230 by goroutine 27: os/exec.(*Cmd).Wait() /usr/lib/golang/src/os/exec/exec.go:505 +0xb4 github.com/opencontainers/runc/libcontainer/configs.Command.Run.func1() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/configs/config.go:373 +0x55 Goroutine 23 (running) created at: testing.(*T).Run() /usr/lib/golang/src/testing/testing.go:1168 +0x5bb testing.runTests.func1() /usr/lib/golang/src/testing/testing.go:1439 +0xa6 testing.tRunner() /usr/lib/golang/src/testing/testing.go:1123 +0x202 testing.runTests() /usr/lib/golang/src/testing/testing.go:1437 +0x612 testing.(*M).Run() /usr/lib/golang/src/testing/testing.go:1345 +0x3b3 main.main() _testmain.go:69 +0x236 Goroutine 27 (running) created at: github.com/opencontainers/runc/libcontainer/configs.Command.Run() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/configs/config.go:372 +0x415 github.com/opencontainers/runc/libcontainer/configs_test.TestCommandHookRunTimeout() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/configs/config_test.go:223 +0x3ed testing.tRunner() /usr/lib/golang/src/testing/testing.go:1123 +0x202 ================== testing.go:1038: race detected during execution of test --- FAIL: TestCommandHookRunTimeout (0.10s) Apparently, the issue is we call two Wait()s for the same command which can race internally. Fix is easy -- since we already have a waiting goroutine, wait for it to return instead of calling a second Wait(). Signed-off-by: Kir Kolyshkin --- libcontainer/configs/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcontainer/configs/config.go b/libcontainer/configs/config.go index e1cd16265..3808f79a8 100644 --- a/libcontainer/configs/config.go +++ b/libcontainer/configs/config.go @@ -387,7 +387,7 @@ func (c Command) Run(s *specs.State) error { return err case <-timerCh: cmd.Process.Kill() - cmd.Wait() + <-errC return fmt.Errorf("hook ran past specified timeout of %.1fs", c.Timeout.Seconds()) } } From b7744547368ba758fc2947e423cb812c69537492 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 8 Feb 2021 15:33:24 -0800 Subject: [PATCH 2/3] libct/int: fix a data race Fix for the following: $ sudo go test -race -v ./libcontainer/integration/ === RUN TestUsernsCheckpoint ================== WARNING: DATA RACE Write at 0x00c00019e650 by goroutine 15: bytes.(*Buffer).ReadFrom() /usr/lib/golang/src/bytes/buffer.go:200 +0x48 io.copyBuffer() /usr/lib/golang/src/io/io.go:395 +0x3fe io.Copy() /usr/lib/golang/src/io/io.go:368 +0x7a os/exec.(*Cmd).writerDescriptor.func1() /usr/lib/golang/src/os/exec/exec.go:311 +0x4a os/exec.(*Cmd).Start.func1() /usr/lib/golang/src/os/exec/exec.go:441 +0x38 Previous write at 0x00c00019e650 by goroutine 12: bytes.(*Buffer).ReadFrom() /usr/lib/golang/src/bytes/buffer.go:200 +0x48 io.copyBuffer() /usr/lib/golang/src/io/io.go:395 +0x3fe io.Copy() /usr/lib/golang/src/io/io.go:368 +0x7a os/exec.(*Cmd).writerDescriptor.func1() /usr/lib/golang/src/os/exec/exec.go:311 +0x4a os/exec.(*Cmd).Start.func1() /usr/lib/golang/src/os/exec/exec.go:441 +0x38 Goroutine 15 (running) created at: os/exec.(*Cmd).Start() /usr/lib/golang/src/os/exec/exec.go:440 +0xaaa github.com/opencontainers/runc/libcontainer.(*linuxContainer).criuSwrk() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:1490 +0x5f7 github.com/opencontainers/runc/libcontainer.(*linuxContainer).Restore() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:1398 +0x29ef github.com/opencontainers/runc/libcontainer/integration.testCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:204 +0x1898 github.com/opencontainers/runc/libcontainer/integration.TestUsernsCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:48 +0x177 testing.tRunner() /usr/lib/golang/src/testing/testing.go:1123 +0x202 Goroutine 12 (finished) created at: os/exec.(*Cmd).Start() /usr/lib/golang/src/os/exec/exec.go:440 +0xaaa github.com/opencontainers/runc/libcontainer.(*initProcess).start() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/process_linux.go:313 +0xda github.com/opencontainers/runc/libcontainer.(*linuxContainer).start() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:366 +0xb0 github.com/opencontainers/runc/libcontainer.(*linuxContainer).Start() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:263 +0x27b github.com/opencontainers/runc/libcontainer.(*linuxContainer).Run() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:273 +0x46 github.com/opencontainers/runc/libcontainer/integration.testCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:108 +0x941 github.com/opencontainers/runc/libcontainer/integration.TestUsernsCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:48 +0x177 testing.tRunner() /usr/lib/golang/src/testing/testing.go:1123 +0x202 ================== ================== WARNING: DATA RACE Read at 0x00c00019e630 by goroutine 15: bytes.(*Buffer).Len() /usr/lib/golang/src/bytes/buffer.go:73 +0x58 bytes.(*Buffer).grow() /usr/lib/golang/src/bytes/buffer.go:118 +0x4f bytes.(*Buffer).ReadFrom() /usr/lib/golang/src/bytes/buffer.go:202 +0x7c io.copyBuffer() /usr/lib/golang/src/io/io.go:395 +0x3fe io.Copy() /usr/lib/golang/src/io/io.go:368 +0x7a os/exec.(*Cmd).writerDescriptor.func1() /usr/lib/golang/src/os/exec/exec.go:311 +0x4a os/exec.(*Cmd).Start.func1() /usr/lib/golang/src/os/exec/exec.go:441 +0x38 Previous write at 0x00c00019e630 by goroutine 12: bytes.(*Buffer).ReadFrom() /usr/lib/golang/src/bytes/buffer.go:209 +0x1c4 io.copyBuffer() /usr/lib/golang/src/io/io.go:395 +0x3fe io.Copy() /usr/lib/golang/src/io/io.go:368 +0x7a os/exec.(*Cmd).writerDescriptor.func1() /usr/lib/golang/src/os/exec/exec.go:311 +0x4a os/exec.(*Cmd).Start.func1() /usr/lib/golang/src/os/exec/exec.go:441 +0x38 Goroutine 15 (running) created at: os/exec.(*Cmd).Start() /usr/lib/golang/src/os/exec/exec.go:440 +0xaaa github.com/opencontainers/runc/libcontainer.(*linuxContainer).criuSwrk() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:1490 +0x5f7 github.com/opencontainers/runc/libcontainer.(*linuxContainer).Restore() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:1398 +0x29ef github.com/opencontainers/runc/libcontainer/integration.testCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:204 +0x1898 github.com/opencontainers/runc/libcontainer/integration.TestUsernsCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:48 +0x177 testing.tRunner() /usr/lib/golang/src/testing/testing.go:1123 +0x202 Goroutine 12 (finished) created at: os/exec.(*Cmd).Start() /usr/lib/golang/src/os/exec/exec.go:440 +0xaaa github.com/opencontainers/runc/libcontainer.(*initProcess).start() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/process_linux.go:313 +0xda github.com/opencontainers/runc/libcontainer.(*linuxContainer).start() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:366 +0xb0 github.com/opencontainers/runc/libcontainer.(*linuxContainer).Start() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:263 +0x27b github.com/opencontainers/runc/libcontainer.(*linuxContainer).Run() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:273 +0x46 github.com/opencontainers/runc/libcontainer/integration.testCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:108 +0x941 github.com/opencontainers/runc/libcontainer/integration.TestUsernsCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:48 +0x177 testing.tRunner() /usr/lib/golang/src/testing/testing.go:1123 +0x202 ================== ================== WARNING: DATA RACE Read at 0x00c00019e648 by goroutine 15: bytes.(*Buffer).Len() /usr/lib/golang/src/bytes/buffer.go:73 +0x7e bytes.(*Buffer).grow() /usr/lib/golang/src/bytes/buffer.go:118 +0x4f bytes.(*Buffer).ReadFrom() /usr/lib/golang/src/bytes/buffer.go:202 +0x7c io.copyBuffer() /usr/lib/golang/src/io/io.go:395 +0x3fe io.Copy() /usr/lib/golang/src/io/io.go:368 +0x7a os/exec.(*Cmd).writerDescriptor.func1() /usr/lib/golang/src/os/exec/exec.go:311 +0x4a os/exec.(*Cmd).Start.func1() /usr/lib/golang/src/os/exec/exec.go:441 +0x38 Previous write at 0x00c00019e648 by goroutine 12: bytes.(*Buffer).grow() /usr/lib/golang/src/bytes/buffer.go:147 +0x2d7 bytes.(*Buffer).ReadFrom() /usr/lib/golang/src/bytes/buffer.go:202 +0x7c io.copyBuffer() /usr/lib/golang/src/io/io.go:395 +0x3fe io.Copy() /usr/lib/golang/src/io/io.go:368 +0x7a os/exec.(*Cmd).writerDescriptor.func1() /usr/lib/golang/src/os/exec/exec.go:311 +0x4a os/exec.(*Cmd).Start.func1() /usr/lib/golang/src/os/exec/exec.go:441 +0x38 Goroutine 15 (running) created at: os/exec.(*Cmd).Start() /usr/lib/golang/src/os/exec/exec.go:440 +0xaaa github.com/opencontainers/runc/libcontainer.(*linuxContainer).criuSwrk() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:1490 +0x5f7 github.com/opencontainers/runc/libcontainer.(*linuxContainer).Restore() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:1398 +0x29ef github.com/opencontainers/runc/libcontainer/integration.testCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:204 +0x1898 github.com/opencontainers/runc/libcontainer/integration.TestUsernsCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:48 +0x177 testing.tRunner() /usr/lib/golang/src/testing/testing.go:1123 +0x202 Goroutine 12 (finished) created at: os/exec.(*Cmd).Start() /usr/lib/golang/src/os/exec/exec.go:440 +0xaaa github.com/opencontainers/runc/libcontainer.(*initProcess).start() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/process_linux.go:313 +0xda github.com/opencontainers/runc/libcontainer.(*linuxContainer).start() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:366 +0xb0 github.com/opencontainers/runc/libcontainer.(*linuxContainer).Start() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:263 +0x27b github.com/opencontainers/runc/libcontainer.(*linuxContainer).Run() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/container_linux.go:273 +0x46 github.com/opencontainers/runc/libcontainer/integration.testCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:108 +0x941 github.com/opencontainers/runc/libcontainer/integration.TestUsernsCheckpoint() /home/kir/go/src/github.com/opencontainers/runc/libcontainer/integration/checkpoint_test.go:48 +0x177 testing.tRunner() /usr/lib/golang/src/testing/testing.go:1123 +0x202 ================== testing.go:1038: race detected during execution of test --- FAIL: TestUsernsCheckpoint (0.62s) Signed-off-by: Kir Kolyshkin --- libcontainer/integration/checkpoint_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libcontainer/integration/checkpoint_test.go b/libcontainer/integration/checkpoint_test.go index 627a013f2..cecf0b547 100644 --- a/libcontainer/integration/checkpoint_test.go +++ b/libcontainer/integration/checkpoint_test.go @@ -194,10 +194,11 @@ func testCheckpoint(t *testing.T, userns bool) { t.Fatal(err) } + var restoreStdout bytes.Buffer restoreProcessConfig := &libcontainer.Process{ Cwd: "/", Stdin: restoreStdinR, - Stdout: &stdout, + Stdout: &restoreStdout, Init: true, } @@ -242,7 +243,7 @@ func testCheckpoint(t *testing.T, userns bool) { t.Fatal(s.String(), pid) } - output := stdout.String() + output := restoreStdout.String() if !strings.Contains(output, "Hello!") { t.Fatal("Did not restore the pipe correctly:", output) } From d73b4443efde8f9a88f01c08f5225bd3120a4564 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 8 Feb 2021 15:38:27 -0800 Subject: [PATCH 3/3] ci: enable -race from matrix Add a new test matrix dimension so all tests are run twice, with and without race detector. Signed-off-by: Kir Kolyshkin --- .github/workflows/test.yml | 5 +++-- Makefile | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 683977c1c..62c665d8e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -21,6 +21,7 @@ jobs: matrix: go-version: [1.14.x, 1.15.x, 1.16.0-rc1] rootless: ["rootless", ""] + race: ["-race", ""] steps: @@ -41,7 +42,7 @@ jobs: go-version: ${{ matrix.go-version }} - name: build - run: sudo -E PATH="$PATH" make all + run: sudo -E PATH="$PATH" make EXTRA_FLAGS="${{ matrix.race }}" all - name: install bats uses: mig4/setup-bats@v1 @@ -50,7 +51,7 @@ jobs: - name: unit test if: matrix.rootless != 'rootless' - run: sudo -E PATH="$PATH" -- make localunittest + run: sudo -E PATH="$PATH" -- make TESTFLAGS="${{ matrix.race }}" localunittest - name: add rootless user if: matrix.rootless == 'rootless' diff --git a/Makefile b/Makefile index e13d8f1d4..bb5c17c94 100644 --- a/Makefile +++ b/Makefile @@ -20,7 +20,9 @@ ifneq ($(GO111MODULE),off) endif ifeq ($(shell $(GO) env GOOS),linux) ifeq (,$(filter $(shell $(GO) env GOARCH),mips mipsle mips64 mips64le ppc64)) - GO_BUILDMODE := "-buildmode=pie" + ifeq (,$(findstring -race,$(EXTRA_FLAGS))) + GO_BUILDMODE := "-buildmode=pie" + endif endif endif GO_BUILD := $(GO) build -trimpath $(MOD_VENDOR) $(GO_BUILDMODE) $(EXTRA_FLAGS) -tags "$(BUILDTAGS)" \