github-vet / github-vet/rangeloop-pointer-findings

istio/bots: policybot/mgrs/syncmgr/mgr.go; 101 LoC

Open
#18,161 0 comments 0 reactions 0 assignees View on GitHub
fresh large
Dominant language
No language data
Stars
0
Forks
0
PR merge metrics
PR metrics pending

Description

Found a possible issue in [istio/bots](https://www.github.com/istio/bots) at [policybot/mgrs/syncmgr/mgr.go](https://github.com/istio/bots/blob/9fa3f10e5af8aad5fed6a8f9e47fbbbd6783c374/policybot/mgrs/syncmgr/mgr.go#L1008-L1108)

Below is the message reported by the analyzer for this snippet of code. Beware that the analyzer only reports the first issue it finds, so please do not limit your consideration to the contents of the below message.

> range-loop variable repo used in defer or goroutine at line 1033

[Click here to see the code in its original context.](https://github.com/istio/bots/blob/9fa3f10e5af8aad5fed6a8f9e47fbbbd6783c374/policybot/mgrs/syncmgr/mgr.go#L1008-L1108)

Click here to show the 101 line(s) of Go which triggered the analyzer.

```go
for _, repo := range ss.mgr.reg.Repos() {
r, ok := ss.mgr.reg.SingleRecord(refresher.RecordType, repo.OrgAndRepo)
if !ok {
continue
}

tor := r.(*refresher.TestOutputRecord)
g := resultgatherer.TestResultGatherer{
Client: ss.mgr.blobstore,
BucketName: tor.BucketName,
PreSubmitPrefix: tor.PreSubmitTestPath,
PostSubmitPrefix: tor.PostSubmitTestPath,
}

scope.Debugf("Getting test results for org %s", repo.OrgLogin)
prMin := env.RegisterIntVar("PR_MIN", 0, "The minimum PR to scan for test results").Get()
prMax := env.RegisterIntVar("PR_MAX", -1, "The maximum PR to scan for test results").Get()

var completedTests = make(map[string]bool)
ctLock := sync.RWMutex{}
wg := sync.WaitGroup{}
wg.Add(1)
go func() {
ctLock.Lock()
defer ctLock.Unlock()
err := ss.mgr.store.QueryTestResultByDone(ss.ctx, repo.OrgLogin, repo.RepoName,
func(result *storage.TestResult) error {
completedTests[result.RunPath] = true
return nil
})
if err != nil {
scope.Warnf("Unable to fetch previous tests: %s", err)
}
wg.Done()
}()
prPaths := g.GetAllPullRequestsChan(ss.ctx, repo.OrgLogin, repo.RepoName).WithBuffer(100)
// I think a composition syntax would be better here...
errorChan := prPaths.Transform(func(prPathi interface{}) (prNum interface{}, err error) {
prPath := prPathi.(string)
prParts := strings.Split(prPath, "/")
if len(prParts) < 2 {
err = errors.New("too few segments in pr path")
return
}
prNum = prParts[len(prParts)-2]
if prInt, ierr := strconv.Atoi(prParts[len(prParts)-2]); ierr == nil {
// skip this PR if it's outside the min and max inclusive
if prInt < prMin || (prMax > -1 && prInt > prMax) {
err = pipeline.ErrSkip
}
}
return
}).WithContext(ss.ctx).OnError(func(e error) {
// TODO: this should probably be reported out or something...
scope.Warnf("error processing test: %s", e)
}).WithParallelism(50).Transform(func(prNumi interface{}) (testRunPaths interface{}, err error) {
prNum := prNumi.(string)
tests, err := g.GetTestsForPR(ss.ctx, repo.OrgLogin, repo.RepoName, prNum)
var result [][]string

// Wait for a comprehensive list of completed tests
wg.Wait()
ctLock.RLock()
defer ctLock.RUnlock()

for testName, runPaths := range tests {
for _, runPath := range runPaths {
if _, ok := completedTests[runPath]; !ok {
result = append(result, []string{testName, runPath})
}
}
}
testRunPaths = result
return
}).Expand().Transform(func(testRunPathi interface{}) (i interface{}, err error) {
inputArray := testRunPathi.([]string)
testRunPath := inputArray[1]
testName := inputArray[0]
if strings.Contains(testRunPath, "00") {
fmt.Printf("checking test %s\n", testRunPath)
}
return g.GetTestResult(ss.ctx, testName, testRunPath, repo.OrgLogin)
}).Batch(50).To(func(input interface{}) error {
var testResults []*storage.TestResult
for _, i := range input.([]interface{}) {
singleResult := i.(*storage.TestResult)
singleResult.OrgLogin = repo.OrgLogin
singleResult.RepoName = repo.RepoName
testResults = append(testResults, singleResult)
}
fmt.Printf("saving TestResult batch of size %d\n", len(testResults))
err := ss.mgr.store.WriteTestResults(ss.ctx, testResults)
if err != nil {
return err
}
return nil
}).WithParallelism(1).Go()
for err := range errorChan {
result = multierror.Append(result, err.Err())
}
}

```

Leave a reaction on this issue to contribute to the project by classifying this instance as a **Bug** :-1:, **Mitigated** :+1:, or **Desirable Behavior** :rocket:
See the descriptions of the classifications [here](https://github.com/github-vet/rangeclosure-findings#how-can-i-help) for more information.

commit ID: 9fa3f10e5af8aad5fed6a8f9e47fbbbd6783c374

Contributor guide

No contributing guide indexed for this repository

Research direction

Open policybot/mgrs/syncmgr/mgr.go around lines 1008-1108 and review the reported range-loop variable capture in the goroutine. Compare the finding with the surrounding code, then leave a reaction classifying it as Bug, Mitigated, or Desirable Behavior; completion is the submitted classification.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend
Issue type
Bug
Difficulty
1/5
Estimated time
Under an hour
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.