Skip to content

x/tools/godoc/vfs/mapfs: easy to accidentally misuse API, causing severe problems #34591

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
dmitshur opened this issue Sep 28, 2019 · 3 comments
Closed

x/tools/godoc/vfs/mapfs: easy to accidentally misuse API, causing severe problems #34591

dmitshur opened this issue Sep 28, 2019 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Sep 28, 2019 •
edited
Loading

mapfs.New documentation says:

[...] Map keys should be forward slash-separated pathnames and not contain a leading slash.

However, there is no validation on the input. If the user accidentally provides map keys with leading slashes, it results in incorrect programs and hard-to-diagnose problems.

For example:

package main

import (
	"fmt"

	"golang.org/x/tools/godoc/vfs/mapfs"
)

func main() {
	fs := mapfs.New(map[string]string{
		// Incorrect mapfs.New usage, map key with a leading slash.
		"/doc/a.txt": "a",
	})
	for _, path := range []string{
		// Both of these are valid paths.
		"/doc/a.txt",
		"doc/a.txt",
	} {
		_, err := fs.Open(path)
		fmt.Printf("fs.Open(%q): %v\n", path, err)
		_, err = fs.Stat(path)
		fmt.Printf("fs.Stat(%q): %v\n", path, err)
		_, err = fs.Lstat(path)
		fmt.Printf("fs.Lstat(%q): %v\n", path, err)
	}
}

It produces output:

fs.Open("/doc/a.txt"): file does not exist
fs.Stat("/doc/a.txt"): file does not exist
fs.Lstat("/doc/a.txt"): file does not exist
fs.Open("doc/a.txt"): file does not exist
fs.Stat("doc/a.txt"): file does not exist
fs.Lstat("doc/a.txt"): file does not exist

When mapfs.New is used correctly, like so:

fs := mapfs.New(map[string]string{
	// Correct mapfs.New usage, map key has no leading slash.
	"doc/a.txt": "a",
})

Then the output is as expected:

fs.Open("/doc/a.txt"): <nil>
fs.Stat("/doc/a.txt"): <nil>
fs.Lstat("/doc/a.txt"): <nil>
fs.Open("doc/a.txt"): <nil>
fs.Stat("doc/a.txt"): <nil>
fs.Lstat("doc/a.txt"): <nil>

I think it would be net helpful to perform validation in New and help users learn as quickly as possible when they're misusing the API by panicking with a descriptive error message. Panics aren't good, but incorrect results can be even worse. Most mapfs.New usage happens at program initialization time and in tests.

/cc @jayconrod
@dmitshur dmitshur added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository. labels Sep 28, 2019
@dmitshur dmitshur added this to the Unreleased milestone Sep 28, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/197859 mentions this issue: godoc/vfs/mapfs: panic on invalid New usage

@dmitshur
Copy link
Contributor Author

I think this can be fixed in the following ways:

  1. detect invalid API usage and panic with a helpful message

  2. detect invalid API usage and log a helpful message to stderr

  3. change API to allow leading slashes, and start to support them

I've sent a CL that implements option 1 to see if it'd catch any invalid usages in x/tools, and there weren't any.

The downside of option 3 is that it makes API harder to use: when leading slash is optional, it's no longer clear whether one should include it or not. It will lead to more inconsistent styles. It makes mapfs more complicated, and I'd prefer not to do this.

That leaves 1 or 2. Given there are no invalid usages in x/tools (nor in x/website, I checked), I'd prefer 1. I believe a hard-to-miss panic will save people time and cause fewer problems in the long term. mapfs is primarily an internal godoc support package, so we should have more freedom to change it compared to more general-purpose packages.

@dmitshur dmitshur self-assigned this May 8, 2020
@dmitshur
Copy link
Contributor Author

dmitshur commented May 8, 2020 •
edited
Loading

I've thought about this quite a bit, and I still think option 1 has the best trade-offs based on what we know now. If this newly added panic happens somewhere, I currently believe learning about that will bring greater positive value than the disruption it would cause. If we learn of new evidence suggesting otherwise, we can revert and consider another approach.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels May 8, 2020
@golang golang locked and limited conversation to collaborators May 8, 2021
henderjon pushed a commit to oggodoc/godoc that referenced this issue Jun 13, 2024
@dmitshur
godoc/vfs/mapfs: panic on invalid New usage
7072b74 
mapfs.New documentation says:

> Map keys should be forward slash-separated pathnames
> and not contain a leading slash.

It is invalid input if a provided path contains a leading slash, and it
causes the returned filesystem to have undefined behavior. Package mapfs
is often used in tests, so this can lead to a serious problem elsewhere.

Help detect invalid API usage sooner by validating input, and panicking
when it contains leading slashes. Programs that use mapfs API correctly
will be unaffected, and it will be faster for incorrect programs—if any
exist today or will be created in the future—to detect and correct such
problems.

Fixes golang/go#34591.

Change-Id: I77e5f0f4628edf83480604135f58bfb62e521d80
Reviewed-on: https://go-review.googlesource.com/c/tools/+/197859
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
2 participants
@dmitshur @gopherbot