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.

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

x/perf: NaN values cause panics at the trend handler #28508

Open
kpacha opened this issue Oct 31, 2018 · 1 comment

Comments

@kpacha
Copy link

commented Oct 31, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.11.1

Does this issue reproduce with the latest release?

yes, it does

What operating system and processor architecture are you using (go env)?

darwin/amd64

What did you do?

Using the localperf cmd (golang.org/x/perf/analysis/localperf) when I tried to check the trends over the last commits, the server crashed with this panic message:

panic: json: unsupported value: NaN

It happens when the sets of benchmarks have different tests (benchmarks added or removed).

The easiest way to reproduce it is by extending the test golang.org/x/perf/analysis/app/trend_test.go

diff --git a/analysis/app/trend_test.go b/analysis/app/trend_test.go
index 8bf2157..140c6ed 100644
--- a/analysis/app/trend_test.go
+++ b/analysis/app/trend_test.go
@@ -17,12 +17,14 @@ func TestTableToJS(t *testing.T) {
                [][]string{
                        {"hello", "15.1"},
                        {"world", "20"},
+                       {"crash", "NaN"},
                }, true)
        have := tableToJS(in, []column{{Name: "text"}, {Name: "num"}})
        want := `{cols: [{"id":"text","type":"string","label":"text"},
 {"id":"num","type":"number","label":"num"}],
 rows: [{c:[{v: "hello"}, {v: 15.1}]},
-{c:[{v: "world"}, {v: 20}]}]}`
+{c:[{v: "world"}, {v: 20}]},
+{c:[{v: "crash"}, {v: null}]}]}`
        if d := diff.Diff(string(have), want); d != "" {
                t.Errorf("tableToJS returned wrong JS (- have, + want):\n%s", d)
        }
--- FAIL: TestTableToJS (0.00s)
panic: json: unsupported value: NaN [recovered]
	panic: json: unsupported value: NaN

goroutine 56 [running]:
testing.tRunner.func1(0xc000199600)
	/usr/local/opt/go/libexec/src/testing/testing.go:792 +0x387
panic(0x13fa3c0, 0xc000115290)
	/usr/local/opt/go/libexec/src/runtime/panic.go:513 +0x1b9
golang.org/x/perf/analysis/app.tableToJS(0xc0001150e0, 0xc0000b3f18, 0x2, 0x2, 0x3, 0x3)
	/Users/dani/go/src/golang.org/x/perf/analysis/app/trend.go:496 +0xbed
golang.org/x/perf/analysis/app.TestTableToJS(0xc000199600)
	/Users/dani/go/src/golang.org/x/perf/analysis/app/trend_test.go:22 +0x214
testing.tRunner(0xc000199600, 0x1494e20)
	/usr/local/opt/go/libexec/src/testing/testing.go:827 +0xbf
created by testing.(*T).Run
	/usr/local/opt/go/libexec/src/testing/testing.go:878 +0x353
exit status 2
FAIL	golang.org/x/perf/analysis/app	0.025s

What did you expect to see?

I guess a nil value could be ok

What did you see instead?

A panic

Comments

By the way, this dirty hack fixed the issue:

diff --git a/analysis/app/trend.go b/analysis/app/trend.go
index 88d88ab..87b309d 100644
--- a/analysis/app/trend.go
+++ b/analysis/app/trend.go
@@ -488,7 +488,11 @@ func tableToJS(t *table.Table, columns []column) template.JS {
                        case []int:
                                value, err = json.Marshal(column[i])
                        case []float64:
-                               value, err = json.Marshal(column[i])
+                               if math.IsNaN(column[i]) {
+                                       value, err = json.Marshal(nil)
+                               } else {
+                                       value, err = json.Marshal(column[i])
+                               }
                        default:
                                value = []byte(`"unknown column type"`)
                        }
@gopherbot gopherbot added this to the Unreleased milestone Oct 31, 2018
@kpacha

This comment has been minimized.

Copy link
Author

commented Mar 23, 2019

hi,

it looks like this issue did not catch the attention of the maintainers... neither the repo nor the public server has been fixed

I was very careful not to disclose already uploaded benchmarks that panic or even the URL that crashes, because I though it was obvious and it would just trigger some cheesy DoS attack, but I have some examples I could share with the assignee of this issue (if it gets one)

cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.