Server prevents walks to ".."

[djdv]

intro(5) says

All directories must support walks to the directory .. (dot–dot) meaning parent directory, although by convention directories contain no explicit entry for .. or . (dot). The parent of the root directory of a server's tree is itself.

And walk(5) says

The name .. (dot–dot) represents the parent directory. ... A walk of the name .. in the root directory of a server is equivalent to a walk with no name elements.

I don't see any mention of this in diod's protocol document, but I do see it used in their tests: https://github.com/chaos/diod/blob/9da28f911978957dbec251c653200db7a4dcad6e/tests/user/testopenfid.c#L72

Which leads me to believe the same requirement for 9P, still holds true for 9P2000.L.

However, even if File implementations handle a dot-dot request inside their Walk method, Server explicitly forbids the request from being issued to the File.

https://github.com/hugelgupf/p9/blob/49c780c51a3091e9209a490b365460da8f75c9f8/p9/handlers.go#L1165 calls: https://github.com/hugelgupf/p9/blob/49c780c51a3091e9209a490b365460da8f75c9f8/p9/handlers.go#L1183 calls: https://github.com/hugelgupf/p9/blob/49c780c51a3091e9209a490b365460da8f75c9f8/p9/handlers.go#L1065-L1070

Bypassing the check for .. seems to work as expected (tested against cmd\p9ufs with Client; even trying to escape the root resolves to just . of p9ufs's -root argument).

for _, name := range names {
+   if name == ".." {
+       continue
+   }
    err = checkSafeName(name)
    if err != nil {
        return
    }
}

However, I'm not familiar enough with how the library tracks fid references, so I'm not sure if such a simple exception to the name rules somehow breaks reference handling in some way. The logic around here is concerning https://github.com/hugelgupf/p9/blob/49c780c51a3091e9209a490b365460da8f75c9f8/p9/handlers.go#L1146 since newRef should actually be walkRef's parent or walkRef itself if walkRef is the root; but we add newRef as a child of walkRef.

doWalk currently handles the case for clones (nil names), and for steps (some string name). But if the logic for stepping isn't also valid for backtracking (".." names), support for that may have to be added. I'm not sure.

[hugelgupf]

I think you are correct, a 9P2000.L server should allow this behavior. I believe this is one of the custom gVisor modifications we made for security reasons -- we knew our client would never use ".." and we wanted to prevent users from walking outside of our root directory if they compromised the sentry.

[djdv]

Thanks for the context. This can probably be closed since gVisor requires this. Or left open for visibility.

To add context on my end, I ended up not needing to use .. cllient-side either, although there were some times where it would have made things easier. Specifically, I remember writing something like grep that walks a tree and returns a list of {name, fid} pairs. These files would be read and the data inspected, if it matched something - the file was to be unlinked. If I could walk to .. then I could easily call unlinkat in that scope, alternatively if remove wasn't deprecated that would work too. (While it's not exposed to the client, all the files in my system keep track of their parent and update it when moving around / renamed. Which should allow messages like rename and remove to function properly.)

To work around this, the files in question were already storing data that made reconstructing their path easy. (Similar to how Plan 9's network clone and ctl files return data from read that tells you the name you need to walk to.) So on a match I'd just reconstruct the path and remove the final name in order to issue an unlinkat.

[hugelgupf]

Oh -- I think we can totally fix this. gVisor doesn't use this library -- I forked this out of gVisor so p9 would get better usability (use net instead of gVisor's unet etc) and to bring it into 9P2000.L spec compliance because of gVisor's modifications. I'd be happy to take a PR for this!

[djdv]

Awesome! I have to take care of some stuff today but will look at the patch again and submit it later. (I remember it working, so it should be fine but since it's been sitting and unused for a while; it needs a sanity check.)