Skip to content

Commit cecc4ba

Browse files
mosajjalclaude
andcommitted
fix: resolve race conditions, shutdown leaks, and critical bugs across pipeline
Fix data races: - Add sync.RWMutex to dedup hash table in capture, protecting concurrent read/write during packet processing and ticker cleanup - Add domainFilterMu sync.RWMutex to domain skip/allow filtering, ensuring thread-safe hot-reload of domain lists - Add splunkConnMu sync.RWMutex to protect global splunk connection map - Convert all captureConfig methods from value to pointer receivers to prevent copying the embedded mutex Fix critical bugs: - packet.go: use packet.ip instead of stale local ip4 in defrag return path, which caused incorrect protocol detection and flow extraction - util.go: MaskSize6 validation checked MaskSize4 for lower bound - influx.go: metric counter used wrong name "stdoutSkipped" - util.go: remove redundant nested if-err-not-nil in config parsing Fix graceful shutdown: - Add defer close(closeChannel) to all 12 output backends to prevent Close() from deadlocking on context cancellation - Add case <-ctx.Done() to output worker loops (sentinel, splunk, zinc, victorialogs) that were missing it - Add sync.WaitGroup for worker lifecycle tracking in influx, parquet, postgres, stdout, victorialogs outputs Replace log.Fatal with recoverable errors: - elastic, file, postgres, syslog, influx, functions.go: return errors instead of fataling, allowing the dispatch loop to gracefully remove failed outputs - LoadDomainsCsv now returns error as 4th value; callers log and skip update on failure instead of crashing Other fixes: - outputs.go: non-blocking fan-out dispatch with dropped packet metrics - packet.go: log and count silent decode errors via decodingErrors metric - packet.go: single errgroup for all input handler workers - livecap_windows.go: snaplen 1600 -> 65535 - parquet.go: iterate all DNS questions instead of only first - functions.go: store suffix domains pre-reversed for efficient lookup, add 30s HTTP timeout for remote domain lists - splunk.go: remove deprecated rand.Seed - main.go: remove no-op ctx.Done(), bound spin-wait with ctx.Err() Add tests for dedup concurrency, defrag path correctness, transport processing (UDP/TCP/non-DNS), and decode error handling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 8ed6416 commit cecc4ba

25 files changed

Lines changed: 630 additions & 142 deletions

cmd/dnsmonster/main.go

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,9 @@ func handleInterrupt(ctx context.Context) {
5050
go func() {
5151
<-c
5252
log.Infof("SIGINT Received. Stopping capture...")
53-
go util.GlobalCancel()
54-
go ctx.Done()
53+
util.GlobalCancel()
5554
<-time.After(4 * time.Second)
5655
log.Fatal("emergency exit")
57-
os.Exit(1)
5856
}()
5957
}
6058

@@ -92,14 +90,12 @@ func main() {
9290
g.Go(func() error { capture.GlobalCaptureConfig.CheckFlagsAndStart(ctx); return nil })
9391
// Set up output dispatch
9492
var c chan util.DNSResult
95-
for {
93+
for ctx.Err() == nil {
9694
c = capture.GlobalCaptureConfig.GetResultChannel()
97-
if c == nil {
98-
time.Sleep(10 * time.Millisecond)
99-
continue
100-
} else {
95+
if c != nil {
10196
break
10297
}
98+
time.Sleep(10 * time.Millisecond)
10399
}
104100

105101
g.Go(func() error { return setupOutputs(ctx, &c) })

cmd/dnsmonster/outputs.go

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,15 @@ package main
1717

1818
import (
1919
"context"
20+
"fmt"
2021
"time"
2122

22-
_ "github.com/mosajjal/dnsmonster/internal/output" // this will automatically set up all the outputs
23-
"github.com/mosajjal/dnsmonster/internal/util"
23+
metrics "github.com/rcrowley/go-metrics"
2424
log "github.com/sirupsen/logrus"
2525
"golang.org/x/sync/errgroup"
26+
27+
_ "github.com/mosajjal/dnsmonster/internal/output" // this will automatically set up all the outputs
28+
"github.com/mosajjal/dnsmonster/internal/util"
2629
)
2730

2831
// a helper function to remove one of the outputs from the globaldispatch list
@@ -48,9 +51,9 @@ func setupOutputs(ctx context.Context, resultChannel *chan util.DNSResult) error
4851
}
4952
}
5053

51-
// check to see if at least one output is specified, otherwise we should panic exit
54+
// check to see if at least one output is specified
5255
if len(util.GlobalDispatchList) == 0 {
53-
log.Fatal("No output specified. Please specify at least one output")
56+
return fmt.Errorf("no output specified, please specify at least one output")
5457
}
5558
// todo: currently, there's no check to see if allowdomains and skipdomains are provided if the output type demands it.
5659

@@ -72,21 +75,22 @@ func setupOutputs(ctx context.Context, resultChannel *chan util.DNSResult) error
7275
log.Infof("allowDomains refresh interval is %s", util.GeneralFlags.AllowDomainsRefreshInterval)
7376
}
7477
g, gCtx := errgroup.WithContext(ctx)
78+
dispatchedPackets := metrics.GetOrRegisterCounter("dispatchedPackets", metrics.DefaultRegistry)
79+
droppedPackets := metrics.GetOrRegisterCounter("droppedPackets", metrics.DefaultRegistry)
80+
7581
g.Go(func() error {
7682
// blocking loop
7783
for {
7884
select {
7985
case data := <-*resultChannel:
8086
for _, o := range util.GlobalDispatchList {
81-
// Non-blocking send with timeout to prevent blocking on full channels
87+
// Non-blocking send to prevent blocking on full channels
8288
select {
8389
case o.OutputChannel() <- data:
84-
// Successfully sent
85-
case <-time.After(100 * time.Millisecond):
86-
// Channel is full or blocked, log and continue
87-
log.Warnf("Output channel blocked, dropping packet")
88-
case <-gCtx.Done():
89-
return nil
90+
dispatchedPackets.Inc(1)
91+
default:
92+
// Channel is full, drop packet
93+
droppedPackets.Inc(1)
9094
}
9195
}
9296

@@ -99,7 +103,7 @@ func setupOutputs(ctx context.Context, resultChannel *chan util.DNSResult) error
99103
}
100104
}
101105
})
102-
return nil
106+
return g.Wait()
103107
}
104108

105109
// vim: foldmethod=marker

internal/capture/afpacket_linux.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ func afpacketComputeSize(targetSizeMb uint, snaplen uint, pageSize uint) (
9292
// return err
9393
// }
9494

95-
func (config captureConfig) initializeLiveAFpacket(devName, filter string) *afpacketHandle {
95+
func (config *captureConfig) initializeLiveAFpacket(devName, filter string) *afpacketHandle {
9696
// Open device
9797
// var tPacket *afpacket.TPacket
9898
var err error

internal/capture/afpacket_nonlinux.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ func (afhandle *afpacketHandle) Stat() (uint, uint, error) {
5959
return 0, 0, fmt.Errorf("Dnsmonster has been compiled without afpacket support for this platform")
6060
}
6161

62-
func (config captureConfig) initializeLiveAFpacket(devName, filter string) *afpacketHandle {
62+
func (config *captureConfig) initializeLiveAFpacket(devName, filter string) *afpacketHandle {
6363
return nil
6464
}
6565

internal/capture/capture.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ type captureConfig struct {
6969
ratioA int
7070
ratioB int
7171
dedupHashTable map[uint64]bool
72+
dedupMu sync.RWMutex
7273
}
7374

7475
// GlobalCaptureConfig is accessible globally
@@ -84,11 +85,11 @@ func init() {
8485

8586
}
8687

87-
func (config captureConfig) GetResultChannel() chan util.DNSResult {
88+
func (config *captureConfig) GetResultChannel() chan util.DNSResult {
8889
return config.resultChannel
8990
}
9091

91-
func (config captureConfig) cleanExit(ctx context.Context) {
92+
func (config *captureConfig) cleanExit(ctx context.Context) {
9293
ctx.Done()
9394
log.Infof("Stopping capture...")
9495
}
@@ -139,7 +140,9 @@ func (config *captureConfig) CheckFlagsAndStart(ctx context.Context) {
139140
select {
140141
case <-time.NewTicker(config.DedupCleanupInterval).C:
141142
log.Infof("cleaning up dedup hash table")
143+
config.dedupMu.Lock()
142144
config.dedupHashTable = make(map[uint64]bool)
145+
config.dedupMu.Unlock()
143146
case <-gCtx.Done():
144147
return nil
145148
}

internal/capture/capture_test.go

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@ package capture
1818
import (
1919
"strconv"
2020
"strings"
21+
"sync"
2122
"testing"
23+
"time"
2224
)
2325

2426
func TestSampleRatioParsing(t *testing.T) {
@@ -236,6 +238,109 @@ func TestPortValidation(t *testing.T) {
236238
}
237239
}
238240

241+
// TestDedupHashTableConcurrency verifies that concurrent reads and writes
242+
// to the dedup hash table are safe when protected by the mutex.
243+
// Must pass with -race.
244+
func TestDedupHashTableConcurrency(t *testing.T) {
245+
config := &captureConfig{
246+
dedupHashTable: make(map[uint64]bool),
247+
}
248+
249+
const goroutines = 10
250+
const iterations = 1000
251+
252+
var wg sync.WaitGroup
253+
254+
// Launch writer goroutines
255+
for i := 0; i < goroutines; i++ {
256+
wg.Add(1)
257+
go func(id int) {
258+
defer wg.Done()
259+
for j := 0; j < iterations; j++ {
260+
hash := FNV1A([]byte{byte(id), byte(j), byte(j >> 8)})
261+
config.dedupMu.RLock()
262+
_, exists := config.dedupHashTable[hash]
263+
config.dedupMu.RUnlock()
264+
if !exists {
265+
config.dedupMu.Lock()
266+
config.dedupHashTable[hash] = true
267+
config.dedupMu.Unlock()
268+
}
269+
}
270+
}(i)
271+
}
272+
273+
// Launch a goroutine that clears the table (simulating the ticker)
274+
wg.Add(1)
275+
go func() {
276+
defer wg.Done()
277+
for j := 0; j < 10; j++ {
278+
config.dedupMu.Lock()
279+
config.dedupHashTable = make(map[uint64]bool)
280+
config.dedupMu.Unlock()
281+
time.Sleep(time.Millisecond)
282+
}
283+
}()
284+
285+
wg.Wait()
286+
287+
// If we got here without a race detector complaint, the test passes
288+
config.dedupMu.RLock()
289+
size := len(config.dedupHashTable)
290+
config.dedupMu.RUnlock()
291+
t.Logf("Final dedup table size: %d", size)
292+
}
293+
294+
// TestDedupHashTableClearing verifies the clearing mechanism works:
295+
// entries are removed when the table is cleared.
296+
func TestDedupHashTableClearing(t *testing.T) {
297+
config := &captureConfig{
298+
dedupHashTable: make(map[uint64]bool),
299+
}
300+
301+
// Add some entries
302+
for i := uint64(0); i < 100; i++ {
303+
config.dedupMu.Lock()
304+
config.dedupHashTable[i] = true
305+
config.dedupMu.Unlock()
306+
}
307+
308+
config.dedupMu.RLock()
309+
if len(config.dedupHashTable) != 100 {
310+
config.dedupMu.RUnlock()
311+
t.Fatalf("Expected 100 entries, got %d", len(config.dedupHashTable))
312+
}
313+
config.dedupMu.RUnlock()
314+
315+
// Clear the table (simulating what the ticker does)
316+
config.dedupMu.Lock()
317+
config.dedupHashTable = make(map[uint64]bool)
318+
config.dedupMu.Unlock()
319+
320+
config.dedupMu.RLock()
321+
if len(config.dedupHashTable) != 0 {
322+
config.dedupMu.RUnlock()
323+
t.Fatalf("Expected 0 entries after clearing, got %d", len(config.dedupHashTable))
324+
}
325+
config.dedupMu.RUnlock()
326+
327+
// Verify we can add entries again after clearing
328+
config.dedupMu.Lock()
329+
config.dedupHashTable[42] = true
330+
config.dedupMu.Unlock()
331+
332+
config.dedupMu.RLock()
333+
if len(config.dedupHashTable) != 1 {
334+
config.dedupMu.RUnlock()
335+
t.Fatalf("Expected 1 entry after re-adding, got %d", len(config.dedupHashTable))
336+
}
337+
_, exists := config.dedupHashTable[42]
338+
config.dedupMu.RUnlock()
339+
if !exists {
340+
t.Fatal("Expected key 42 to exist")
341+
}
342+
}
343+
239344
// Benchmark dedup hash lookup
240345
func BenchmarkDedupLookup(b *testing.B) {
241346
dedupTable := make(map[uint64]bool)

internal/capture/dnstap.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ func dnsTapMsgToDNSResult(msg []byte) (*util.DNSResult, error) {
119119
return &myDNSResult, nil
120120
}
121121

122-
func (config captureConfig) StartDNSTap(ctx context.Context) error {
122+
func (config *captureConfig) StartDNSTap(ctx context.Context) error {
123123
log.Info("Starting DNStap capture")
124124

125125
packetsCaptured := metrics.GetOrRegisterGauge("packetsCaptured", metrics.DefaultRegistry)

internal/capture/livecap_windows.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ type livePcapHandle struct {
2929
}
3030

3131
func initializeLivePcap(devName, filter string) (*livePcapHandle, error) {
32-
handle, err := pcap.OpenLive(devName, 1600, true, pcap.BlockForever)
32+
handle, err := pcap.OpenLive(devName, 65535, true, pcap.BlockForever)
3333
if err != nil {
3434
return nil, fmt.Errorf("failed to open live capture on %s: %w", devName, err)
3535
}

internal/capture/nondnstap.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import (
2525
"golang.org/x/sync/errgroup"
2626
)
2727

28-
func (config captureConfig) StartNonDNSTap(ctx context.Context) error {
28+
func (config *captureConfig) StartNonDNSTap(ctx context.Context) error {
2929
packetsCaptured := metrics.GetOrRegisterGauge("packetsCaptured", metrics.DefaultRegistry)
3030
packetsDropped := metrics.GetOrRegisterGauge("packetsDropped", metrics.DefaultRegistry)
3131
packetsDuplicate := metrics.GetOrRegisterCounter("packetsDuplicate", metrics.DefaultRegistry)
@@ -117,9 +117,13 @@ func (config captureConfig) StartNonDNSTap(ctx context.Context) error {
117117
skipForDudup := false
118118
if config.Dedup {
119119
hash := FNV1A(data)
120+
config.dedupMu.RLock()
120121
_, ok := config.dedupHashTable[hash] // check for existence
122+
config.dedupMu.RUnlock()
121123
if !ok {
124+
config.dedupMu.Lock()
122125
config.dedupHashTable[hash] = true
126+
config.dedupMu.Unlock()
123127
} else {
124128
skipForDudup = true
125129
packetsDuplicate.Inc(1)

0 commit comments

Comments
 (0)