From 4cf59bb086058c82ecb364773545bc16c150f12b Mon Sep 17 00:00:00 2001 From: Aleksandr Nogikh Date: Thu, 7 Apr 2022 16:28:16 +0000 Subject: dashboard: repeat bug reproduction every 100 days This has two objectives: 1) Make repros better over time (as we update descriptions / improve syzlang / repro- and fuzzing algorithms), even for old bugs. 2) Constrain the impact of backward compatibility bugs in syzkaller. The fewer old bugs we have, the less likely it is to stumble on some problem during patch testing / bug or fix bisection. Restructure the needReproForBug and add tests. --- dashboard/app/api.go | 29 ++++++--- dashboard/app/repro_test.go | 140 ++++++++++++++++++++++++++++++++------------ 2 files changed, 122 insertions(+), 47 deletions(-) diff --git a/dashboard/app/api.go b/dashboard/app/api.go index e2ee8ab7a..778577584 100644 --- a/dashboard/app/api.go +++ b/dashboard/app/api.go @@ -62,6 +62,10 @@ type APINamespaceHandler func(c context.Context, ns string, r *http.Request, pay const ( maxReproPerBug = 10 reproRetryPeriod = 24 * time.Hour // try 1 repro per day until we have at least syz repro + // Attempt a new repro every ~ 3 months, even if we have already found it for the bug. This should: + // 1) Improve old repros over time (as we update descriptions / change syntax / repro algorithms). + // 2) Constrain the impact of bugs in syzkaller's backward compatibility. Fewer old repros, fewer problems. + reproStalePeriod = 100 * 24 * time.Hour ) // Overridable for testing. @@ -1246,14 +1250,23 @@ func needRepro(c context.Context, bug *Bug) bool { } func needReproForBug(c context.Context, bug *Bug) bool { - return bug.ReproLevel < ReproLevelC && - len(bug.Commits) == 0 && - bug.Title != corruptedReportTitle && - bug.Title != suppressedReportTitle && - config.Namespaces[bug.Namespace].NeedRepro(bug) && - (bug.NumRepro < maxReproPerBug || - bug.ReproLevel == ReproLevelNone && - timeSince(c, bug.LastReproTime) > reproRetryPeriod) + // We are already have fixing commits. + if len(bug.Commits) > 0 { + return false + } + if bug.Title == corruptedReportTitle || + bug.Title == suppressedReportTitle { + return false + } + if !config.Namespaces[bug.Namespace].NeedRepro(bug) { + return false + } + if bug.ReproLevel < ReproLevelC { + // We have not found a C repro yet, try until we do. + return bug.NumRepro < maxReproPerBug || timeSince(c, bug.LastReproTime) >= reproRetryPeriod + } + // When a C repro is already found, still do a repro attempt once in a while. + return timeSince(c, bug.LastReproTime) >= reproStalePeriod } func putText(c context.Context, ns, tag string, data []byte, dedup bool) (int64, error) { diff --git a/dashboard/app/repro_test.go b/dashboard/app/repro_test.go index 84a6b69ee..48a345be0 100644 --- a/dashboard/app/repro_test.go +++ b/dashboard/app/repro_test.go @@ -131,45 +131,6 @@ func TestNeedRepro3_dup(t *testing.T) { testNeedRepro3(t, dupCrash) } func TestNeedRepro3_closed(t *testing.T) { testNeedRepro3(t, closedCrash) } func TestNeedRepro3_closedRepro(t *testing.T) { testNeedRepro3(t, closedWithReproCrash) } -// Test that after uploading 5 syz repros, app stops requesting repros. -func testNeedRepro4(t *testing.T, crashCtor func(c *Ctx) *dashapi.Crash, newBug bool) { - c := NewCtx(t) - defer c.Close() - - crash1 := crashCtor(c) - crash1.ReproOpts = []byte("opts") - crash1.ReproSyz = []byte("repro syz") - for i := 0; i < maxReproPerBug-1; i++ { - resp, _ := c.client.ReportCrash(crash1) - c.expectEQ(resp.NeedRepro, true) - needRepro, _ := c.client.NeedRepro(testCrashID(crash1)) - c.expectEQ(needRepro, true) - } - - resp, _ := c.client.ReportCrash(crash1) - c.expectEQ(resp.NeedRepro, false) - needRepro, _ := c.client.NeedRepro(testCrashID(crash1)) - c.expectEQ(needRepro, false) - - // No more repros even after a day. - c.advanceTime(25 * time.Hour) - crash1.ReproOpts = nil - crash1.ReproSyz = nil - - resp, _ = c.client.ReportCrash(crash1) - c.expectEQ(resp.NeedRepro, false) - needRepro, _ = c.client.NeedRepro(testCrashID(crash1)) - c.expectEQ(needRepro, false) - if newBug { - c.client.pollBug() - } -} - -func TestNeedRepro4_normal(t *testing.T) { testNeedRepro4(t, normalCrash, true) } -func TestNeedRepro4_dup(t *testing.T) { testNeedRepro4(t, dupCrash, false) } -func TestNeedRepro4_closed(t *testing.T) { testNeedRepro4(t, closedCrash, true) } -func TestNeedRepro4_closedRepro(t *testing.T) { testNeedRepro4(t, closedWithReproCrash, true) } - func normalCrash(c *Ctx) *dashapi.Crash { build := testBuild(1) c.client.UploadBuild(build) @@ -237,3 +198,104 @@ func TestNeedReproMissing(t *testing.T) { c.expectEQ(err, nil) c.expectEQ(needRepro, true) } + +// In addition to the above, do a number of quick tests of the needReproForBug function. +func TestNeedReproIsolated(t *testing.T) { + c := NewCtx(t) + defer c.Close() + + nowTime := c.mockedTime + tests := []struct { + bug *Bug + needRepro bool + }{ + { + // A bug without a repro. + bug: &Bug{ + Title: "normal bug without a repro", + }, + needRepro: true, + }, + { + // Corrupted bug. + bug: &Bug{ + Title: corruptedReportTitle, + }, + needRepro: false, + }, + { + // Suppressed bug. + bug: &Bug{ + Title: suppressedReportTitle, + }, + needRepro: false, + }, + { + // A bug without a C repro. + bug: &Bug{ + Title: "some normal bug with a syz-repro", + ReproLevel: ReproLevelSyz, + }, + needRepro: true, + }, + { + // A bug for which we have recently found a repro. + bug: &Bug{ + Title: "some normal recent bug", + ReproLevel: ReproLevelC, + LastReproTime: nowTime.Add(-time.Hour * 24), + }, + needRepro: false, + }, + { + // A bug which has an old C repro. + bug: &Bug{ + Title: "some normal bug with old repro", + ReproLevel: ReproLevelC, + NumRepro: 2 * maxReproPerBug, + LastReproTime: nowTime.Add(-reproStalePeriod), + }, + needRepro: true, + }, + { + // Several failed repro attepts are OK. + bug: &Bug{ + Title: "some normal bug with several fails", + NumRepro: maxReproPerBug - 1, + LastReproTime: nowTime, + }, + needRepro: true, + }, + { + // ... but there are limits. + bug: &Bug{ + Title: "some normal bug with too much fails", + NumRepro: maxReproPerBug, + LastReproTime: nowTime, + }, + needRepro: false, + }, + { + // Make sure we try until we find a C repro, not just a syz repro. + bug: &Bug{ + Title: "too much fails, but only a syz repro", + ReproLevel: ReproLevelSyz, + NumRepro: maxReproPerBug, + LastReproTime: nowTime.Add(-time.Hour * 24), + }, + needRepro: true, + }, + } + + for _, test := range tests { + bug := test.bug + if bug.Namespace == "" { + bug.Namespace = "test1" + } + funcResult := needReproForBug(c.ctx, bug) + if funcResult != test.needRepro { + t.Errorf("For %#v expected needRepro=%v, got needRepro=%v", + bug, test.needRepro, funcResult) + } + } +} -- cgit mrf-deployment