ci: fix semgrep warnings and failures across all code
Open
Nobody has claimed this yet.
stage/accepted
theme/ci
type/bug
- Dominant language
- Go
- Stars
- 17k
- Forks
- 2.1k
- Avg merge
- 1d 9h
- Merged PRs (30d)
- 105
Description
The semgrep rules we have are triggered on each PR raised, however, they are only ran on the files which have changed as part of the PR. When running the ruleset locally, a number of problems are found. These should be fixed, so that the code is both better, and that we can run semgrep across all files for CI.
Semgrep Results:
$ semgrep --config=.semgrep/ ./
┌─────────────┐
│ Scan Status │
└─────────────┘
Scanning 5233 files tracked by git with 17 Code rules:
Language Rules Files Origin Rules
───────────────────────────── ─────────────────
<multilang> 2 2580 Unknown 17
go 8 1780
js 1 904
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:03
┌──────────────────┐
│ 73 Code Findings │
└──────────────────┘
client/allocrunner/alloc_runner_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
2086┆ t.Parallel()
client/allocrunner/consul_grpc_sock_hook.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
251┆ select {
252┆ case <-p.doneCh:
253┆ return
nil
254┆ case <-time.After(socketProxyStopWaitTime):
255┆ return
errSocketProxyTimeout
client/allocrunner/consul_http_sock_hook.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
197┆ select {
198┆ case <-p.doneCh:
199┆ case <-time.After(socketProxyStopWaitTime):
200┆ return
errSocketProxyTimeout
client/allocrunner/health_hook.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
238┆ select {
239┆ case <-ctx.Done():
240┆ // Graceful
shutdown
241┆
return
242┆
243┆ case <-tracker.AllocStoppedCh():
244┆ // Allocation has stopped so no need to set
health
245┆
return
246┆
247┆ case <-time.After(time.Until(deadline)):
[hid 4 additional lines, adjust with --max-lines-per-finding]
client/allocrunner/taskrunner/artifact_hook_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
167┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
255┆ t.Parallel()
client/allocrunner/taskrunner/lazy_handle.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
101┆ select {
102┆ case <-l.shutdownCtx.Done():
103┆ return nil,
l.shutdownCtx.Err()
104┆ case <-time.After(backoff):
client/allocrunner/taskrunner/script_check_hook.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
419┆ select {
420┆ case <-ctx.Done():
421┆ return
err
422┆ case <-time.After(backoff):
client/allocrunner/taskrunner/sids_hook.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
258┆ select {
259┆ case <-ctx.Done():
260┆ return
false
261┆ case <-time.After(next):
262┆ return
true
client/allocrunner/taskrunner/stats_hook.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
109┆ select {
110┆ case <-time.After(h.interval):
111┆ goto
MAIN
112┆ case <-ctx.Done():
113┆
return
client/allocrunner/taskrunner/task_runner.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
1003┆ select {
1004┆ case result := <-resultCh:
1005┆ return
result
1006┆ case <-tr.shutdownDelayCtx.Done():
1007┆
break
1008┆ case <-time.After(delay):
⋮┆----------------------------------------
1084┆ select {
1085┆ case result := <-resultCh:
1086┆ return result,
nil
1087┆ case <-time.After(backoff):
client/allocrunner/taskrunner/template/template.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
480┆ select {
481┆ case <-time.After(t):
482┆ case <-tm.shutdownCh:
483┆
return
client/allocrunner/taskrunner/vault_hook.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
350┆ select {
351┆ case <-h.ctx.Done():
352┆ return "",
true
353┆ case <-time.After(backoff):
client/allocwatcher/alloc_watcher.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
377┆ select {
378┆ case <-time.After(retry):
379┆
continue
380┆ case <-ctx.Done():
381┆ return
ctx.Err()
⋮┆----------------------------------------
467┆ select {
468┆ case <-time.After(retry):
469┆
continue
470┆ case <-ctx.Done():
471┆ return "",
ctx.Err()
client/client.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
578┆ select {
579┆ case <-c.fpInitialized:
580┆ case <-time.After(batchFirstFingerprintsProcessingGrace):
581┆ logger.Warn("batch fingerprint operation timed out; proceeding to register with fingerprinted
plugins so far")
⋮┆----------------------------------------
1989┆ select {
1990┆ case <-c.rpcRetryWatcher():
1991┆ case <-time.After(c.retryIntv(retryIntv)):
1992┆ case <-c.shutdownCh:
1993┆
return
⋮┆----------------------------------------
2346┆ select {
2347┆ case <-c.rpcRetryWatcher():
2348┆
continue
2349┆ case <-time.After(retry):
2350┆
continue
2351┆ case <-c.shutdownCh:
2352┆
return
⋮┆----------------------------------------
2408┆ select {
2409┆ case <-c.rpcRetryWatcher():
2410┆
continue
2411┆ case <-time.After(retry):
2412┆
continue
2413┆ case <-c.shutdownCh:
2414┆
return
⋮┆----------------------------------------
2433┆ select {
2434┆ case <-time.After(wait):
2435┆ // Wait for the server we contact to receive
the
2436┆ //
allocations
2437┆ continue
OUTER
2438┆ case <-c.shutdownCh:
2439┆
return
client/client_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
1979┆ t.Parallel()
client/devicemanager/instance.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
505┆ select {
506┆ case <-i.ctx.Done():
507┆
return
508┆ case <-time.After(backoff):
509┆ goto
START
client/logmon/logmon.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
265┆ select {
266┆ case <-l.hasFinishedCopied:
267┆ case <-time.After(processOutputCloseTolerance):
⋮┆----------------------------------------
292┆ select {
293┆ case <-closeDone:
294┆ case <-time.After(processOutputCloseTolerance):
295┆ l.logger.Warn("timed out waiting for read-side of process output pipe to
close")
client/pluginmanager/drivermanager/instance.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
297┆ select {
298┆ case <-time.After(backoff):
299┆ case <-i.ctx.Done():
300┆
cancel()
301┆
return
⋮┆----------------------------------------
429┆ select {
430┆ case <-time.After(backoff):
431┆ case <-i.ctx.Done():
432┆
cancel()
433┆
return
client/widmgr/widmgr_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
21┆ t.Parallel()
command/agent/command.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
981┆ select {
982┆ case <-signalCh:
983┆ return
1
984┆ case <-time.After(gracefulTimeout):
985┆ return
1
986┆ case <-gracefulCh:
987┆ return
0
command/agent/pprof/pprof.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
136┆ select {
137┆ case <-time.After(d):
138┆ case <-ctx.Done():
command/tls_cert_create_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
19┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
47┆ t.Parallel()
drivers/docker/coordinator.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
304┆ select {
305┆ case <-ctx.Done():
306┆ // We have been
cancelled
307┆
return
308┆ case <-time.After(d.removeDelay):
⋮┆----------------------------------------
345┆ select {
346┆ case <-ctx.Done():
347┆ // We have been
cancelled
348┆
return
349┆ case <-time.After(3 * time.Second):
drivers/docker/driver_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
3073┆ t.Parallel()
drivers/mock/command.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
84┆ select {
85┆ case <-cancelCh:
86┆ logger.Warn("exiting before done writing output", "i", i, "total",
outputRepeat)
87┆
return
88┆ case <-time.After(repeatDuration):
89┆ if _, err := io.WriteString(writer, output); err != nil
{
90┆ logger.Error("failed to write to stdout",
"error", err)
91┆
errCh <- err
92┆
return
93┆
}
drivers/mock/driver.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
558┆ select {
559┆ case <-h.waitCh:
560┆ d.logger.Debug("not killing task: already exited", "task_name",
h.taskConfig.Name)
561┆ case <-time.After(h.killAfter):
562┆ d.logger.Debug("killing task due to kill_after", "task_name",
h.taskConfig.Name)
563┆
h.kill()
drivers/shared/eventer/eventer.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
102┆ select {
103┆ case <-time.After(consumer.timeout):
104┆ filtered = append(filtered,
consumer)
105┆ e.logger.Warn("timeout sending event", "task_id", event.TaskID, "message",
event.Message)
106┆ case <-consumer.ctx.Done():
107┆ // consumer context finished, filtering it out of
loop
108┆
close(consumer.ch)
109┆ case consumer.ch <- event:
110┆ filtered = append(filtered,
consumer)
drivers/shared/executor/executor.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
577┆ select {
578┆ case <-e.processExited:
579┆ case <-time.After(grace):
580┆
proc.Kill()
⋮┆----------------------------------------
592┆ select {
593┆ case <-e.processExited:
594┆ case <-time.After(time.Second * 15):
595┆ e.logger.Warn("process did not exit after 15
seconds")
596┆ merr.Errors = append(merr.Errors, fmt.Errorf("process did not exit after 15
seconds"))
drivers/shared/executor/executor_linux.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
276┆ select {
277┆ case <-l.userProcExited:
278┆ return
nil
279┆ case <-time.After(grace):
280┆ // Force kill all container processes after grace
period,
281┆ // hence `true`
argument.
282┆ if err := l.container.Signal(os.Kill, true); err != nil
{
283┆
return err
284┆
}
⋮┆----------------------------------------
294┆ select {
295┆ case <-l.userProcExited:
296┆ return
nil
297┆ case <-time.After(time.Second * 15):
298┆ return fmt.Errorf("process failed to exit after 15
seconds")
⋮┆----------------------------------------
433┆ select {
434┆ case result := <-waitCh:
435┆ ps :=
result.ps
436┆ if result.err != nil
{
437┆ if exitErr, ok :=
result.err.(*exec.ExitError); ok {
438┆ ps =
exitErr.ProcessState
439┆
} else {
440┆ return
nil, 0, result.err
441┆
}
442┆
}
[hid 9 additional lines, adjust with --max-lines-per-finding]
helper/pluginutils/grpcutils/utils.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
36┆ select {
37┆ case <-pluginCtx.Done():
38┆ err =
structs.ErrPluginShutdown
39┆ case <-reqCtx.Done():
40┆ err =
reqCtx.Err()
41┆
42┆ // There is no guarantee that the select will choose
the
43┆ // doneCtx first so we have to double
check
44┆ select
{
45┆ case
<-pluginCtx.Done():
[hid 4 additional lines, adjust with --max-lines-per-finding]
94┆ select {
95┆ case <-pluginCtx.Done():
96┆ err =
structs.ErrPluginShutdown
97┆ case <-time.After(3 * time.Second):
helper/pool/pool.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
524┆ select {
525┆ case <-p.shutdownCh:
526┆
return
527┆ case <-time.After(time.Second):
nomad/acl_endpoint_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
2685┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
2721┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
2781┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
2817┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
2877┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
2925┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
2977┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
3013┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
3486┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
3567┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
3739┆ t.Parallel()
nomad/alloc_endpoint_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
1679┆ t.Parallel()
⋮┆----------------------------------------
▶▶┆ Autofix ▶ ci.Parallel(t)
1762┆ t.Parallel()
nomad/fsm.go
semgrep.no-time-in-fsm
time.Now() should not be called from within the Server's FSM. Apply Raft log messages to the
State Store must be deterministic so that each server contains exactly the same state. Since
time drifts between nodes, it must be set before the Raft log message is applied so that all
Raft members see the same timestamp.
638┆ Launch: time.Now(),
nomad/heartbeat.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
253┆ select {
254┆ case <-time.After(5 * time.Second):
255┆
h.heartbeatTimersLock.Lock()
256┆ num :=
len(h.heartbeatTimers)
257┆
h.heartbeatTimersLock.Unlock()
258┆ metrics.SetGauge([]string{"nomad", "heartbeat", "active"},
float32(num))
259┆
260┆ case <-h.shutdownCh:
261┆
return
nomad/leader.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
540┆ select {
541┆ case <-time.After(s.config.ReplicationBackoff):
542┆ goto
START
543┆ case <-stopCh:
544┆
return
⋮┆----------------------------------------
1762┆ select {
1763┆ case <-time.After(s.config.ReplicationBackoff):
1764┆ goto
START
1765┆ case <-stopCh:
1766┆
return
⋮┆----------------------------------------
1904┆ select {
1905┆ case <-time.After(s.config.ReplicationBackoff):
1906┆ goto
START
1907┆ case <-stopCh:
1908┆
return
nomad/node_endpoint_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
4381┆ t.Parallel()
nomad/plan_apply_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
894┆ t.Parallel()
nomad/plan_endpoint_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
138┆ t.Parallel()
nomad/rpc.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
217┆ select {
218┆ case <-ctx.Done():
219┆ case <-time.After(*loopDelay):
⋮┆----------------------------------------
602┆ select {
603┆ case <-time.After(jitter):
604┆ goto
CHECK_LEADER
605┆ case <-r.shutdownCh:
nomad/state/state_store.go
semgrep.no-time-in-fsm
time.Now() should not be called from within the Server's FSM. Apply Raft log messages to the
State Store must be deterministic so that each server contains exactly the same state. Since
time drifts between nodes, it must be set before the Raft log message is applied so that all
Raft members see the same timestamp.
4825┆ status.RequireProgressBy = time.Now().Add(status.ProgressDeadline)
nomad/structs/workload_id_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
175┆ t.Parallel()
nomad/util_test.go
semgrep.tests-no-parallel
Use ci.Parallel(t) instead of t.Parallel()
▶▶┆ Autofix ▶ ci.Parallel(t)
200┆ t.Parallel()
nomad/worker.go
semgrep.time-after-leak
Potential leak of time.Timer, consider using NewSafeTimer instead
884┆ select {
885┆ case <-time.After(backoff):
886┆ return
false
887┆ case <-w.ctx.Done():
888┆ return
true
┌──────────────┐
│ Scan Summary │
└──────────────┘
Some files were skipped or only partially analyzed.
Scan was limited to files tracked by git.
Partially scanned: 15 files only partially analyzed due to parsing or internal Semgrep errors
Scan skipped: 8 files larger than 1.0 MB, 1 files matching .semgrepignore patterns
For a full list of skipped files, run semgrep with the --verbose flag.
Ran 17 rules on 3484 files: 73 findings.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the rules in .semgrep and the affected Go files listed in the Semgrep output, then run semgrep --config=.semgrep/ ./ locally. Address the reported warnings and failures, including time.After findings and t.Parallel findings, until the full repository scan completes without findings and CI can scan all files.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- ci-cd, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100