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

x/tools/godoc/vfs: missing root directory listing when mounting vfs.NameSpace via httpfs and serving by http.FileServer #14190

Closed
srinathh opened this issue Feb 2, 2016 · 7 comments
Milestone

Comments

@srinathh
Copy link
Contributor

@srinathh srinathh commented Feb 2, 2016

I'm trying to mount a few folders into a virtual file system via vfs.NameSpace and serving them using http.FileServer through a httpfs.New(). I am unable to see a listing of the root folder of NameSpace and get a 404 page not found.

Interestingly, when i do NameSpace.ReadDir("/") on the NameSpace and loop through the results, i see the expected folders. There are also no problems getting a directory listing via either http.Dir() or httpfs.New(vfs.OS("FolderName")).

Also when I directly browse the mounted folders (as explained below), I see the folders mounted properly. Only the root directory is not listing.

package main

import (
    "golang.org/x/tools/godoc/vfs"
    "golang.org/x/tools/godoc/vfs/httpfs"
    "log"
    "net/http"
    "os"
)

func main() {
    // Binding two folders to the namespace
    ns := vfs.NameSpace{}
    ns.Bind("/music", vfs.OS("Folder1"), "/", vfs.BindReplace)
    ns.Bind("/videos", vfs.OS("Folder2"), "/", vfs.BindReplace)

    // Reading the root of the namespace. This works as expected and prints "music" and "videos"
    fis, err := ns.ReadDir("/")
    if err != nil {
        log.Println(err)
        os.Exit(1)
    }

    for _, fi := range fis {
        log.Println(fi.Name())
    }

    // This works when browsing http://127.0.0.1:9090/music/ or http://127.0.0.1:9090/videos/
    // but produces a 404 not found when browsing http://127.0.0.1:9090/. The expected behavior
    // should be that a directory listing showing the folders music and videos should be displayed.
    http.Handle("/", http.FileServer(httpfs.New(ns)))
    http.ListenAndServe(":9090", http.DefaultServeMux)
}
@bradfitz bradfitz changed the title Missing root directory listing when mounting vfs.NameSpace via httpfs and serving by http.FileServer x/tools/godoc/vfs: missing root directory listing when mounting vfs.NameSpace via httpfs and serving by http.FileServer Feb 2, 2016
@bradfitz bradfitz added this to the Unreleased milestone Feb 2, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Feb 2, 2016

godoc never relies on that functionality so it's possible it's just a bug we never noticed.

@srinathh
Copy link
Contributor Author

@srinathh srinathh commented Feb 13, 2016

I have figured out the reason this bug is happening. The issue is that behavior of vfs.NameSpace is not consistent with expectations of how a normal file system behaves and that causes typical directory traversal algorithms to break on vfs.NameSpace.

The issue is that NameSpace is implemented with the implicit assumption that it will be used with a FileSystem mounted at root mount point with other FileSystems mounted somewhere within that. This is of course the use case of godoc as also explained in the documentation of vfs.NameSpace where GOROOT is being mounted at root.

The use case above mounts several folders under root but nothing at root breaking this implicit assumption. Unless we explicitly bind a FileSystem directory at root mount-point "/" of vfs.NameSpace , running Open() or Stat() on root "/" causes a not found error (or a return value that won't be a directory if we mount a file). This happens even though ReadDir() correctly resolves and lists the contents of the root directory as expected.

Most algorithms that traverse a directory structure either Open() or Stat() the root before reading the contents. The not found error that comes when a directory is not explicitly bound at the root mount causes the traversal routine to fail fast without trying to list directory contents. This happens in http.FileServer and I had a similar failure in another piece of directory traversing code I am writing.

I was able to get the right behavior by If i mount an empty directory at root like below.

package main

import (
    "golang.org/x/tools/godoc/vfs"
    "golang.org/x/tools/godoc/vfs/httpfs"
    "log"
    "net/http"
    "os"
)

func main() {
    ns := vfs.NameSpace{}
    // Folder3 is simply an empty folder to be mounted at the root mount point
    ns.Bind("/", vfs.OS("Folder3"), "/", vfs.BindReplace)
    ns.Bind("/music", vfs.OS("Folder1"), "/", vfs.BindReplace)
    ns.Bind("/videos", vfs.OS("Folder2"), "/", vfs.BindReplace)

    fis, err := ns.ReadDir("/")
    if err != nil {
        log.Println(err)
        os.Exit(1)
    }
    for _, fi := range fis {
        log.Println(fi.Name())
    }
    http.Handle("/", http.FileServer(httpfs.New(ns)))
    http.ListenAndServe(":9090", http.DefaultServeMux)
}

I think the least disruptive resolution to this issue will be to add a NewNamespace() function to create a NameSpace that automatically mounts at root mount point "/" an implementation of FileSystem that simply emulates an empty directory so this surprising behavior doesn't occur. The advantage of this approach is that absolutely nothing would change for any current usage of NameSpace by godoc and any other existing programs while new programs can just use the provided function.

Happy to work on a CL if the approach sounds ok.

@srinathh
Copy link
Contributor Author

@srinathh srinathh commented Feb 14, 2016

I have a working implementation of a fix for this issue at https://github.com/srinathh/emptyvfs for review/discussion of the proposed solution approach.

If the approach looks ok, I can move the implementation into vfs package and initiate the code-review package.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Feb 14, 2016

Feel free to send a code review. In addition to being easier to review in Gerrit, it will also guarantee that you've signed a CLA (since our Gerrit doesn't allow uploads before a CLA is signed). Reviewers shouldn't be reviewing code elsewhere (e.g. your personal github repo) without knowing a CLA is on file anyway.

@srinathh
Copy link
Contributor Author

@srinathh srinathh commented Feb 15, 2016

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 15, 2016

CL https://golang.org/cl/19445 mentions this issue.

@srinathh
Copy link
Contributor Author

@srinathh srinathh commented Mar 16, 2016

Hi Brad, did you get a chance to review the CL?

@golang golang locked and limited conversation to collaborators Mar 21, 2017
Bogdan-D pushed a commit to Bogdan-D/godocx that referenced this issue Nov 13, 2019
The existing implementation of NameSpace implicitly assumes that a
FileSystem with a directory at the top will be mounted at the root
mount point "/" of the NameSpace. If this is not the case, then
Stat("/") will fail even if ReadDir("/") succeedes. This is unexpected
behavior which breaks directory traversal routines (eg. http.FileServer).

This CL adds an unexported implementation of FileSystem called emptyVFS
that emulates an empty directory and adds a NewNameSpace() function that
binds emptyVFS to "/" so that unexpected behavior does not arise even if
the use does not mount anything explicitly at "/".

Latest patch set causes the FileInfo of the empty top level emulated
directory to return "/" for Name() and Zero Time for ModTime() and
removes the related struct state fields being used in the previous
implementation.

Fixes golang/go#14190

Change-Id: Idce2fc3c9b81206847a33840d76b660059d53d18
Reviewed-on: https://go-review.googlesource.com/19445
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
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.

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