expvar: Map.String can make invalid JSON with low ASCII bytes

[bradfitz]

Go 1.20:

https://go.dev/play/p/b0A7ivMg0wn

func main() {
	m := expvar.NewMap("foo")
	m.Add("\x01", 1)
	fmt.Println(m.String())

	out := map[string]int64{}
	err := json.Unmarshal([]byte(m.String()), &out)
	if err != nil {
		log.Fatal(err)
	}
}

=>

{"\x01": 1}
2009/11/10 23:00:00 invalid character 'x' in string escape code

The (*expvar.Map).String method is implemented using fmt %q to print strings:

func (v *Map) String() string {
	var b strings.Builder
	fmt.Fprintf(&b, "{")
	first := true
	v.Do(func(kv KeyValue) {
		if !first {
			fmt.Fprintf(&b, ", ")
		}
		fmt.Fprintf(&b, "%q: ", kv.Key)
		if kv.Value != nil {
			fmt.Fprintf(&b, "%v", kv.Value)
		} else {
			fmt.Fprint(&b, "null")
		}
		first = false
	})
	fmt.Fprintf(&b, "}")
	return b.String()
}

But Go's %q doesn't make valid JSON when there are bytes under 0x20.

/cc @dsnet @maisem

[dsnet]

This is related to a wider set of issues where the MarshalJSON and UnmarshalJSON methods of certain types in stdlib (e.g., #47353) do not properly handle JSON strings. Most of these packages cannot depend on "encoding/json" without bringing in a transitive dependency on "reflect" and "unicode".

Fortunately, the cost of importing "unicode" has dramatically dropped with the work of @thanm and @cherrymui on dead-code elimination of global maps in https://go.dev/cl/461315.

It may be worth pulling out basic JSON string formatting and parsing logic as an internal package.

Setting that aside, is this the fault of expvar.Map.String? The method doesn't document any guarantees that the format is JSON.

[bradfitz]

@dsnet, https://pkg.go.dev/expvar#Var

type Var interface {
	// String returns a valid JSON value for the variable.
	// Types with String methods that do not return valid JSON
	// (such as time.Time) must not be used as a Var.
	String() string
}

It uses that to make JSON for HTTP:

Package expvar provides a standardized interface to public variables, such as operation counters in servers. It exposes these variables via HTTP at /debug/vars in JSON format.

[bradfitz]

Also, expvar already depends on encoding/json and uses it for

https://cs.opensource.google/go/go/+/refs/tags/go1.20.2:src/expvar/expvar.go;l=249

// String implements the Var interface. To get the unquoted string
// use Value.
func (v *String) String() string {
	s := v.Value()
	b, _ := json.Marshal(s)
	return string(b)
}

So we could do the same in Map.

The extra allocs are sad, though.

Maybe we can pool a json.Encoder that writes to an io.Writer type that we can indirect to write through to our StringBuilder.

Or only call json.Marshal on sketchy looking strings.

[dsnet]

Huh. Fail, I forgot to look at expvar.Var itself and only looked at expvar.Map.String.

[cherrymui]

@bradfitz you're the only owner of the expvar package. Would you like to send a fix? Thanks.

[dsnet]

I can send out a fix.

[gopherbot]

Change https://go.dev/cl/476336 mentions this issue: expvar: emit valid JSON strings