From 77a4bf17ca111096ac79fd6f2064d69cc01146c2 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Thu, 22 Feb 2018 09:20:57 +0800 Subject: [PATCH 1/2] fix race condition in GetOrOpenStream in incoming streams map --- streams_map_incoming_bidi.go | 4 +++- streams_map_incoming_generic.go | 4 +++- streams_map_incoming_uni.go | 4 +++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/streams_map_incoming_bidi.go b/streams_map_incoming_bidi.go index 8a35f044f..e88d6397f 100644 --- a/streams_map_incoming_bidi.go +++ b/streams_map_incoming_bidi.go @@ -69,18 +69,20 @@ func (m *incomingBidiStreamsMap) AcceptStream() (streamI, error) { } func (m *incomingBidiStreamsMap) GetOrOpenStream(id protocol.StreamID) (streamI, error) { + m.mutex.RLock() if id > m.maxStream { + m.mutex.RUnlock() return nil, fmt.Errorf("peer tried to open stream %d (current limit: %d)", id, m.maxStream) } // if the id is smaller than the highest we accepted // * this stream exists in the map, and we can return it, or // * this stream was already closed, then we can return the nil if id <= m.highestStream { - m.mutex.RLock() s := m.streams[id] m.mutex.RUnlock() return s, nil } + m.mutex.RUnlock() m.mutex.Lock() var start protocol.StreamID diff --git a/streams_map_incoming_generic.go b/streams_map_incoming_generic.go index 830b690d9..dc79b496c 100644 --- a/streams_map_incoming_generic.go +++ b/streams_map_incoming_generic.go @@ -67,18 +67,20 @@ func (m *incomingItemsMap) AcceptStream() (item, error) { } func (m *incomingItemsMap) GetOrOpenStream(id protocol.StreamID) (item, error) { + m.mutex.RLock() if id > m.maxStream { + m.mutex.RUnlock() return nil, fmt.Errorf("peer tried to open stream %d (current limit: %d)", id, m.maxStream) } // if the id is smaller than the highest we accepted // * this stream exists in the map, and we can return it, or // * this stream was already closed, then we can return the nil if id <= m.highestStream { - m.mutex.RLock() s := m.streams[id] m.mutex.RUnlock() return s, nil } + m.mutex.RUnlock() m.mutex.Lock() var start protocol.StreamID diff --git a/streams_map_incoming_uni.go b/streams_map_incoming_uni.go index 9091d6357..536b42ddd 100644 --- a/streams_map_incoming_uni.go +++ b/streams_map_incoming_uni.go @@ -69,18 +69,20 @@ func (m *incomingUniStreamsMap) AcceptStream() (receiveStreamI, error) { } func (m *incomingUniStreamsMap) GetOrOpenStream(id protocol.StreamID) (receiveStreamI, error) { + m.mutex.RLock() if id > m.maxStream { + m.mutex.RUnlock() return nil, fmt.Errorf("peer tried to open stream %d (current limit: %d)", id, m.maxStream) } // if the id is smaller than the highest we accepted // * this stream exists in the map, and we can return it, or // * this stream was already closed, then we can return the nil if id <= m.highestStream { - m.mutex.RLock() s := m.streams[id] m.mutex.RUnlock() return s, nil } + m.mutex.RUnlock() m.mutex.Lock() var start protocol.StreamID From 4fa68b07885865d34fa34e3b1fea60b9621b503d Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Thu, 22 Feb 2018 09:21:59 +0800 Subject: [PATCH 2/2] fix race condition in GetStream in outgoing streams map --- streams_map_incoming_bidi.go | 3 +++ streams_map_incoming_generic.go | 3 +++ streams_map_incoming_uni.go | 3 +++ streams_map_outgoing_bidi.go | 3 ++- streams_map_outgoing_generic.go | 3 ++- streams_map_outgoing_uni.go | 3 ++- 6 files changed, 15 insertions(+), 3 deletions(-) diff --git a/streams_map_incoming_bidi.go b/streams_map_incoming_bidi.go index e88d6397f..f7f80a280 100644 --- a/streams_map_incoming_bidi.go +++ b/streams_map_incoming_bidi.go @@ -85,6 +85,9 @@ func (m *incomingBidiStreamsMap) GetOrOpenStream(id protocol.StreamID) (streamI, m.mutex.RUnlock() m.mutex.Lock() + // no need to check the two error conditions from above again + // * maxStream can only increase, so if the id was valid before, it definitely is valid now + // * highestStream is only modified by this function var start protocol.StreamID if m.highestStream == 0 { start = m.nextStream diff --git a/streams_map_incoming_generic.go b/streams_map_incoming_generic.go index dc79b496c..40d7b7500 100644 --- a/streams_map_incoming_generic.go +++ b/streams_map_incoming_generic.go @@ -83,6 +83,9 @@ func (m *incomingItemsMap) GetOrOpenStream(id protocol.StreamID) (item, error) { m.mutex.RUnlock() m.mutex.Lock() + // no need to check the two error conditions from above again + // * maxStream can only increase, so if the id was valid before, it definitely is valid now + // * highestStream is only modified by this function var start protocol.StreamID if m.highestStream == 0 { start = m.nextStream diff --git a/streams_map_incoming_uni.go b/streams_map_incoming_uni.go index 536b42ddd..f9fe97b7e 100644 --- a/streams_map_incoming_uni.go +++ b/streams_map_incoming_uni.go @@ -85,6 +85,9 @@ func (m *incomingUniStreamsMap) GetOrOpenStream(id protocol.StreamID) (receiveSt m.mutex.RUnlock() m.mutex.Lock() + // no need to check the two error conditions from above again + // * maxStream can only increase, so if the id was valid before, it definitely is valid now + // * highestStream is only modified by this function var start protocol.StreamID if m.highestStream == 0 { start = m.nextStream diff --git a/streams_map_outgoing_bidi.go b/streams_map_outgoing_bidi.go index d2c92dec2..e7d9150be 100644 --- a/streams_map_outgoing_bidi.go +++ b/streams_map_outgoing_bidi.go @@ -85,10 +85,11 @@ func (m *outgoingBidiStreamsMap) openStreamImpl() (streamI, error) { } func (m *outgoingBidiStreamsMap) GetStream(id protocol.StreamID) (streamI, error) { + m.mutex.RLock() if id >= m.nextStream { + m.mutex.RUnlock() return nil, qerr.Error(qerr.InvalidStreamID, fmt.Sprintf("peer attempted to open stream %d", id)) } - m.mutex.RLock() s := m.streams[id] m.mutex.RUnlock() return s, nil diff --git a/streams_map_outgoing_generic.go b/streams_map_outgoing_generic.go index 5a2836026..80236c153 100644 --- a/streams_map_outgoing_generic.go +++ b/streams_map_outgoing_generic.go @@ -86,10 +86,11 @@ func (m *outgoingItemsMap) openStreamImpl() (item, error) { } func (m *outgoingItemsMap) GetStream(id protocol.StreamID) (item, error) { + m.mutex.RLock() if id >= m.nextStream { + m.mutex.RUnlock() return nil, qerr.Error(qerr.InvalidStreamID, fmt.Sprintf("peer attempted to open stream %d", id)) } - m.mutex.RLock() s := m.streams[id] m.mutex.RUnlock() return s, nil diff --git a/streams_map_outgoing_uni.go b/streams_map_outgoing_uni.go index 77511b780..fd2701b75 100644 --- a/streams_map_outgoing_uni.go +++ b/streams_map_outgoing_uni.go @@ -85,10 +85,11 @@ func (m *outgoingUniStreamsMap) openStreamImpl() (sendStreamI, error) { } func (m *outgoingUniStreamsMap) GetStream(id protocol.StreamID) (sendStreamI, error) { + m.mutex.RLock() if id >= m.nextStream { + m.mutex.RUnlock() return nil, qerr.Error(qerr.InvalidStreamID, fmt.Sprintf("peer attempted to open stream %d", id)) } - m.mutex.RLock() s := m.streams[id] m.mutex.RUnlock() return s, nil