Skip to content
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

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

Closed
bradfitz opened this issue Mar 14, 2023 · 7 comments
Closed

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

bradfitz opened this issue Mar 14, 2023 · 7 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.22

Comments

@bradfitz
Copy link
Contributor

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
Copy link
Member

dsnet commented Mar 14, 2023 •
edited

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

bradfitz commented Mar 14, 2023 •
edited

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
Copy link
Member

dsnet commented Mar 14, 2023

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

@cherrymui cherrymui added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 14, 2023
@cherrymui cherrymui added this to the Backlog milestone Mar 14, 2023
@cherrymui
Copy link
Member

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

@dsnet
Copy link
Member

dsnet commented Mar 14, 2023

I can send out a fix.

@gopherbot
Copy link

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

@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Aug 19, 2023
cellularmitosis pushed a commit to cellularmitosis/go that referenced this issue Aug 24, 2023
@dsnet @cellularmitosis
expvar: emit valid JSON strings
318096b 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
5 participants
@bradfitz @dmitshur @dsnet @gopherbot @cherrymui