Skip to content

archive/zip: fs.ReadDir(..., ".") can return "." and cause fs.WalkDir to stack overflow #50179

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.

Sign up for GitHub

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

Jump to bottom
Closed
ericchiang opened this issue Dec 15, 2021 · 7 comments
Closed

archive/zip: fs.ReadDir(..., ".") can return "." and cause fs.WalkDir to stack overflow #50179

ericchiang opened this issue Dec 15, 2021 · 7 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@ericchiang
Copy link
Contributor

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

$ go version
go version go1.17.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go en
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/ericchiang/Library/Caches/go-build"
GOENV="/Users/ericchiang/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/ericchiang/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/ericchiang/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.5"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/d1/wpvcpdrs2tlgn5xs3_xm88th0000gn/T/go-build1948477688=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Self contained reproducer: https://go.dev/play/p/APYHtDb8IVg

We were attempting to scan a JAR (which are ZIPs) using archive/zip with fs.WalkDir and found that it was crashing our program. Our code looks something like this:

zr, err := zip.NewReader(r, size)
if err != nil {
    // ...
}
// WalkDir was looping forever and stack overflowing.
fs.WalkDir(zr, ".", func(path string, d fs.DirEntry, err error) error {
    // logic
})

After digging into it, it appeared that our ZIP has an entry for "." and fs.ReadDir(zr, ".") returned "." as a result, causing fs.WalkDir to walk "." over and over again.

We worked around this by wrapping the *zip.Reader in a struct that overrode ReadDir()

type zipFS struct { *zip.Reader }
func (z *zipFS) ReadDir(name string) ([]fs.DirEntry, error) {
    entries, err := fs.ReadDir(z.Reader, name)
    if err != nil {
        return nil, err
    }
    j := 0
    for i := 0; i < len(entries); i++ {
        if entires[i].Name() == name {
            // ReadDir() returned the same name as a value, prevent infinite loop.
            continue
        }
        entries[j] = entries[i]
        j++
    }
    entires = entries[:j]
    return entries, nil
}

What did you expect to see?

Running the playground link above, I'd expect fs.WalkDir to return normally.

% go run hi.go 
.
test

What did you see instead?

Instead it stack overflows.

% go run hi.go
.
test
runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0xc020280408 stack=[0xc020280000, 0xc040280000]
fatal error: stack overflow

runtime stack:
runtime.throw({0x10d13b5, 0x1175060})
	/usr/local/go/src/runtime/panic.go:1198 +0x71
runtime.newstack()
	/usr/local/go/src/runtime/stack.go:1088 +0x5ac
runtime.morestack()
	/usr/local/go/src/runtime/asm_amd64.s:461 +0x8b

goroutine 1 [running]:
runtime.heapBitsSetType(0xc006405260, 0x10, 0x10, 0x10bfd60)
	/usr/local/go/src/runtime/mbitmap.go:822 +0xbcc fp=0xc020280418 sp=0xc020280410 pc=0x10136ec
runtime.mallocgc(0x10, 0x10bfd60, 0x1)
	/usr/local/go/src/runtime/malloc.go:1100 +0x65e fp=0xc020280498 sp=0xc020280418 pc=0x100c09e
internal/reflectlite.unsafe_New(0x10b7de0)
	/usr/local/go/src/runtime/malloc.go:1238 +0x27 fp=0xc0202804c0 sp=0xc020280498 pc=0x1057c47
internal/reflectlite.Swapper({0x10b7de0, 0xc00641a498})
	/usr/local/go/src/internal/reflectlite/swapper.go:65 +0x3c5 fp=0xc0202805b8 sp=0xc0202804c0 pc=0x1060005
sort.Slice({0x10b7de0, 0xc00641a498}, 0x2)
	/usr/local/go/src/sort/slice.go:18 +0x58 fp=0xc020280618 sp=0xc0202805b8 pc=0x1086158
io/fs.ReadDir({0x10f2a20, 0xc0000c0070}, {0x10cfbe5, 0x1085dd8})
	/usr/local/go/src/io/fs/readdir.go:45 +0x2b9 fp=0xc0202806f8 sp=0xc020280618 pc=0x10887b9
io/fs.walkDir({0x10f2a20, 0xc0000c0070}, {0x10cfbe5, 0x1}, {0x28a190a8, 0xc0000ca000}, 0x10d80f0)
	/usr/local/go/src/io/fs/walk.go:74 +0x14c fp=0xc0202807b0 sp=0xc0202806f8 pc=0x1088e4c
io/fs.walkDir({0x10f2a20, 0xc0000c0070}, {0x10cfbe5, 0x1}, {0x28a190a8, 0xc0000ca000}, 0x10d80f0)
	/usr/local/go/src/io/fs/walk.go:85 +0x27f fp=0xc020280868 sp=0xc0202807b0 pc=0x1088f7f
io/fs.walkDir({0x10f2a20, 0xc0000c0070}, {0x10cfbe5, 0x1}, {0x28a190a8, 0xc0000ca000}, 0x10d80f0)
	/usr/local/go/src/io/fs/walk.go:85 +0x27f fp=0xc020280920 sp=0xc020280868 pc=0x1088f7f
@ericchiang
Copy link
Contributor Author

/cc @dsnet @bradfitz

Per https://dev.golang.org/owners

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 15, 2021
@cherrymui cherrymui added this to the Backlog milestone Dec 15, 2021
@ericchiang
Copy link
Contributor Author

Smaller reproducer

package main

import (
	"archive/zip"
	"bytes"
	"io/fs"
	"log"
	"time"
)

func main() {
	h := &zip.FileHeader{Name: ".", Modified: time.Now()}
	h.SetMode(fs.ModeDir | 0755)

	b := &bytes.Buffer{}
	zw := zip.NewWriter(b)
	if _, err := zw.CreateHeader(h); err != nil {
		log.Fatalf("create header: %v", err)
	}
	if err := zw.Close(); err != nil {
		log.Fatalf("closing zip writier: %v", err)
	}

	r := bytes.NewReader(b.Bytes())
	zr, err := zip.NewReader(r, r.Size())
	if err != nil {
		log.Fatalf("create new reader: %v", err)
	}

	// WalkDir loops forever.
	fs.WalkDir(zr, ".", func(path string, d fs.DirEntry, err error) error {
		return nil
	})
}

This was referenced Dec 30, 2021
Closed
Merged
@stefanobaghino
Copy link

I tried to run the reproducer on Go 1.21 and it looks like this has been solved, the output is:

.
test

as expected.

@ianlancetaylor
Copy link
Member

When I run the test case in a terminal, not in the Go playground, I see it print

.
test
runtime: goroutine stack exceeds 1000000000-byte limit

followed by a stack trace.

@seankhliao
Copy link
Member

As far as I know, a lone . isn't a valid filename anywhere.
Should the archive/zip filesystem return an error on ReadDir if there's such an entry?
Or should io/fs.WalkDir guard against it by ignoring or erroring?

@ianlancetaylor
Copy link
Member

I don't think we should change anything in io/fs.

I suppose the archive/zip ReadDir method should return an error if it sees a file named ".". There is no reasonable way that we can handle that.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/656495 mentions this issue: archive/zip: error on ReadDir if there are invalid file names

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@stefanobaghino @ericchiang @ianlancetaylor @gopherbot @seankhliao @cherrymui