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

os: RemoveAll failing silently in go 1.12.2 when removing non-empty directories at / #31468

Closed
BooleanCat opened this issue Apr 15, 2019 · 9 comments

Comments

@BooleanCat
Copy link

@BooleanCat BooleanCat commented Apr 15, 2019

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

1.12.{2|3|4}

Does this issue reproduce with the latest release?

Yes

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

go env Output
docker run \
                --rm \
                -v /Users/pivotal/workspace/deletepls:/deletepls \
                -w /deletepls \
                -it golang:1.12.4-alpine3.9 \
                go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
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=/tmp/go-build8
97413456=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I work on the garden project, we help orchestrate container lifecycle operations. I bumped the version of go 1.12.4 we build with and saw a consistent failure in our CI for a test related to removing a file.

I see the same failure on go 1.12.2 and go 1.12.3, but not on go 1.12.1.

I have reproduced the issue with a small go binary here (I'm going to continue to tease out the issue here, for now it's doing roughly what my code in the garden project is doing):

package main

import (
	"os"
	"path/filepath"
)

func main() {
	hello := filepath.Join("/", "hello")

	if err := os.MkdirAll(filepath.Join(hello, "there"), 0700); err != nil {
		panic(err)
	}

	if err := os.RemoveAll(hello); err != nil {
		panic(err)
	}

	if _, err := os.Stat(hello); err == nil {
		panic("what?")
	}
}

Edited with simpler solution: the problem seems to occur when I try to RemoveAll a directory positioned at / after having ChDir'd to /.

Edited again: Chdir is not needed to repro

I run make with this makefile

.PHONY = run

tag ?= 1.12.4-alpine3.9

run:
	docker run \
		--rm \
		-v /Users/pivotal/workspace/deletepls:/deletepls \
		-w /deletepls \
		-it golang:$(tag) \
		go run main.go

... and observe a panic on "what?" (indicating that I was able to stat a file I just deleted).

When I run make tag=1.12.1-alpine3.9 I observe no panic.

What did you expect to see?

I expect this snippet to not panic.

What did you see instead?

The snippet panics on go 1.12.2+ but not on 1.12.1.

@BooleanCat
Copy link
Author

@BooleanCat BooleanCat commented Apr 15, 2019

Interestingly, I can delete /hello with RemoveAll if I RemoveAll("hello") rather than RemoveAll("/hello") (i.e. use a relative path).

@BooleanCat
Copy link
Author

@BooleanCat BooleanCat commented Apr 15, 2019

This might be the culprit - if I revert this line it works as I expect: d039e12#diff-55fac2af5cab3979fb5c47e41d21e0e4L54

@BooleanCat BooleanCat changed the title os.RemoveAll failing silently in go 1.12.2 (still figuring out exact circumstances) os.RemoveAll failing silently in go 1.12.2 when removing non-empty directories at / Apr 15, 2019
@BooleanCat
Copy link
Author

@BooleanCat BooleanCat commented Apr 15, 2019

Resolved in PR - will sign CLA and other admin tomorrow.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 15, 2019

Thanks--I have a slightly different patch with a test.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 15, 2019

@gopherbot please open backport to 1.12

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 15, 2019

Backport issue(s) opened: #31474 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 15, 2019

Change https://golang.org/cl/172058 mentions this issue: os: don't treat RemoveAll("/x") as RemoveAll("x")

@bradfitz bradfitz changed the title os.RemoveAll failing silently in go 1.12.2 when removing non-empty directories at / os: RemoveAll failing silently in go 1.12.2 when removing non-empty directories at / Apr 15, 2019
@BooleanCat
Copy link
Author

@BooleanCat BooleanCat commented Apr 15, 2019

@ianlancetaylor I was thinking about how I'd write a test for this given I'd need to write at / to reproduce with RemoveAll, but saw the export test file - neat trick!

@gopherbot gopherbot closed this in 3ebd952 Apr 15, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 15, 2019

Change https://golang.org/cl/172080 mentions this issue: [release-branch.go1.12] os: don't treat RemoveAll("/x") as RemoveAll("x")

gopherbot pushed a commit that referenced this issue Apr 15, 2019
…"x")

Updates #31468
Fixes #31474

Change-Id: I5c4e61631b8af35bfc14b0cb9bc77feec100e340
Reviewed-on: https://go-review.googlesource.com/c/go/+/172058
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 3ebd952)
Reviewed-on: https://go-review.googlesource.com/c/go/+/172080
@golang golang locked and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.