Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions pkg/email/lore/parse.go
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be tested.

Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@ type Thread struct {

// Series represents a single patch series sent over email.
type Series struct {
Subject string
MessageID string
Version int
Corrupted string // If non-empty, contains a reason why the series better be ignored.
Tags []string
Patches []Patch
Subject string
MessageID string
Version int
Corrupted string // If non-empty, contains a reason why the series better be ignored.
Tags []string
Patches []Patch
BaseCommitHint string
}

type Patch struct {
Expand Down Expand Up @@ -88,6 +89,13 @@ func PatchSeries(emails []*Email) []*Series {
if !ok {
continue
}
if series.BaseCommitHint == "" { // Usually base-commit is in patch 0 or 1. Check them all to be safe.
regex := regexp.MustCompile(`(?m)^base-commit:\s*([0-9a-fA-F]{40})$`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't compile the regexp for each message. Ideally it should be done once outside of the function, see the majority of other regexp.MustCompile cases.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also move this extraction to pkg/email.

matches := regex.FindStringSubmatch(email.Body)
if len(matches) >= 2 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just check for nil.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid an unnecessary crash if the author wrote the tag and forgot the hash. But I just realized it wouldn't match in that case! Will update.

series.BaseCommitHint = matches[1]
}
}
seq := patch.Seq.ValueOr(1)
if seq == 0 {
// The cover email is not of interest.
Expand Down
21 changes: 11 additions & 10 deletions syz-cluster/pkg/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,16 +127,17 @@ type NewFinding struct {
}

type Series struct {
ID string `json:"id"` // Only included in the reply.
ExtID string `json:"ext_id"`
Title string `json:"title"`
AuthorEmail string `json:"author_email"`
Cc []string `json:"cc"`
Version int `json:"version"`
Link string `json:"link"`
SubjectTags []string `json:"subject_tags"`
PublishedAt time.Time `json:"published_at"`
Patches []SeriesPatch `json:"patches"`
ID string `json:"id"` // Only included in the reply.
ExtID string `json:"ext_id"`
Title string `json:"title"`
AuthorEmail string `json:"author_email"`
Cc []string `json:"cc"`
Version int `json:"version"`
Link string `json:"link"`
SubjectTags []string `json:"subject_tags"`
PublishedAt time.Time `json:"published_at"`
Patches []SeriesPatch `json:"patches"`
BaseCommitHint string `json:"base_commit"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: let's keep json name similar to the field name.

}

func (s *Series) PatchBodies() [][]byte {
Expand Down
15 changes: 8 additions & 7 deletions syz-cluster/pkg/db/entities.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@ import (
)

type Series struct {
ID string `spanner:"ID"`
ExtID string `spanner:"ExtID"`
AuthorName string `spanner:"AuthorName"`
AuthorEmail string `spanner:"AuthorEmail"`
Title string `spanner:"Title"`
Link string `spanner:"Link"`
Version int64 `spanner:"Version"`
ID string `spanner:"ID"`
ExtID string `spanner:"ExtID"`
AuthorName string `spanner:"AuthorName"`
AuthorEmail string `spanner:"AuthorEmail"`
Title string `spanner:"Title"`
Link string `spanner:"Link"`
Version int64 `spanner:"Version"`
BaseCommitHint string `spanner:"BaseCommitHint"`
// In LKML patches, there are often hints at the target tree for the patch.
SubjectTags []string `spanner:"SubjectTags"`
PublishedAt time.Time `spanner:"PublishedAt"`
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE Series DROP COLUMN BaseCommitHint;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE Series ADD COLUMN BaseCommitHint STRING(256);
36 changes: 19 additions & 17 deletions syz-cluster/pkg/service/series.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,15 @@ func (s *SeriesService) getSessionSeries(ctx context.Context, sessionID string,

func (s *SeriesService) UploadSeries(ctx context.Context, series *api.Series) (*api.UploadSeriesResp, error) {
seriesObj := &db.Series{
ID: uuid.NewString(),
ExtID: series.ExtID,
AuthorEmail: series.AuthorEmail,
Title: series.Title,
Version: int64(series.Version),
Link: series.Link,
PublishedAt: series.PublishedAt,
Cc: series.Cc,
ID: uuid.NewString(),
ExtID: series.ExtID,
AuthorEmail: series.AuthorEmail,
Title: series.Title,
Version: int64(series.Version),
Link: series.Link,
PublishedAt: series.PublishedAt,
Cc: series.Cc,
BaseCommitHint: series.BaseCommitHint,
}
for _, tag := range series.SubjectTags {
const tageSizeLimit = 511
Expand Down Expand Up @@ -120,15 +121,16 @@ func (s *SeriesService) getSeries(ctx context.Context,
return nil, fmt.Errorf("failed to fetch patches: %w", err)
}
ret := &api.Series{
ID: series.ID,
ExtID: series.ExtID,
Title: series.Title,
AuthorEmail: series.AuthorEmail,
Version: int(series.Version),
Cc: series.Cc,
PublishedAt: series.PublishedAt,
Link: series.Link,
SubjectTags: series.SubjectTags,
ID: series.ID,
ExtID: series.ExtID,
Title: series.Title,
AuthorEmail: series.AuthorEmail,
Version: int(series.Version),
Cc: series.Cc,
PublishedAt: series.PublishedAt,
Link: series.Link,
SubjectTags: series.SubjectTags,
BaseCommitHint: series.BaseCommitHint,
}
for _, patch := range patches {
var body []byte
Expand Down
12 changes: 12 additions & 0 deletions syz-cluster/pkg/triage/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,15 @@ func (cs *CommitSelector) Select(series *api.Series, tree *api.Tree, lastBuild *
}
return SelectResult{Reason: reasonNotApplies}, nil
}

func (cs *CommitSelector) TrySelectWithHint(series *api.Series) (SelectResult, error) {
if series.BaseCommitHint == "" {
return SelectResult{}, nil
}
var git GitTreeOps
baseCommit, err := git.Git.Commit(series.BaseCommitHint)
if err != nil {
return SelectResult{}, err
}
return SelectResult{Commit: baseCommit.Hash}, nil
}
15 changes: 8 additions & 7 deletions syz-cluster/series-tracker/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,14 @@ func (sf *SeriesFetcher) handleSeries(ctx context.Context, series *lore.Series,
date = time.Now()
}
apiSeries := &api.Series{
ExtID: series.MessageID,
AuthorEmail: first.Author,
Title: series.Subject,
Version: series.Version,
SubjectTags: series.Tags,
Link: loreLink(series.MessageID),
PublishedAt: date,
ExtID: series.MessageID,
AuthorEmail: first.Author,
Title: series.Subject,
Version: series.Version,
SubjectTags: series.Tags,
Link: loreLink(series.MessageID),
PublishedAt: date,
BaseCommitHint: series.BaseCommitHint,
}
sp := seriesProcessor{}
for i, patch := range series.Patches {
Expand Down
52 changes: 37 additions & 15 deletions syz-cluster/workflow/triage-step/main.go
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase it on top of the current master.

Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,24 @@ func (triager *seriesTriager) GetVerdict(ctx context.Context, sessionID string)
func (triager *seriesTriager) prepareFuzzingTask(ctx context.Context, series *api.Series, trees []*api.Tree,
target *triage.MergedFuzzConfig) (*api.FuzzTask, error) {
var skipErr error
selector := triage.NewCommitSelector(triager.ops, triager.DebugTracer)

// First try to use hints from the series description.
if series.BaseCommitHint != "" {
for _, tree := range trees {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we should be able to determine the tree from the commit:

  • Given a commit, we can query the branches that have it.
  • Branches are prefixed by tree names (that's the way syz-cluster pulls them).

You can use #6553 as reference here.

triager.Log("considering tree %q with a hint", tree.Name)
result, err := selector.TrySelectWithHint(series)
if err != nil {
return nil, fmt.Errorf("failed to run TrySelectWithHint for %q: %w", tree.Name, err)
}
if result.Commit != "" {
triager.Log("selected base commit with hint: %s", result.Commit)
return buildFuzzTask(series, tree, target, result.Commit), nil
}
triager.Log("failed to find a base commit with hint for %q: %s", tree.Name, result.Reason)
}
}

for _, tree := range trees {
triager.Log("considering tree %q", tree.Name)
arch := "amd64"
Expand All @@ -127,7 +145,6 @@ func (triager *seriesTriager) prepareFuzzingTask(ctx context.Context, series *ap
return nil, fmt.Errorf("failed to query the last build for %q: %w", tree.Name, err)
}
triager.Log("%q's last build: %q", tree.Name, lastBuild)
selector := triage.NewCommitSelector(triager.ops, triager.DebugTracer)
result, err := selector.Select(series, tree, lastBuild)
if err != nil {
// TODO: the workflow step must be retried.
Expand All @@ -141,24 +158,29 @@ func (triager *seriesTriager) prepareFuzzingTask(ctx context.Context, series *ap
continue
}
triager.Log("selected base commit: %s", result.Commit)
base := api.BuildRequest{
TreeName: tree.Name,
TreeURL: tree.URL,
ConfigName: target.KernelConfig,
CommitHash: result.Commit,
Arch: arch,
}
fuzz := &api.FuzzTask{
Base: base,
Patched: base,
FuzzConfig: *target.FuzzConfig,
}
fuzz.Patched.SeriesID = series.ID
return fuzz, nil
return buildFuzzTask(series, tree, target, result.Commit), nil
}
return nil, skipErr
}

func buildFuzzTask(series *api.Series, tree *api.Tree, target *triage.MergedFuzzConfig, commit string) *api.FuzzTask {
arch := "amd64"
base := api.BuildRequest{
TreeName: tree.Name,
TreeURL: tree.URL,
ConfigName: target.KernelConfig,
CommitHash: commit,
Arch: arch,
}
fuzz := &api.FuzzTask{
Base: base,
Patched: base,
FuzzConfig: *target.FuzzConfig,
}
fuzz.Patched.SeriesID = series.ID
return fuzz
}

type SkipTriageError struct {
Reason error
}
Expand Down
Loading