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

[ericchiang]

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]

/cc @dsnet @bradfitz

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

[ericchiang]

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
	})
}

[stefanobaghino]

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]

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]

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]

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]

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