http2: fix weight overflow in RFC 7540 write scheduler

We use uint8 (0-255, inclusive) to represent the RFC 7540 priorities
weight (1-256, inclusive). To account for the difference, we add 1 to
the uint8 weight value within sortPriorityNodeSiblingsRFC7540.

However, the addition was done before converting the uint8 type to
float. As a result, when provided a maximum weight value, overflow will
happen and will cause the scheduler to treat the maximum weight as a
minimum weight instead.

This CL fixes the issue by making sure the addition happens after the
type conversion.

Change-Id: I404e87e5ad85fa06d5fa49cda613c93ac8847bdc
Reviewed-on: https://go-review.googlesource.com/c/net/+/714742
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Reviewed-by: Nicholas Husin <[email protected]>

Author: nicholashusin

http2/writesched_priority_rfc7540.go

@@ -214,8 +214,8 @@ func (z sortPriorityNodeSiblingsRFC7540) Swap(i, k int) { z[i], z[k] = z[k], z[i
 func (z sortPriorityNodeSiblingsRFC7540) Less(i, k int) bool {
 	// Prefer the subtree that has sent fewer bytes relative to its weight.
 	// See sections 5.3.2 and 5.3.4.
-	wi, bi := float64(z[i].weight+1), float64(z[i].subtreeBytes)
-	wk, bk := float64(z[k].weight+1), float64(z[k].subtreeBytes)
+	wi, bi := float64(z[i].weight)+1, float64(z[i].subtreeBytes)
+	wk, bk := float64(z[k].weight)+1, float64(z[k].subtreeBytes)
 	if bi == 0 && bk == 0 {
 		return wi >= wk
 	}

http2/writesched_priority_rfc7540_test.go

@@ -548,6 +548,39 @@ func TestPriorityWeights(t *testing.T) {
 	}
 }
 
+func TestPriorityWeightsMinMax(t *testing.T) {
+	ws := defaultPriorityWriteScheduler()
+	ws.OpenStream(1, OpenStreamOptions{})
+	ws.OpenStream(2, OpenStreamOptions{})
+
+	sc := &serverConn{maxFrameSize: 8}
+	st1 := &stream{id: 1, sc: sc}
+	st2 := &stream{id: 2, sc: sc}
+	st1.flow.add(40)
+	st2.flow.add(40)
+
+	// st2 gets 256x the bandwidth of st1 (256 = (255+1)/(0+1)).
+	// The maximum frame size is 8 bytes. The write sequence should be:
+	//   st2, total bytes so far is (st1=0,  st=8)
+	//   st1, total bytes so far is (st1=8,  st=8)
+	//   st2, total bytes so far is (st1=8,  st=16)
+	//   st2, total bytes so far is (st1=8,  st=24)
+	//   st2, total bytes so far is (st1=8,  st=32)
+	//   st2, total bytes so far is (st1=8,  st=40)  // 5x bandwidth
+	//   st1, total bytes so far is (st1=16, st=40)
+	//   st1, total bytes so far is (st1=24, st=40)
+	//   st1, total bytes so far is (st1=32, st=40)
+	//   st1, total bytes so far is (st1=40, st=40)
+	ws.Push(FrameWriteRequest{&writeData{1, make([]byte, 40), false}, st1, nil})
+	ws.Push(FrameWriteRequest{&writeData{2, make([]byte, 40), false}, st2, nil})
+	ws.AdjustStream(1, PriorityParam{StreamDep: 0, Weight: 0})
+	ws.AdjustStream(2, PriorityParam{StreamDep: 0, Weight: 255})
+
+	if err := checkPopAll(ws, []uint32{2, 1, 2, 2, 2, 2, 1, 1, 1, 1}); err != nil {
+		t.Error(err)
+	}
+}
+
 func TestPriorityRstStreamOnNonOpenStreams(t *testing.T) {
 	ws := NewPriorityWriteScheduler(&PriorityWriteSchedulerConfig{
 		MaxClosedNodesInTree: 0,