From 7969135abc464e3d23a809768ed62f617f20624f Mon Sep 17 00:00:00 2001
From: Robert Zaremba <robert@zaremba.ch>
Date: Wed, 24 Feb 2021 13:47:32 +0100
Subject: [PATCH] rework unsafe string -> bytes conversion #8674 (#8684)

Changing the unsafe string -> bytes conversion to fix
a security concern.

Closes: #8670

Co-authored-by: Alessio Treglia <alessio@tendermint.com>
---
 types/address/hash.go      | 11 +++++------
 types/address/hash_test.go | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/types/address/hash.go b/types/address/hash.go
index d6a249a8a2..ddfec81dff 100644
--- a/types/address/hash.go
+++ b/types/address/hash.go
@@ -64,11 +64,10 @@ func Module(moduleName string, key []byte) []byte {
 	return Hash("module", append(mKey, key...))
 }
 
-// unsafeStrToByteArray uses unsafe to convert string into byte array. Returned array
-// cannot be altered after this functions is called
+// unsafeStrToByteArray uses unsafe to convert string into byte array. Returned bytes
+// must not be altered after this function is called as it will cause a segmentation fault.
 func unsafeStrToByteArray(s string) []byte {
-	sh := *(*reflect.SliceHeader)(unsafe.Pointer(&s)) // nolint:govet
-	sh.Cap = sh.Len
-	bs := *(*[]byte)(unsafe.Pointer(&sh)) // nolint:govet
-	return bs
+	var buf = *(*[]byte)(unsafe.Pointer(&s))
+	(*reflect.SliceHeader)(unsafe.Pointer(&buf)).Cap = len(s)
+	return buf
 }
diff --git a/types/address/hash_test.go b/types/address/hash_test.go
index be096c357f..4cc6c68806 100644
--- a/types/address/hash_test.go
+++ b/types/address/hash_test.go
@@ -2,7 +2,9 @@ package address
 
 import (
 	"crypto/sha256"
+	"runtime"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/suite"
@@ -75,6 +77,23 @@ func (suite *AddressSuite) TestModule() {
 	assert.NotEqual(addr2, addr3, "changing key must change address")
 }
 
+func unsafeConvertABC() []byte {
+	return unsafeStrToByteArray("abc")
+}
+
+func (suite *AddressSuite) TestUnsafeStrToBytes() {
+	// we convert in other function to trigger GC. We want to check that
+	// the underlying array in []bytes is accessible after GC will finish swapping.
+	for i := 0; i < 5; i++ {
+		b := unsafeConvertABC()
+		runtime.GC()
+		<-time.NewTimer(2 * time.Millisecond).C
+		b2 := append(b, 'd')
+		suite.Equal("abc", string(b))
+		suite.Equal("abcd", string(b2))
+	}
+}
+
 type addrMock struct {
 	Addr []byte
 }