diff --git a/toxics/bandwidth.go b/toxics/bandwidth.go index 2dd1e881..7440a4e6 100644 --- a/toxics/bandwidth.go +++ b/toxics/bandwidth.go @@ -38,8 +38,11 @@ func (t *BandwidthToxic) Pipe(stub *ToxicStub) { } else { sleep += time.Duration(len(p.Data)) * time.Millisecond / time.Duration(t.Rate) } - // If the rate is low enough, split the packet up and send in 100 millisecond intervals - for int64(len(p.Data)) > t.Rate*100 { + // If the rate is low enough, split the packet up and send in 100 millisecond intervals. + // Guard on t.Rate > 0: a non-positive rate disables throttling (see above), and + // t.Rate*100 would otherwise be a zero split size (infinite loop emitting empty + // chunks) or a negative slice bound (panic). + for t.Rate > 0 && int64(len(p.Data)) > t.Rate*100 { select { case <-time.After(100 * time.Millisecond): stub.Output <- &stream.StreamChunk{ diff --git a/toxics/bandwidth_test.go b/toxics/bandwidth_test.go index ede49bef..9a663055 100644 --- a/toxics/bandwidth_test.go +++ b/toxics/bandwidth_test.go @@ -2,12 +2,14 @@ package toxics_test import ( "bytes" + "fmt" "io" "net" "strings" "testing" "time" + "github.com/Shopify/toxiproxy/v2/stream" "github.com/Shopify/toxiproxy/v2/testhelper" "github.com/Shopify/toxiproxy/v2/toxics" ) @@ -93,3 +95,54 @@ func BenchmarkBandwidthToxic100MB(b *testing.B) { b.Error("Failed to close TCP connection", err) } } + +// TestBandwidthToxicNonPositiveRate verifies that a bandwidth toxic configured +// with a non-positive rate (rate <= 0) forwards data unthrottled instead of +// hanging or panicking. The Pipe already special-cases `t.Rate <= 0` to disable +// throttling (sleep = 0), so the intent is that such a toxic is a no-op. +// +// Regression test: with rate == 0 the packet-splitting loop +// (`for len(p.Data) > t.Rate*100`) spins forever emitting empty chunks +// (p.Data[:0]) and never forwards the payload; with rate < 0 the same loop +// slices p.Data[:negative] and panics with "slice bounds out of range". +func TestBandwidthToxicNonPositiveRate(t *testing.T) { + for _, rate := range []int64{0, -1} { + rate := rate + t.Run(fmt.Sprintf("rate_%d", rate), func(t *testing.T) { + toxic := &toxics.BandwidthToxic{Rate: rate} + + input := make(chan *stream.StreamChunk) + output := make(chan *stream.StreamChunk, 100) + stub := toxics.NewToxicStub(input, output) + + payload := []byte("hello world") + + panicked := make(chan interface{}, 1) + go func() { + defer func() { panicked <- recover() }() + toxic.Pipe(stub) + }() + + go func() { input <- &stream.StreamChunk{Data: payload} }() + + select { + case chunk := <-output: + if !bytes.Equal(chunk.Data, payload) { + t.Errorf("rate=%d: expected full payload forwarded unthrottled, "+ + "got %d bytes (%q)", rate, len(chunk.Data), chunk.Data) + } + case p := <-panicked: + t.Fatalf("rate=%d: BandwidthToxic.Pipe panicked: %v", rate, p) + case <-time.After(2 * time.Second): + t.Fatalf("rate=%d: timed out; BandwidthToxic.Pipe did not forward "+ + "the chunk (infinite split loop)", rate) + } + + // Unblock and stop the Pipe goroutine if it is still running. + select { + case stub.Interrupt <- struct{}{}: + case <-time.After(time.Second): + } + }) + } +}