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

Jump to bottom

x/build: NaN values cause panics at the trend handler for perf.golang.org #28508

Closed
kpacha opened this issue Oct 31, 2018 · 2 comments
Closed

x/build: NaN values cause panics at the trend handler for perf.golang.org #28508

kpacha opened this issue Oct 31, 2018 · 2 comments
Labels
Builders x/build issues (builders, bots, dashboards) compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@kpacha
Copy link

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

kpacha commented Mar 23, 2019 •
edited

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

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 22, 2020
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 15, 2022
@mknyszek mknyszek changed the title x/perf: NaN values cause panics at the trend handler x/build: NaN values cause panics at the trend handler for perf.golang.org Dec 21, 2022
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Dec 21, 2022
@mknyszek
Copy link
Contributor

I believe we have evidence that nobody really uses the trend handler, so we're planning on just deleting it. We now have perf.golang.org/dashboard which is the replacement for that.

@mknyszek mknyszek closed this as not planned Won't fix, can't repro, duplicate, stale Dec 21, 2022
@golang golang locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@mknyszek @kpacha @ALTree @gopherbot