Skip to content

move the transport parameter stream limit check to the parser #2944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 22, 2020
Merged

move the transport parameter stream limit check to the parser #2944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6217ec5
remove stray comment in the streams map
marten-seemann Dec 18, 2020
c7d3aee
move the transport parameter stream limit check to the parser
marten-seemann Dec 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 36 additions & 6 deletions internal/wire/transport_parameter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,17 @@ import (
)

var _ = Describe("Transport Parameters", func() {
getRandomValue := func() uint64 {
getRandomValueUpTo := func(max int64) uint64 {
maxVals := []int64{math.MaxUint8 / 4, math.MaxUint16 / 4, math.MaxUint32 / 4, math.MaxUint64 / 4}
return uint64(rand.Int63n(maxVals[int(rand.Int31n(4))]))
m := maxVals[int(rand.Int31n(4))]
if m > max {
m = max
}
return uint64(rand.Int63n(m))
}

getRandomValue := func() uint64 {
return getRandomValueUpTo(math.MaxInt64)
}

BeforeEach(func() {
Expand Down Expand Up @@ -79,8 +87,8 @@ var _ = Describe("Transport Parameters", func() {
InitialMaxStreamDataUni: protocol.ByteCount(getRandomValue()),
InitialMaxData: protocol.ByteCount(getRandomValue()),
MaxIdleTimeout: 0xcafe * time.Second,
MaxBidiStreamNum: protocol.StreamNum(getRandomValue()),
MaxUniStreamNum: protocol.StreamNum(getRandomValue()),
MaxBidiStreamNum: protocol.StreamNum(getRandomValueUpTo(int64(protocol.MaxStreamCount))),
MaxUniStreamNum: protocol.StreamNum(getRandomValueUpTo(int64(protocol.MaxStreamCount))),
DisableActiveMigration: true,
StatelessResetToken: &token,
OriginalDestinationConnectionID: protocol.ConnectionID{0xde, 0xad, 0xbe, 0xef},
Expand Down Expand Up @@ -252,6 +260,28 @@ var _ = Describe("Transport Parameters", func() {
Expect(err.Error()).To(ContainSubstring("TRANSPORT_PARAMETER_ERROR: inconsistent transport parameter length"))
})

It("errors if initial_max_streams_bidi is too large", func() {
b := &bytes.Buffer{}
utils.WriteVarInt(b, uint64(initialMaxStreamsBidiParameterID))
utils.WriteVarInt(b, uint64(utils.VarIntLen(uint64(protocol.MaxStreamCount+1))))
utils.WriteVarInt(b, uint64(protocol.MaxStreamCount+1))
addInitialSourceConnectionID(b)
err := (&TransportParameters{}).Unmarshal(b.Bytes(), protocol.PerspectiveServer)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("TRANSPORT_PARAMETER_ERROR: initial_max_streams_bidi too large: 1152921504606846977 (maximum 1152921504606846976)"))
})

It("errors if initial_max_streams_uni is too large", func() {
b := &bytes.Buffer{}
utils.WriteVarInt(b, uint64(initialMaxStreamsUniParameterID))
utils.WriteVarInt(b, uint64(utils.VarIntLen(uint64(protocol.MaxStreamCount+1))))
utils.WriteVarInt(b, uint64(protocol.MaxStreamCount+1))
addInitialSourceConnectionID(b)
err := (&TransportParameters{}).Unmarshal(b.Bytes(), protocol.PerspectiveServer)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("TRANSPORT_PARAMETER_ERROR: initial_max_streams_uni too large: 1152921504606846977 (maximum 1152921504606846976)"))
})

It("handles huge max_ack_delay values", func() {
b := &bytes.Buffer{}
val := uint64(math.MaxUint64) / 5
Expand Down Expand Up @@ -416,8 +446,8 @@ var _ = Describe("Transport Parameters", func() {
InitialMaxStreamDataBidiRemote: protocol.ByteCount(getRandomValue()),
InitialMaxStreamDataUni: protocol.ByteCount(getRandomValue()),
InitialMaxData: protocol.ByteCount(getRandomValue()),
MaxBidiStreamNum: protocol.StreamNum(getRandomValue()),
MaxUniStreamNum: protocol.StreamNum(getRandomValue()),
MaxBidiStreamNum: protocol.StreamNum(getRandomValueUpTo(int64(protocol.MaxStreamCount))),
MaxUniStreamNum: protocol.StreamNum(getRandomValueUpTo(int64(protocol.MaxStreamCount))),
ActiveConnectionIDLimit: getRandomValue(),
}
Expect(params.ValidFor0RTT(params)).To(BeTrue())
Expand Down
6 changes: 6 additions & 0 deletions internal/wire/transport_parameters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,14 @@ func (p *TransportParameters) readNumericTransportParameter(
p.InitialMaxData = protocol.ByteCount(val)
case initialMaxStreamsBidiParameterID:
p.MaxBidiStreamNum = protocol.StreamNum(val)
if p.MaxBidiStreamNum > protocol.MaxStreamCount {
return fmt.Errorf("initial_max_streams_bidi too large: %d (maximum %d)", p.MaxBidiStreamNum, protocol.MaxStreamCount)
}
case initialMaxStreamsUniParameterID:
p.MaxUniStreamNum = protocol.StreamNum(val)
if p.MaxUniStreamNum > protocol.MaxStreamCount {
return fmt.Errorf("initial_max_streams_uni too large: %d (maximum %d)", p.MaxUniStreamNum, protocol.MaxStreamCount)
}
case maxIdleTimeoutParameterID:
p.MaxIdleTimeout = utils.MaxDuration(protocol.MinRemoteIdleTimeout, time.Duration(val)*time.Millisecond)
case maxUDPPayloadSizeParameterID:
Expand Down
6 changes: 2 additions & 4 deletions mock_stream_manager_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 3 additions & 8 deletions session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ type streamManager interface {
AcceptStream(context.Context) (Stream, error)
AcceptUniStream(context.Context) (ReceiveStream, error)
DeleteStream(protocol.StreamID) error
UpdateLimits(*wire.TransportParameters) error
UpdateLimits(*wire.TransportParameters)
HandleMaxStreamsFrame(*wire.MaxStreamsFrame) error
CloseWithError(error)
}
Expand Down Expand Up @@ -1389,10 +1389,7 @@ func (s *session) restoreTransportParameters(params *wire.TransportParameters) {
s.peerParams = params
s.connIDGenerator.SetMaxActiveConnIDs(params.ActiveConnectionIDLimit)
s.connFlowController.UpdateSendWindow(params.InitialMaxData)
if err := s.streamsMap.UpdateLimits(params); err != nil {
s.closeLocal(err)
return
}
s.streamsMap.UpdateLimits(params)
}

func (s *session) processTransportParameters(params *wire.TransportParameters) {
Expand Down Expand Up @@ -1435,9 +1432,7 @@ func (s *session) processTransportParametersImpl(params *wire.TransportParameter
// Our local idle timeout will always be > 0.
s.idleTimeout = utils.MinNonZeroDuration(s.config.MaxIdleTimeout, params.MaxIdleTimeout)
s.keepAliveInterval = utils.MinDuration(s.idleTimeout/2, protocol.MaxKeepAliveInterval)
if err := s.streamsMap.UpdateLimits(params); err != nil {
return err
}
s.streamsMap.UpdateLimits(params)
s.packer.HandleTransportParameters(params)
s.frameParser.SetAckDelayExponent(params.AckDelayExponent)
s.connFlowController.UpdateSendWindow(params.InitialMaxData)
Expand Down
8 changes: 1 addition & 7 deletions streams_map.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,15 +222,9 @@ func (m *streamsMap) HandleMaxStreamsFrame(f *wire.MaxStreamsFrame) error {
return nil
}

func (m *streamsMap) UpdateLimits(p *wire.TransportParameters) error {
if p.MaxBidiStreamNum > protocol.MaxStreamCount ||
p.MaxUniStreamNum > protocol.MaxStreamCount {
return qerr.StreamLimitError
}
// Max{Uni,Bidi}StreamID returns the highest stream ID that the peer is allowed to open.
func (m *streamsMap) UpdateLimits(p *wire.TransportParameters) {
m.outgoingBidiStreams.SetMaxStream(p.MaxBidiStreamNum)
m.outgoingUniStreams.SetMaxStream(p.MaxUniStreamNum)
return nil
}

func (m *streamsMap) CloseWithError(err error) {
Expand Down
17 changes: 2 additions & 15 deletions streams_map_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/lucas-clemente/quic-go/internal/flowcontrol"
"github.com/lucas-clemente/quic-go/internal/mocks"
"github.com/lucas-clemente/quic-go/internal/protocol"
"github.com/lucas-clemente/quic-go/internal/qerr"
"github.com/lucas-clemente/quic-go/internal/wire"

. "github.com/onsi/ginkgo"
Expand Down Expand Up @@ -330,10 +329,10 @@ var _ = Describe("Streams Map", func() {
m.perspective = pers
_, err := m.OpenStream()
expectTooManyStreamsError(err)
Expect(m.UpdateLimits(&wire.TransportParameters{
m.UpdateLimits(&wire.TransportParameters{
MaxBidiStreamNum: 5,
MaxUniStreamNum: 8,
})).To(Succeed())
})

mockSender.EXPECT().queueControlFrame(gomock.Any()).Times(2)
// test we can only 5 bidirectional streams
Expand All @@ -354,18 +353,6 @@ var _ = Describe("Streams Map", func() {
expectTooManyStreamsError(err)
})
}

It("rejects parameters with too large unidirectional stream counts", func() {
Expect(m.UpdateLimits(&wire.TransportParameters{
MaxUniStreamNum: protocol.MaxStreamCount + 1,
})).To(MatchError(qerr.StreamLimitError))
})

It("rejects parameters with too large unidirectional stream counts", func() {
Expect(m.UpdateLimits(&wire.TransportParameters{
MaxBidiStreamNum: protocol.MaxStreamCount + 1,
})).To(MatchError(qerr.StreamLimitError))
})
})

Context("handling MAX_STREAMS frames", func() {
Expand Down