Repository: quic-go/quic-go
Pull request #4388 (2024-03-24 21:01:39)

http3: reject duplicate QPACK decoder and encoder streams

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/quic-go/quic-go/pull/4388?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 84.87%. Comparing base [(`268208f`)](https://app.codecov.io/gh/quic-go/quic-go/commit/268208fbefab03c827951bc44ba89889778c3fef?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) to head [(`abc315d`)](https://app.codecov.io/gh/quic-go/quic-go/pull/4388?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #4388      +/-   ##
==========================================
+ Coverage   84.86%   84.87%   +0.01%     
==========================================
  Files         152      152              
  Lines       14324    14334      +10     
==========================================
+ Hits        12155    12165      +10     
  Misses       1670     1670              
  Partials      499      499              
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/quic-go/quic-go/pull/4388?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).


Patch:
diff --git a/http3/conn.go b/http3/conn.go
index 591afbb2..a2da426d 100644
--- a/http3/conn.go
+++ b/http3/conn.go
@@ -40,7 +40,11 @@ func newConnection(
 }
 
 func (c *connection) HandleUnidirectionalStreams() {
-	var rcvdControlStream atomic.Bool
+	var (
+		rcvdControlStr      atomic.Bool
+		rcvdQPACKEncoderStr atomic.Bool
+		rcvdQPACKDecoderStr atomic.Bool
+	)
 
 	for {
 		str, err := c.quicConn.AcceptUniStream(context.Background())
@@ -61,9 +65,17 @@ func (c *connection) HandleUnidirectionalStreams() {
 			// We're only interested in the control stream here.
 			switch streamType {
 			case streamTypeControlStream:
-			case streamTypeQPACKEncoderStream, streamTypeQPACKDecoderStream:
+			case streamTypeQPACKEncoderStream:
+				if isFirst := rcvdQPACKEncoderStr.CompareAndSwap(false, true); !isFirst {
+					c.quicConn.CloseWithError(quic.ApplicationErrorCode(ErrCodeStreamCreationError), "duplicate QPACK encoder stream")
+				}
+				// Our QPACK implementation doesn't use the dynamic table yet.
+				return
+			case streamTypeQPACKDecoderStream:
+				if isFirst := rcvdQPACKDecoderStr.CompareAndSwap(false, true); !isFirst {
+					c.quicConn.CloseWithError(quic.ApplicationErrorCode(ErrCodeStreamCreationError), "duplicate QPACK decoder stream")
+				}
 				// Our QPACK implementation doesn't use the dynamic table yet.
-				// TODO: check that only one stream of each type is opened.
 				return
 			case streamTypePushStream:
 				switch c.perspective {
@@ -83,7 +95,7 @@ func (c *connection) HandleUnidirectionalStreams() {
 				return
 			}
 			// Only a single control stream is allowed.
-			if isFirstControlStr := rcvdControlStream.CompareAndSwap(false, true); !isFirstControlStr {
+			if isFirstControlStr := rcvdControlStr.CompareAndSwap(false, true); !isFirstControlStr {
 				c.quicConn.CloseWithError(quic.ApplicationErrorCode(ErrCodeStreamCreationError), "duplicate control stream")
 				return
 			}


Test patch:
diff --git a/http3/conn_test.go b/http3/conn_test.go
index bd990582..643236cb 100644
--- a/http3/conn_test.go
+++ b/http3/conn_test.go
@@ -124,6 +124,41 @@ var _ = Describe("Connection", func() {
 				}()
 				Eventually(done).Should(BeClosed())
 			})
+
+			It(fmt.Sprintf("rejects duplicate QPACK %s streams", name), func() {
+				qconn := mockquic.NewMockEarlyConnection(mockCtrl)
+				conn := newConnection(
+					qconn,
+					false,
+					nil,
+					protocol.PerspectiveClient,
+					utils.DefaultLogger,
+				)
+				buf := bytes.NewBuffer(quicvarint.Append(nil, streamType))
+				str1 := mockquic.NewMockStream(mockCtrl)
+				str1.EXPECT().Read(gomock.Any()).DoAndReturn(buf.Read).AnyTimes()
+				buf2 := bytes.NewBuffer(quicvarint.Append(nil, streamType))
+				str2 := mockquic.NewMockStream(mockCtrl)
+				str2.EXPECT().Read(gomock.Any()).DoAndReturn(buf2.Read).AnyTimes()
+				qconn.EXPECT().AcceptUniStream(gomock.Any()).Return(str1, nil)
+				qconn.EXPECT().AcceptUniStream(gomock.Any()).Return(str2, nil)
+				testDone := make(chan struct{})
+				qconn.EXPECT().AcceptUniStream(gomock.Any()).DoAndReturn(func(context.Context) (quic.ReceiveStream, error) {
+					<-testDone
+					return nil, errors.New("test done")
+				})
+				qconn.EXPECT().CloseWithError(qerr.ApplicationErrorCode(ErrCodeStreamCreationError), gomock.Any()).Do(func(qerr.ApplicationErrorCode, string) error {
+					close(testDone)
+					return nil
+				})
+				done := make(chan struct{})
+				go func() {
+					defer GinkgoRecover()
+					defer close(done)
+					conn.HandleUnidirectionalStreams()
+				}()
+				Eventually(done).Should(BeClosed())
+			})
 		}
 
 		It("resets streams other than the control stream and the QPACK streams", func() {