From 5a1fc1b94d44c7f3c968aac54f5a51619e0b2a14 Mon Sep 17 00:00:00 2001 From: Sarah Jamie Lewis Date: Thu, 7 Nov 2019 16:11:14 -0800 Subject: [PATCH] Fixing Race Conditions --- .drone.yml | 2 +- application/application.go | 2 ++ connection/connection.go | 9 +++++++-- connection/connection_test.go | 3 +++ testing/tests.sh | 15 ++++++++------- utils/networking.go | 8 ++++++++ 6 files changed, 29 insertions(+), 10 deletions(-) diff --git a/.drone.yml b/.drone.yml index 5c40d3e..0c4d295 100644 --- a/.drone.yml +++ b/.drone.yml @@ -33,7 +33,7 @@ pipeline: commands: - ./tor -f ./torrc - sleep 15 - - go test -v git.openprivacy.ca/openprivacy/libricochet-go/testing + - go test -race -v git.openprivacy.ca/openprivacy/libricochet-go/testing notify-email: image: drillster/drone-email host: build.openprivacy.ca diff --git a/application/application.go b/application/application.go index 2c58860..ee3d009 100644 --- a/application/application.go +++ b/application/application.go @@ -144,7 +144,9 @@ func (ra *RicochetApplication) Run(ls connectivity.ListenService) { if !ra.v3identity.Initialized() || ra.contactManager == nil { return } + ra.lock.Lock() ra.ls = ls + ra.lock.Unlock() var err error for err == nil { conn, err := ra.ls.Accept() diff --git a/connection/connection.go b/connection/connection.go index 347c131..b038842 100644 --- a/connection/connection.go +++ b/connection/connection.go @@ -32,8 +32,9 @@ type Connection struct { messageBuilder utils.MessageBuilder - closed bool - closing bool + closed bool + closingLock sync.Mutex + closing bool // This mutex is exclusively for preventing races during blocking // interactions with Process; specifically Do and Break. Don't use // it for anything else. See those functions for an explanation. @@ -311,6 +312,8 @@ func (rc *Connection) Process(handler Handler) error { go func() { rc.processBlockMutex.Lock() defer rc.processBlockMutex.Unlock() + rc.closingLock.Lock() + defer rc.closingLock.Unlock() rc.closed = true close(closedChan) }() @@ -468,5 +471,7 @@ func (rc *Connection) Close() { // Kill the Ricochet Connection. log.Debugf("Closing Ricochet Connection for %v", rc.RemoteHostname) rc.conn.Close() + rc.closingLock.Lock() rc.closed = true + rc.closingLock.Unlock() } diff --git a/connection/connection_test.go b/connection/connection_test.go index 3aaa349..aea106a 100644 --- a/connection/connection_test.go +++ b/connection/connection_test.go @@ -53,6 +53,9 @@ func TestProcessAuthAs3DHServer(t *testing.T) { t.Errorf("Error while testing ProcessAuthAsServer: %v", err) } + // Wait for server to finish + time.Sleep(time.Second * 2) + // Test Close rc.Close() } diff --git a/testing/tests.sh b/testing/tests.sh index 4091b23..ebdd5b4 100755 --- a/testing/tests.sh +++ b/testing/tests.sh @@ -4,13 +4,14 @@ set -e pwd -go test ${1} -coverprofile=utils.cover.out -v ./utils -go test ${1} -coverprofile=channels.cover.out -v ./channels -go test ${1} -coverprofile=channels.v3.inbound.cover.out -v ./channels/v3/inbound -go test ${1} -coverprofile=connection.cover.out -v ./connection -go test ${1} -coverprofile=policies.cover.out -v ./policies -go test ${1} -coverprofile=identity.cover.out -v ./identity -go test ${1} -coverprofile=root.cover.out -v ./ +GORACE="haltonerror=1" +go test -race ${1} -coverprofile=utils.cover.out -v ./utils +go test -race ${1} -coverprofile=channels.cover.out -v ./channels +go test -race ${1} -coverprofile=channels.v3.inbound.cover.out -v ./channels/v3/inbound +go test -race ${1} -coverprofile=connection.cover.out -v ./connection +go test -race ${1} -coverprofile=policies.cover.out -v ./policies +go test -race ${1} -coverprofile=identity.cover.out -v ./identity +go test -race ${1} -coverprofile=root.cover.out -v ./ echo "mode: set" > coverage.out && cat *.cover.out | grep -v mode: | sort -r | \ awk '{if($1 != last) {print $0;last=$1}}' >> coverage.out rm -rf *.cover.out diff --git a/utils/networking.go b/utils/networking.go index f3adaeb..605f9e7 100644 --- a/utils/networking.go +++ b/utils/networking.go @@ -8,6 +8,7 @@ import ( "git.openprivacy.ca/openprivacy/libricochet-go/log" "golang.org/x/crypto/nacl/secretbox" "io" + "sync" ) const ( @@ -43,10 +44,13 @@ type RicochetNetwork struct { // Derived ephemeral session key for connection key [32]byte encrypt bool + lock sync.Mutex } // SetEncryptionKey sets the ephemeral encryption key for this session. func (rn *RicochetNetwork) SetEncryptionKey(key [32]byte) { + rn.lock.Lock() + defer rn.lock.Unlock() log.Debugf("turning on ephemeral session encryption for connection") copy(rn.key[:], key[:]) @@ -67,6 +71,7 @@ func (rn *RicochetNetwork) SendRicochetPacket(dst io.Writer, channel int32, data binary.BigEndian.PutUint16(packet[2:4], uint16(channel)) copy(packet[4:], data[:]) + rn.lock.Lock() if rn.encrypt { var nonce [24]byte if _, err := io.ReadFull(rand.Reader, nonce[:]); err != nil { @@ -77,6 +82,7 @@ func (rn *RicochetNetwork) SendRicochetPacket(dst io.Writer, channel int32, data binary.BigEndian.PutUint16(packet[0:2], uint16(len(encrypted)+2)) packet = append(packet[0:2], encrypted...) } + rn.lock.Unlock() for pos := 0; pos < len(packet); { n, err := dst.Write(packet[pos:]) @@ -112,6 +118,7 @@ func (rn *RicochetNetwork) RecvRicochetPacket(reader io.Reader) (RicochetData, e return packet, err } + rn.lock.Lock() if rn.encrypt { var decryptNonce [24]byte if len(packetBytes) > 24 { @@ -127,6 +134,7 @@ func (rn *RicochetNetwork) RecvRicochetPacket(reader io.Reader) (RicochetData, e return packet, errors.New("ciphertext length was too short") } } + rn.lock.Unlock() packet.Channel = int32(binary.BigEndian.Uint16(packetBytes[0:2])) packet.Data = make([]byte, len(packetBytes)-2)