os: RemoveAll blocks indefinitely on large directories without permissions #29921

efritz · 2019-01-24T16:42:11Z

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

$ go version
go version go1.11.2 linux/amd64

Does this issue reproduce with the latest release?

Yes - the behavior can still be traced in the source of the master branch.

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

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/efritz/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/efritz/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build861899082=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Create a directory parent with more than 1024 files in it owned by the non-current user. This behavior originally presented itself with a directory mounted in Docker that created 1050 root-owned files locale files. The following script creates a directory parent with such contents.

#!/bin/bash -ex

mkdir -p parent

for i in {0..1025}; do
    touch "parent/$i"
done

chown root:root -R parent/*

Then, run the following go program in the same directory.

package main

import (
	"fmt"
	"os"
)

func main() {
	if err := os.RemoveAll("parent"); err != nil {
		fmt.Printf("> %s\n", err.Error())
		os.Exit(1)
	}

	fmt.Printf("Ok\n")
}

What did you expect to see?

A program terminating with a permissions error.

What did you see instead?

A non-terminating program. Inspecting the program with strace shows the same set of unlink attempts and permissions errors. This is due to the following loop, which attempts to delete the contents of the parent directory in batches of 1024.

go/src/os/removeall_at.go

Line 89 in ba1de79

names, readErr := file.Readdirnames(request)

This logic is subtly wrong, as if all 1024 files cannot be deleted, recurseErr will be set (and not read within the loop) and the subsequent Readdir call will return the same set of filenames. This logic will also be problematic for any directory containing at least 1024 files which cannot be deleted.

The text was updated successfully, but these errors were encountered:

mvdan · 2019-01-24T16:43:52Z

Seems closely related to #20841, fixed in Go 1.11.

efritz · 2019-01-24T17:00:41Z

I just built the 1.11.5 from source and, although the RemoveAll code has changed, the same behavior is still present. Is there a target branch you would like me to confirm this behavior for?

mvdan · 2019-01-24T17:04:53Z

You could try 1.12beta2, though I don't expect it to be fixed. I only meant to point out the related issue.

efritz · 2019-01-24T17:37:25Z

Here is a patch of 1.12beta2 that solves this problem. Hopefully this will more clearly illustrate the error conditions.

diff --git a/src/os/removeall_at.go b/src/os/removeall_at.go
index f0fed6dc33f4..3ef00b1524e7 100644
--- a/src/os/removeall_at.go
+++ b/src/os/removeall_at.go
@@ -74,9 +74,9 @@ func removeAllFrom(parent *File, path string) error {
 
        // Remove the directory's entries
        var recurseErr error
+       var batchSize = 1024
+       const maxBatchSize = 16384
        for {
-               const request = 1024
-
                // Open the directory to recurse into
                file, err := openFdAt(parentFd, path)
                if err != nil {
@@ -86,7 +86,7 @@ func removeAllFrom(parent *File, path string) error {
                        return err
                }
 
-               names, readErr := file.Readdirnames(request)
+               names, readErr := file.Readdirnames(batchSize)
                // Errors other than EOF should stop us from continuing
                if readErr != nil && readErr != io.EOF {
                        file.Close()
@@ -96,9 +96,11 @@ func removeAllFrom(parent *File, path string) error {
                        return readErr
                }
 
+               numRecurseErrs := 0
                for _, name := range names {
                        err := removeAllFrom(file, name)
                        if err != nil {
+                               numRecurseErrs++
                                recurseErr = err
                        }
                }
@@ -111,9 +113,20 @@ func removeAllFrom(parent *File, path string) error {
                file.Close()
 
                // Finish when the end of the directory is reached
-               if len(names) < request {
+               if len(names) < batchSize {
                        break
                }
+
+               if numRecurseErrs == len(names) {
+                       if batchSize >= maxBatchSize {
+                               // recurseErr is necessarily set at this point
+                               break
+                       }
+
+                       // This entire batch could not be deleted, so we
+                       // need to widen our search space.
+                       batchSize *= 2
+               }
        }
 
        // Remove the directory itself

I'm not proposing this as a solution (it may create other cases I'm not familiar with), and there is still a similar logic error for the noat RemoveAll implementation.

bcmills · 2019-01-24T20:54:15Z

CC @ianlancetaylor

antong · 2019-02-06T20:20:31Z

Seems closely related to #20841, fixed in Go 1.11.

I'd bet that fix is what caused this issue :-)

cuonglm · 2019-03-04T10:28:47Z

@efritz

chown root:root -R parent/*

must be:

chown root:root -R parent

to reproduce the issue, otherwise you can still remove the parent.

deprecated-dev · 2019-04-06T07:15:31Z

I have the same problem.

gopherbot · 2019-04-07T07:18:16Z

Change https://golang.org/cl/171099 mentions this issue: os: fix RemoveAll hangs on large directory

gopherbot · 2019-10-25T19:53:14Z

Change https://golang.org/cl/203502 mentions this issue: os: use an actual RemoveAll failure in TestRemoveAllWithMoreErrorThanReqSize

…ReqSize Previously we injected an error, and the injection points were (empirically) not realistic on some platforms. Instead, we now make the directory read-only, which (on most platforms) suffices to prevent the removal of its files. Fixes #35117 Updates #29921 Change-Id: Ica4e2818566f8c14df3eed7c3b8de5c0abeb6963 Reviewed-on: https://go-review.googlesource.com/c/go/+/203502 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

gopherbot · 2020-03-16T23:30:52Z

Change https://golang.org/cl/223700 mentions this issue: [release-branch.go1.13] os: use an actual RemoveAll failure in TestRemoveAllWithMoreErrorThanReqSize

…moveAllWithMoreErrorThanReqSize Previously we injected an error, and the injection points were (empirically) not realistic on some platforms. Instead, we now make the directory read-only, which (on most platforms) suffices to prevent the removal of its files. Also remove unused test hook, as was done in CL 204060. For #35117. For #29921. Fixes #37895. Change-Id: Ica4e2818566f8c14df3eed7c3b8de5c0abeb6963 Reviewed-on: https://go-review.googlesource.com/c/go/+/203502 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> (cherry picked from commit 06bdd52) Reviewed-on: https://go-review.googlesource.com/c/go/+/223700 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Alexander Rakoczy <alex@golang.org>

mvdan added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 24, 2019

mvdan added this to the Go1.13 milestone Jan 24, 2019

ianlancetaylor added the release-blocker label Jan 25, 2019

gopherbot closed this as completed in c349505 Apr 10, 2019

bcmills mentioned this issue Apr 11, 2019

os: TestRemoveAllDotDot broken at head #31421

Closed

monaka mentioned this issue May 25, 2019

[init] Cleanup in parallel. eclipse-che/che-plugin-broker#63

Closed

golang locked and limited conversation to collaborators Mar 16, 2021

gopherbot added the FrozenDueToAge label Mar 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os: RemoveAll blocks indefinitely on large directories without permissions #29921

os: RemoveAll blocks indefinitely on large directories without permissions #29921

efritz commented Jan 24, 2019

mvdan commented Jan 24, 2019

efritz commented Jan 24, 2019

mvdan commented Jan 24, 2019

efritz commented Jan 24, 2019 •

edited

bcmills commented Jan 24, 2019

antong commented Feb 6, 2019

cuonglm commented Mar 4, 2019

deprecated-dev commented Apr 6, 2019

gopherbot commented Apr 7, 2019

gopherbot commented Oct 25, 2019

gopherbot commented Mar 16, 2020

os: RemoveAll blocks indefinitely on large directories without permissions #29921

os: RemoveAll blocks indefinitely on large directories without permissions #29921

Comments

efritz commented Jan 24, 2019

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

mvdan commented Jan 24, 2019

efritz commented Jan 24, 2019

mvdan commented Jan 24, 2019

efritz commented Jan 24, 2019 • edited

bcmills commented Jan 24, 2019

antong commented Feb 6, 2019

cuonglm commented Mar 4, 2019

deprecated-dev commented Apr 6, 2019

gopherbot commented Apr 7, 2019

gopherbot commented Oct 25, 2019

gopherbot commented Mar 16, 2020

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

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

efritz commented Jan 24, 2019 •

edited