Skip to content

Commit de0c95e

Browse files
committed
Fix bugs found during code audit
- PingCheck: parse output regardless of exit code, fix false negatives on partial packet loss - Worker pool: add panic recovery so a check panic returns failed result instead of deadlocking - E2E checks: use select with context for port acquisition to prevent deadlock on timeout - E2E checks: cap startup wait at timeout/3 (max 2s) to respect timeout budget - DoH e2e: add missing port release sleep after process cleanup - root.go: fix latent slice mutation in append(passed, failed...) - scan.go: guard against negative strings.Repeat with max(0, ...)
1 parent efe3509 commit de0c95e

6 files changed

Lines changed: 62 additions & 22 deletions

File tree

cmd/root.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,9 @@ func writeReport(mode string, results []scanner.Result, elapsed time.Duration, s
7373
if sortBy != "" {
7474
scanner.SortByMetric(passed, sortBy)
7575
}
76-
sorted := append(passed, failed...)
76+
sorted := make([]scanner.Result, 0, len(results))
77+
sorted = append(sorted, passed...)
78+
sorted = append(sorted, failed...)
7779

7880
if err := scanner.WriteReport(sorted, outputFile); err != nil {
7981
return err

cmd/scan.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ func printSummary(report scanner.ChainReport, topN int, totalTime time.Duration)
276276
fmt.Fprintf(w, "\n %s\u250c%s\u2510%s\n", colorDim, strings.Repeat("\u2500", 60), colorReset)
277277
fmt.Fprintf(w, " %s\u2502%s %sTop %d Resolvers%s%s%s\u2502%s\n",
278278
colorDim, colorReset, colorBold, limit, colorReset,
279-
strings.Repeat(" ", 60-17-digitCount(limit)), colorDim, colorReset)
279+
strings.Repeat(" ", max(0, 60-17-digitCount(limit))), colorDim, colorReset)
280280
fmt.Fprintf(w, " %s\u251c%s\u2524%s\n", colorDim, strings.Repeat("\u2500", 60), colorReset)
281281

282282
for i := 0; i < limit; i++ {

internal/scanner/check.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,11 @@ func PingCheck(count int) CheckFunc {
6565

6666
args := buildPingArgs(count, secs, deadline, ip)
6767
cmd := exec.CommandContext(ctx, "ping", args...)
68-
out, err := cmd.CombinedOutput()
69-
if err != nil {
68+
out, _ := cmd.CombinedOutput()
69+
avg := parsePingAvg(string(out))
70+
if avg <= 0 {
7071
return false, nil
7172
}
72-
avg := parsePingAvg(string(out))
7373
return true, Metrics{"ping_ms": avg}
7474
}
7575
}

internal/scanner/doh.go

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -177,14 +177,19 @@ func DoHTunnelCheck(domain string, count int) CheckFunc {
177177
// DoHDnsttCheck runs an e2e test using dnstt-client in DoH mode.
178178
func DoHDnsttCheck(domain, pubkey, testURL string, ports chan int) CheckFunc {
179179
return func(url string, timeout time.Duration) (bool, Metrics) {
180-
port := <-ports
180+
ctx, cancel := context.WithTimeout(context.Background(), timeout)
181+
defer cancel()
182+
183+
var port int
184+
select {
185+
case port = <-ports:
186+
case <-ctx.Done():
187+
return false, nil
188+
}
181189
defer func() { ports <- port }()
182190

183191
start := time.Now()
184192

185-
ctx, cancel := context.WithTimeout(context.Background(), timeout)
186-
defer cancel()
187-
188193
cmd := execCommandContext(ctx, "dnstt-client",
189194
"-doh", url,
190195
"-pubkey", pubkey,
@@ -198,10 +203,16 @@ func DoHDnsttCheck(domain, pubkey, testURL string, ports chan int) CheckFunc {
198203
defer func() {
199204
cmd.Process.Kill()
200205
cmd.Wait()
206+
time.Sleep(100 * time.Millisecond)
201207
}()
202208

209+
// Wait for subprocess to start, but cap at 1/3 of timeout
210+
startupWait := timeout / 3
211+
if startupWait > 2*time.Second {
212+
startupWait = 2 * time.Second
213+
}
203214
select {
204-
case <-time.After(2 * time.Second):
215+
case <-time.After(startupWait):
205216
case <-ctx.Done():
206217
return false, nil
207218
}

internal/scanner/e2e.go

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,19 @@ func execCommandContext(ctx context.Context, name string, args ...string) *exec.
2424

2525
func DnsttCheck(domain, pubkey, testURL string, ports chan int) CheckFunc {
2626
return func(ip string, timeout time.Duration) (bool, Metrics) {
27-
port := <-ports
27+
ctx, cancel := context.WithTimeout(context.Background(), timeout)
28+
defer cancel()
29+
30+
var port int
31+
select {
32+
case port = <-ports:
33+
case <-ctx.Done():
34+
return false, nil
35+
}
2836
defer func() { ports <- port }()
2937

3038
start := time.Now()
3139

32-
ctx, cancel := context.WithTimeout(context.Background(), timeout)
33-
defer cancel()
34-
3540
cmd := execCommandContext(ctx, "dnstt-client",
3641
"-udp", ip+":53",
3742
"-pubkey", pubkey,
@@ -49,8 +54,13 @@ func DnsttCheck(domain, pubkey, testURL string, ports chan int) CheckFunc {
4954
time.Sleep(100 * time.Millisecond)
5055
}()
5156

57+
// Wait for subprocess to start, but cap at 1/3 of timeout
58+
startupWait := timeout / 3
59+
if startupWait > 2*time.Second {
60+
startupWait = 2 * time.Second
61+
}
5262
select {
53-
case <-time.After(2 * time.Second):
63+
case <-time.After(startupWait):
5464
case <-ctx.Done():
5565
return false, nil
5666
}
@@ -65,14 +75,19 @@ func DnsttCheck(domain, pubkey, testURL string, ports chan int) CheckFunc {
6575

6676
func SlipstreamCheck(domain, certPath, testURL string, ports chan int) CheckFunc {
6777
return func(ip string, timeout time.Duration) (bool, Metrics) {
68-
port := <-ports
78+
ctx, cancel := context.WithTimeout(context.Background(), timeout)
79+
defer cancel()
80+
81+
var port int
82+
select {
83+
case port = <-ports:
84+
case <-ctx.Done():
85+
return false, nil
86+
}
6987
defer func() { ports <- port }()
7088

7189
start := time.Now()
7290

73-
ctx, cancel := context.WithTimeout(context.Background(), timeout)
74-
defer cancel()
75-
7691
args := []string{
7792
"-d", domain,
7893
"-r", ip + ":53",
@@ -93,8 +108,13 @@ func SlipstreamCheck(domain, certPath, testURL string, ports chan int) CheckFunc
93108
time.Sleep(100 * time.Millisecond)
94109
}()
95110

111+
// Wait for subprocess to start, but cap at 1/3 of timeout
112+
startupWait := timeout / 3
113+
if startupWait > 2*time.Second {
114+
startupWait = 2 * time.Second
115+
}
96116
select {
97-
case <-time.After(2 * time.Second):
117+
case <-time.After(startupWait):
98118
case <-ctx.Done():
99119
return false, nil
100120
}

internal/scanner/worker.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,15 @@ func RunPool(ips []string, workers int, timeout time.Duration, check CheckFunc,
2525
for i := 0; i < workers; i++ {
2626
go func() {
2727
for ip := range jobs {
28-
ok, m := check(ip, timeout)
29-
results <- Result{IP: ip, OK: ok, Metrics: m}
28+
func() {
29+
defer func() {
30+
if r := recover(); r != nil {
31+
results <- Result{IP: ip, OK: false}
32+
}
33+
}()
34+
ok, m := check(ip, timeout)
35+
results <- Result{IP: ip, OK: ok, Metrics: m}
36+
}()
3037
}
3138
}()
3239
}

0 commit comments

Comments
 (0)