-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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.
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
expvar: Map.String can make invalid JSON with low ASCII bytes #59040
Comments
This is related to a wider set of issues where the 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 |
@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:
|
Also, 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. |
Huh. Fail, I forgot to look at |
@bradfitz you're the only owner of the |
I can send out a fix. |
Change https://go.dev/cl/476336 mentions this issue: |
Map.String and expvarHandler used the %q flag with fmt.Fprintf to escape Go strings, which does so according to the Go grammar, which is not always compatible with JSON strings. Rather than calling json.Marshal for every string, which will always allocate, declare a local appendJSONQuote function that does basic string escaping. Also, we declare an unexported appendJSON method on every concrete Var type so that the final JSON output can be constructed with far fewer allocations. The resulting logic is both more correct and also much faster. This does not alter the whitespace style of Map.String or expvarHandler, but may alter the representation of JSON strings. Performance: name old time/op new time/op delta MapString 5.10µs ± 1% 1.56µs ± 1% -69.33% (p=0.000 n=10+9) name old alloc/op new alloc/op delta MapString 1.21kB ± 0% 0.66kB ± 0% -45.12% (p=0.000 n=10+10) name old allocs/op new allocs/op delta MapString 37.0 ± 0% 7.0 ± 0% -81.08% (p=0.000 n=10+10) Fixes golang#59040 Change-Id: I46a2125f43550b91d52019e5edc003d9dd19590f Reviewed-on: https://go-review.googlesource.com/c/go/+/476336 Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
Go 1.20:
https://go.dev/play/p/b0A7ivMg0wn
=>
The
(*expvar.Map).String
method is implemented using fmt %q to print strings:But Go's %q doesn't make valid JSON when there are bytes under 0x20.
/cc @dsnet @maisem
The text was updated successfully, but these errors were encountered: