From fd8a2786d47c946f0b956003a3b7a037f7322757 Mon Sep 17 00:00:00 2001 From: Guillaume Ballet Date: Fri, 23 Mar 2018 16:35:50 +0100 Subject: [PATCH 1/5] whisper: fix issue in topic list copy Fixes #16271. What was appeneded was a pointer to an object that changes during the iteration. --- whisper/whisperv6/api.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/whisper/whisperv6/api.go b/whisper/whisperv6/api.go index 96e2b17e7cf0..74b8873d1019 100644 --- a/whisper/whisperv6/api.go +++ b/whisper/whisperv6/api.go @@ -558,9 +558,10 @@ func (api *PublicWhisperAPI) NewMessageFilter(req Criteria) (string, error) { } if len(req.Topics) > 0 { - topics = make([][]byte, 0, len(req.Topics)) - for _, topic := range req.Topics { - topics = append(topics, topic[:]) + topics = make([][]byte, len(req.Topics)) + for i, topic := range req.Topics { + topics = append(topics, make([]byte, 0, len(topic))) + copy(topics[i], topic[:]) } } From 9f013f98e9721e6b745b5ef64c7e33fe70af4554 Mon Sep 17 00:00:00 2001 From: Guillaume Ballet Date: Mon, 26 Mar 2018 16:43:18 +0200 Subject: [PATCH 2/5] whisper: fix memalloc issue that slipped in --- whisper/whisperv6/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/whisper/whisperv6/api.go b/whisper/whisperv6/api.go index 74b8873d1019..2e2fdc6955c1 100644 --- a/whisper/whisperv6/api.go +++ b/whisper/whisperv6/api.go @@ -560,7 +560,7 @@ func (api *PublicWhisperAPI) NewMessageFilter(req Criteria) (string, error) { if len(req.Topics) > 0 { topics = make([][]byte, len(req.Topics)) for i, topic := range req.Topics { - topics = append(topics, make([]byte, 0, len(topic))) + topics[i] = make([]byte, 0, len(topic)) copy(topics[i], topic[:]) } } From 090b2b0c9b603bec744a64d9c6a1da03966fccb6 Mon Sep 17 00:00:00 2001 From: Guillaume Ballet Date: Mon, 26 Mar 2018 22:29:37 +0200 Subject: [PATCH 3/5] whisper: add a unit test and fix some copy issue Copying to an empty slice will not change the size of the slice. --- whisper/whisperv6/api.go | 2 +- whisper/whisperv6/api_test.go | 78 +++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 whisper/whisperv6/api_test.go diff --git a/whisper/whisperv6/api.go b/whisper/whisperv6/api.go index 2e2fdc6955c1..f75e50247345 100644 --- a/whisper/whisperv6/api.go +++ b/whisper/whisperv6/api.go @@ -560,7 +560,7 @@ func (api *PublicWhisperAPI) NewMessageFilter(req Criteria) (string, error) { if len(req.Topics) > 0 { topics = make([][]byte, len(req.Topics)) for i, topic := range req.Topics { - topics[i] = make([]byte, 0, len(topic)) + topics[i] = make([]byte, len(topic)) copy(topics[i], topic[:]) } } diff --git a/whisper/whisperv6/api_test.go b/whisper/whisperv6/api_test.go new file mode 100644 index 000000000000..fcc9e5e91524 --- /dev/null +++ b/whisper/whisperv6/api_test.go @@ -0,0 +1,78 @@ +// Copyright 2018 The go-ethereum Authors +// This file is part of the go-ethereum library. +// +// The go-ethereum library is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// The go-ethereum library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with the go-ethereum library. If not, see . + +package whisperv6 + +import ( + "bytes" + "testing" + "crypto/ecdsa" + "time" + + "github.com/ethereum/go-ethereum/common" + set "gopkg.in/fatih/set.v0" +) + +func TestMultipleTopicCopyInNewMessageFilter(t *testing.T) { + w := &Whisper{ + privateKeys: make(map[string]*ecdsa.PrivateKey), + symKeys: make(map[string][]byte), + envelopes: make(map[common.Hash]*Envelope), + expirations: make(map[uint32]*set.SetNonTS), + peers: make(map[*Peer]struct{}), + messageQueue: make(chan *Envelope, messageQueueLimit), + p2pMsgQueue: make(chan *Envelope, messageQueueLimit), + quit: make(chan struct{}), + syncAllowance: DefaultSyncAllowance, + } + w.filters = NewFilters(w) + + keyID, err := w.GenerateSymKey() + if err != nil { + t.Fatalf("Error generating symmetric key: %v", err) + } + api := PublicWhisperAPI{ + w: w, + lastUsed: make(map[string]time.Time), + } + + t1 := [4]byte{0xde, 0xea, 0xbe, 0xef} + t2 := [4]byte{0xca, 0xfe, 0xde, 0xca} + + crit := Criteria{ + SymKeyID: keyID, + Topics: []TopicType{TopicType(t1), TopicType(t2)}, + } + + _, err = api.NewMessageFilter(crit) + if err != nil { + t.Fatalf("Error creating the filter: %v", err) + } + + found := false + candidates := w.filters.getWatchersByTopic(TopicType(t1)) + for _, f := range candidates { + if len(f.Topics) == 2 { + if bytes.Equal(f.Topics[0], t1[:]) && bytes.Equal(f.Topics[1], t2[:]) { + found = true + } + } + } + + if !found { + t.Fatalf("Could not find filter with both topics") + } +} \ No newline at end of file From 563643e66fd0f871c91a705108f7b767097266ab Mon Sep 17 00:00:00 2001 From: Guillaume Ballet Date: Tue, 27 Mar 2018 09:26:23 +0200 Subject: [PATCH 4/5] whisper: fix linter problems --- whisper/whisperv6/api_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/whisper/whisperv6/api_test.go b/whisper/whisperv6/api_test.go index fcc9e5e91524..004a41c9496a 100644 --- a/whisper/whisperv6/api_test.go +++ b/whisper/whisperv6/api_test.go @@ -18,8 +18,8 @@ package whisperv6 import ( "bytes" - "testing" "crypto/ecdsa" + "testing" "time" "github.com/ethereum/go-ethereum/common" @@ -45,7 +45,7 @@ func TestMultipleTopicCopyInNewMessageFilter(t *testing.T) { t.Fatalf("Error generating symmetric key: %v", err) } api := PublicWhisperAPI{ - w: w, + w: w, lastUsed: make(map[string]time.Time), } @@ -54,7 +54,7 @@ func TestMultipleTopicCopyInNewMessageFilter(t *testing.T) { crit := Criteria{ SymKeyID: keyID, - Topics: []TopicType{TopicType(t1), TopicType(t2)}, + Topics: []TopicType{TopicType(t1), TopicType(t2)}, } _, err = api.NewMessageFilter(crit) @@ -75,4 +75,4 @@ func TestMultipleTopicCopyInNewMessageFilter(t *testing.T) { if !found { t.Fatalf("Could not find filter with both topics") } -} \ No newline at end of file +} From 0fb9691705e33e9825d573bf94bcc4fc72113639 Mon Sep 17 00:00:00 2001 From: Guillaume Ballet Date: Tue, 27 Mar 2018 16:04:03 +0200 Subject: [PATCH 5/5] whisper: Allocate the full topic length in the copy This is so that the partial topic code (currently disabled) does not crash when trying to access byte #3, --- whisper/whisperv6/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/whisper/whisperv6/api.go b/whisper/whisperv6/api.go index f75e50247345..3f3a082afe97 100644 --- a/whisper/whisperv6/api.go +++ b/whisper/whisperv6/api.go @@ -560,7 +560,7 @@ func (api *PublicWhisperAPI) NewMessageFilter(req Criteria) (string, error) { if len(req.Topics) > 0 { topics = make([][]byte, len(req.Topics)) for i, topic := range req.Topics { - topics[i] = make([]byte, len(topic)) + topics[i] = make([]byte, TopicLength) copy(topics[i], topic[:]) } }