Skip to content

Commit

Permalink
avoid using sign.Results.Closing when it's invalid
Browse files Browse the repository at this point in the history
The position may be token.NoPos, an invalid position,
if there's a single result parameter without parentheses.
Using token.FileSet.Offset on token.NoPos+1 would panic:

	panic: invalid Pos value 1 (should be in [774665, 781950])

We didn't catch this because our tests always start with an empty
token.FileSet, so most of our files start with the base position 1.
A user ran into the bug when formatting many files at once.

To ensure we catch these bugs quicker in the future,
insert a dummy ten-byte file to every new FileSet we create.

Finally, add a test, which reproduces the panic thanks to the dummy base
file mentioned above.

Fixes #166.
  • Loading branch information
mvdan committed Nov 11, 2021
1 parent ed953e2 commit 9d037d4
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 2 deletions.
13 changes: 11 additions & 2 deletions format/format.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ type Options struct {
// source file.
func Source(src []byte, opts Options) ([]byte, error) {
fset := token.NewFileSet()

// Ensure our parsed files never start with base 1,
// to ensure that using token.NoPos+1 will panic.
fset.AddFile("gofumpt_base.go", 1, 10)

file, err := parser.ParseFile(fset, "", src, parser.ParseComments)
if err != nil {
return nil, err
Expand Down Expand Up @@ -553,10 +558,14 @@ func (f *fumpter) applyPre(c *astutil.Cursor) {
// The body is preceded by a multi-line function
// signature, we move the `) {` to avoid the empty line.
switch {
case isThereAnEmptyLine && sign.Results != nil && !resultClosingIsFirstCharOnEndLine:
case isThereAnEmptyLine && sign.Results != nil &&
!resultClosingIsFirstCharOnEndLine &&
sign.Results.Closing.IsValid(): // there may be no ")"
sign.Results.Closing += 1
f.addNewline(sign.Results.Closing)
case isThereAnEmptyLine && sign.Params != nil && !paramClosingIsFirstCharOnEndLine:

case isThereAnEmptyLine && sign.Params != nil &&
!paramClosingIsFirstCharOnEndLine:
sign.Params.Closing += 1
f.addNewline(sign.Params.Closing)
}
Expand Down
4 changes: 4 additions & 0 deletions gofmt.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,10 @@ func main() {
}

func gofumptMain() {
// Ensure our parsed files never start with base 1,
// to ensure that using token.NoPos+1 will panic.
fileSet.AddFile("gofumpt_base.go", 1, 10)

flag.Usage = usage
flag.Parse()

Expand Down
18 changes: 18 additions & 0 deletions testdata/scripts/func-newlines.txt
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,15 @@ func multilineParamsWithoutEmptyLineWithComment(p1 string,
// comment
println("body")
}

// Same as the others above, but with a single result parameter without
// parentheses. This used to cause token.File.Offset crashes.
func f(p1 string,
p2 string) int {

println("body")
return 0
}
-- foo.go.golden --
package p

Expand Down Expand Up @@ -449,3 +458,12 @@ func multilineParamsWithoutEmptyLineWithComment(p1 string,
// comment
println("body")
}

// Same as the others above, but with a single result parameter without
// parentheses. This used to cause token.File.Offset crashes.
func f(p1 string,
p2 string,
) int {
println("body")
return 0
}

0 comments on commit 9d037d4

Please sign in to comment.