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 blocks indefinitely on large directories without permissions #29921

Closed
efritz opened this issue Jan 24, 2019 · 11 comments
Closed

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

efritz opened this issue Jan 24, 2019 · 11 comments

Comments

@efritz
Copy link

@efritz efritz commented Jan 24, 2019

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.

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.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jan 24, 2019

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

@efritz

This comment has been minimized.

Copy link
Author

@efritz efritz commented Jan 24, 2019

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

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jan 24, 2019

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

@efritz

This comment has been minimized.

Copy link
Author

@efritz efritz commented Jan 24, 2019

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.

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

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 24, 2019

@antong

This comment has been minimized.

Copy link
Contributor

@antong antong commented Feb 6, 2019

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

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

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Mar 4, 2019

@efritz

chown root:root -R parent/*

must be:

chown root:root -R parent

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

@cxfans

This comment has been minimized.

Copy link

@cxfans cxfans commented Apr 6, 2019

I have the same problem.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 7, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 25, 2019

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

gopherbot pushed a commit that referenced this issue Oct 26, 2019
…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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 16, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.