go: flag: PrintDefaults panics trying to ascertain whether a flag was set

(*flag.FlagSet).PrintDefaults assumes that it is safe to call .String() on the zero value of a flag type, or if that flag type is a pointer, a new instance of the variable. Consequently, a program that uses indirect data structures in its flags will panic when a user sets the -h flag.

$ go version
go version devel +7da1f7addf Thu Nov 8 08:19:48 2018 +0000 linux/amd64
$ cat a.go
package main

import "flag"

func main() {
        f := &myFlag{x: new(int)}
        flag.CommandLine.Var(f, "f", "usage")
        flag.CommandLine.PrintDefaults()
}

type myFlag struct {
        x *int
}

func (f *myFlag) Set(s string) error { return nil }
func (f *myFlag) String() string     { _ = *f.x; return "" }

$ go run a.go
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x491878]

goroutine 1 [running]:
main.(*myFlag).String(0xc00007c030, 0x4ab080, 0xc00007c030)
        /usr/local/google/home/adonovan/got/src/golang.org/x/tools/a.go:16 +0x8
flag.isZeroValue(0xc000086000, 0x0, 0x0, 0x4c49e4)
        /usr/local/google/home/adonovan/goroot/src/flag/flag.go:457 +0xff
flag.(*FlagSet).PrintDefaults.func1(0xc000086000)
        /usr/local/google/home/adonovan/goroot/src/flag/flag.go:520 +0x202
flag.(*FlagSet).VisitAll(0xc00007e120, 0xc000092f28)
        /usr/local/google/home/adonovan/goroot/src/flag/flag.go:387 +0x61
flag.(*FlagSet).PrintDefaults(0xc00007e120)
        /usr/local/google/home/adonovan/goroot/src/flag/flag.go:503 +0x4e
main.main()
        /usr/local/google/home/adonovan/got/src/golang.org/x/tools/a.go:8 +0xbc
exit status 2

I don’t think the flag package should be making assumptions about the concrete representation of the flag. That’s the point of the flag.Value interface.

About this issue

Original URL
State: closed
Created 6 years ago
Reactions: 2
Comments: 25 (19 by maintainers)

Most upvoted comments

I did a sketch of my simplification of @bcmills’ check in FlagSet.Var: https://play.golang.org/p/Jsl2nSaJN8U

I’m guessing it would be roughly the same LOC as the current fix but it would be more reliable and less surprising.

jimmyfrasche on Nov 20, 2018

@jimmyfrasche, or we could record whether the default value is the zero value of the type when we first set DefValue, rather than trying to infer it after-the-fact.

(We could even do that without changing the definition of *flag.Flag, by either storing it in a parallel map or wrapping the Flag in a larger, unexported struct.)

bcmills on Nov 19, 2018