From 9c4a0baf6d72d9bf9892eeace0f836d4809542a5 Mon Sep 17 00:00:00 2001 From: Jeromy Date: Fri, 29 Apr 2016 14:16:41 -0700 Subject: [PATCH 1/6] cleanup panics and make NewFromBytes faster --- codec.go | 98 ++++++++++++++++++++++++++++++----------------- multiaddr.go | 19 +++++---- multiaddr_test.go | 20 ++++++++++ protocols.go | 11 ++++-- 4 files changed, 100 insertions(+), 48 deletions(-) diff --git a/codec.go b/codec.go index 64e7d04..e05bbb8 100644 --- a/codec.go +++ b/codec.go @@ -3,7 +3,6 @@ package multiaddr import ( "encoding/base32" "encoding/binary" - "errors" "fmt" "net" "strconv" @@ -52,27 +51,42 @@ func stringToBytes(s string) ([]byte, error) { return b, nil } -func bytesToString(b []byte) (ret string, err error) { +func validateBytes(b []byte) (err error) { // panic handler, in case we try accessing bytes incorrectly. - defer func() { - if e := recover(); e != nil { - ret = "" - switch e := e.(type) { - case error: - err = e - case string: - err = errors.New(e) - default: - err = fmt.Errorf("%v", e) - } + for len(b) > 0 { + code, n, err := ReadVarintCode(b) + b = b[n:] + p := ProtocolWithCode(code) + if p.Code == 0 { + return fmt.Errorf("no protocol with code %d", code) } - }() + if p.Size == 0 { + continue + } + + size, err := sizeForAddr(p, b) + if err != nil { + return err + } + + if len(b) < size { + return fmt.Errorf("invalid value for size") + } + b = b[size:] + } + + return nil +} +func bytesToString(b []byte) (ret string, err error) { s := "" for len(b) > 0 { + code, n, err := ReadVarintCode(b) + if err != nil { + return "", err + } - code, n := ReadVarintCode(b) b = b[n:] p := ProtocolWithCode(code) if p.Code == 0 { @@ -84,7 +98,11 @@ func bytesToString(b []byte) (ret string, err error) { continue } - size := sizeForAddr(p, b) + size, err := sizeForAddr(p, b) + if err != nil { + return "", err + } + a, err := addressBytesToString(p, b[:size]) if err != nil { return "", err @@ -98,36 +116,40 @@ func bytesToString(b []byte) (ret string, err error) { return s, nil } -func sizeForAddr(p Protocol, b []byte) int { +func sizeForAddr(p Protocol, b []byte) (int, error) { switch { case p.Size > 0: - return (p.Size / 8) + return (p.Size / 8), nil case p.Size == 0: - return 0 + return 0, nil default: - size, n := ReadVarintCode(b) - return size + n + size, n, err := ReadVarintCode(b) + if err != nil { + return 0, err + } + + return size + n, nil } } -func bytesSplit(b []byte) (ret [][]byte, err error) { - // panic handler, in case we try accessing bytes incorrectly. - defer func() { - if e := recover(); e != nil { - ret = [][]byte{} - err = e.(error) - } - }() - - ret = [][]byte{} +func bytesSplit(b []byte) ([][]byte, error) { + var ret [][]byte for len(b) > 0 { - code, n := ReadVarintCode(b) + code, n, err := ReadVarintCode(b) + if err != nil { + return nil, err + } + p := ProtocolWithCode(code) if p.Code == 0 { - return [][]byte{}, fmt.Errorf("no protocol with code %d", b[0]) + return nil, fmt.Errorf("no protocol with code %d", b[0]) + } + + size, err := sizeForAddr(p, b[n:]) + if err != nil { + return nil, err } - size := sizeForAddr(p, b[n:]) length := n + size ret = append(ret, b[:length]) b = b[length:] @@ -228,10 +250,14 @@ func addressBytesToString(p Protocol, b []byte) (string, error) { case P_IPFS: // ipfs // the address is a varint-prefixed multihash string representation - size, n := ReadVarintCode(b) + size, n, err := ReadVarintCode(b) + if err != nil { + return "", err + } + b = b[n:] if len(b) != size { - panic("inconsistent lengths") + return "", fmt.Errorf("inconsistent lengths") } m, err := mh.Cast(b) if err != nil { diff --git a/multiaddr.go b/multiaddr.go index 0dbcede..be04a7f 100644 --- a/multiaddr.go +++ b/multiaddr.go @@ -23,11 +23,11 @@ func NewMultiaddr(s string) (Multiaddr, error) { // NewMultiaddrBytes initializes a Multiaddr from a byte representation. // It validates it as an input string. func NewMultiaddrBytes(b []byte) (Multiaddr, error) { - s, err := bytesToString(b) - if err != nil { + if err := validateBytes(b); err != nil { return nil, err } - return NewMultiaddr(s) + + return &multiaddr{bytes: b}, nil } // Equal tests whether two multiaddrs are equal @@ -64,11 +64,14 @@ func (m *multiaddr) Protocols() []Protocol { } }() - size := 0 - ps := []Protocol{} - b := m.bytes[:] + var ps []Protocol + b := m.bytes for len(b) > 0 { - code, n := ReadVarintCode(b) + code, n, err := ReadVarintCode(b) + if err != nil { + panic(err) + } + p := ProtocolWithCode(code) if p.Code == 0 { // this is a panic (and not returning err) because this should've been @@ -78,7 +81,7 @@ func (m *multiaddr) Protocols() []Protocol { ps = append(ps, p) b = b[n:] - size = sizeForAddr(p, b) + size, err := sizeForAddr(p, b) b = b[size:] } return ps diff --git a/multiaddr_test.go b/multiaddr_test.go index 7c75274..ac81ac2 100644 --- a/multiaddr_test.go +++ b/multiaddr_test.go @@ -3,7 +3,9 @@ package multiaddr import ( "bytes" "encoding/hex" + "math/rand" "testing" + "time" ) func newMultiaddr(t *testing.T, a string) Multiaddr { @@ -342,3 +344,21 @@ func TestGetValue(t *testing.T) { assertValueForProto(t, a, P_UDP, "12345") assertValueForProto(t, a, P_UTP, "") } + +func TestFuzzBytes(t *testing.T) { + rand.Seed(time.Now().UnixNano()) + // Bump up these numbers if you want to stress this + buf := make([]byte, 256) + for i := 0; i < 2000; i++ { + l := rand.Intn(len(buf)) + rand.Read(buf[:l]) + + // just checking that it doesnt panic + ma, err := NewMultiaddrBytes(buf[:l]) + if err == nil { + // for any valid multiaddrs, make sure these calls don't panic + ma.String() + ma.Protocols() + } + } +} diff --git a/protocols.go b/protocols.go index 8364d4c..7454590 100644 --- a/protocols.go +++ b/protocols.go @@ -117,16 +117,19 @@ func CodeToVarint(num int) []byte { // VarintToCode converts a varint-encoded []byte to an integer protocol code func VarintToCode(buf []byte) int { - num, _ := ReadVarintCode(buf) + num, _, err := ReadVarintCode(buf) + if err != nil { + panic(err) + } return num } // ReadVarintCode reads a varint code from the beginning of buf. // returns the code, and the number of bytes read. -func ReadVarintCode(buf []byte) (int, int) { +func ReadVarintCode(buf []byte) (int, int, error) { num, n := binary.Uvarint(buf) if n < 0 { - panic("varints larger than uint64 not yet supported") + return 0, 0, fmt.Errorf("varints larger than uint64 not yet supported") } - return int(num), n + return int(num), n, nil } From 256afede6e6810f2375e07a25496590b0033bfe1 Mon Sep 17 00:00:00 2001 From: Jeromy Date: Mon, 2 May 2016 09:17:03 -0700 Subject: [PATCH 2/6] use newer go versions --- .travis.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 7b571f4..4475888 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,9 +1,8 @@ language: go go: - - 1.3 - - release - tip + - 1.6.1 script: - go test -race -cpu=5 -v ./... From 9e13209db1783c48ec19e8b3e606c5a8684ba8b3 Mon Sep 17 00:00:00 2001 From: Jeromy Date: Wed, 4 May 2016 10:51:57 -0700 Subject: [PATCH 3/6] fix underflow error found by fuzzing --- codec.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codec.go b/codec.go index e05bbb8..5193d5b 100644 --- a/codec.go +++ b/codec.go @@ -70,9 +70,10 @@ func validateBytes(b []byte) (err error) { return err } - if len(b) < size { + if len(b) < size || size < 0 { return fmt.Errorf("invalid value for size") } + b = b[size:] } From 91752fd546e12362989c5eb53457e6876830c9b2 Mon Sep 17 00:00:00 2001 From: Jeromy Date: Wed, 4 May 2016 12:11:09 -0700 Subject: [PATCH 4/6] a bit more cleanup, use a bytes.Buffer instead of appending bytes --- codec.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/codec.go b/codec.go index 5193d5b..747f160 100644 --- a/codec.go +++ b/codec.go @@ -1,6 +1,7 @@ package multiaddr import ( + "bytes" "encoding/base32" "encoding/binary" "fmt" @@ -16,7 +17,7 @@ func stringToBytes(s string) ([]byte, error) { // consume trailing slashes s = strings.TrimRight(s, "/") - b := []byte{} + b := new(bytes.Buffer) sp := strings.Split(s, "/") if sp[0] != "" { @@ -31,7 +32,7 @@ func stringToBytes(s string) ([]byte, error) { if p.Code == 0 { return nil, fmt.Errorf("no protocol with name %s", sp[0]) } - b = append(b, CodeToVarint(p.Code)...) + b.Write(CodeToVarint(p.Code)) sp = sp[1:] if p.Size == 0 { // no length. @@ -41,18 +42,19 @@ func stringToBytes(s string) ([]byte, error) { if len(sp) < 1 { return nil, fmt.Errorf("protocol requires address, none given: %s", p.Name) } + a, err := addressStringToBytes(p, sp[0]) if err != nil { return nil, fmt.Errorf("failed to parse %s: %s %s", p.Name, sp[0], err) } - b = append(b, a...) + b.Write(a) sp = sp[1:] } - return b, nil + + return b.Bytes(), nil } func validateBytes(b []byte) (err error) { - // panic handler, in case we try accessing bytes incorrectly. for len(b) > 0 { code, n, err := ReadVarintCode(b) b = b[n:] @@ -104,6 +106,10 @@ func bytesToString(b []byte) (ret string, err error) { return "", err } + if len(b) < size || size < 0 { + return "", fmt.Errorf("invalid value for size") + } + a, err := addressBytesToString(p, b[:size]) if err != nil { return "", err @@ -265,7 +271,7 @@ func addressBytesToString(p Protocol, b []byte) (string, error) { return "", err } return m.B58String(), nil + default: + return "", fmt.Errorf("unknown protocol") } - - return "", fmt.Errorf("unknown protocol") } From ad12fa30fe8ee0e2673a1305a64bf39786919521 Mon Sep 17 00:00:00 2001 From: Jeromy Date: Wed, 4 May 2016 13:26:50 -0700 Subject: [PATCH 5/6] respond to CR feedback --- codec.go | 5 +++++ multiaddr.go | 22 ++++++++++++++++++++-- multiaddr_test.go | 43 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/codec.go b/codec.go index 747f160..554ed4e 100644 --- a/codec.go +++ b/codec.go @@ -57,6 +57,10 @@ func stringToBytes(s string) ([]byte, error) { func validateBytes(b []byte) (err error) { for len(b) > 0 { code, n, err := ReadVarintCode(b) + if err != nil { + return err + } + b = b[n:] p := ProtocolWithCode(code) if p.Code == 0 { @@ -81,6 +85,7 @@ func validateBytes(b []byte) (err error) { return nil } + func bytesToString(b []byte) (ret string, err error) { s := "" diff --git a/multiaddr.go b/multiaddr.go index be04a7f..430cb1d 100644 --- a/multiaddr.go +++ b/multiaddr.go @@ -3,6 +3,7 @@ package multiaddr import ( "bytes" "fmt" + "log" "strings" ) @@ -12,7 +13,13 @@ type multiaddr struct { } // NewMultiaddr parses and validates an input string, returning a *Multiaddr -func NewMultiaddr(s string) (Multiaddr, error) { +func NewMultiaddr(s string) (a Multiaddr, err error) { + defer func() { + if e := recover(); e != nil { + log.Printf("Panic in NewMultiaddr on input %q: %s", s, e) + err = fmt.Errorf("%v", e) + } + }() b, err := stringToBytes(s) if err != nil { return nil, err @@ -22,7 +29,14 @@ func NewMultiaddr(s string) (Multiaddr, error) { // NewMultiaddrBytes initializes a Multiaddr from a byte representation. // It validates it as an input string. -func NewMultiaddrBytes(b []byte) (Multiaddr, error) { +func NewMultiaddrBytes(b []byte) (a Multiaddr, err error) { + defer func() { + if e := recover(); e != nil { + log.Printf("Panic in NewMultiaddrBytes on input %q: %s", b, e) + err = fmt.Errorf("%v", e) + } + }() + if err := validateBytes(b); err != nil { return nil, err } @@ -82,6 +96,10 @@ func (m *multiaddr) Protocols() []Protocol { b = b[n:] size, err := sizeForAddr(p, b) + if err != nil { + panic(err) + } + b = b[size:] } return ps diff --git a/multiaddr_test.go b/multiaddr_test.go index ac81ac2..d469451 100644 --- a/multiaddr_test.go +++ b/multiaddr_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/hex" "math/rand" + "strings" "testing" "time" ) @@ -140,6 +141,10 @@ func TestStringToBytes(t *testing.T) { if !bytes.Equal(b1, b2) { t.Error("failed to convert", s, "to", b1, "got", b2) } + + if err := validateBytes(b2); err != nil { + t.Error(err) + } } testString("/ip4/127.0.0.1/udp/1234", "047f0000011104d2") @@ -155,6 +160,10 @@ func TestBytesToString(t *testing.T) { t.Error("failed to decode hex", h) } + if err := validateBytes(b); err != nil { + t.Error(err) + } + s2, err := bytesToString(b) if err != nil { t.Error("failed to convert", b) @@ -362,3 +371,37 @@ func TestFuzzBytes(t *testing.T) { } } } + +func randMaddrString() string { + good_corpus := []string{"tcp", "ip", "udp", "ipfs", "0.0.0.0", "127.0.0.1", "12345", "QmbHVEEepCi7rn7VL7Exxpd2Ci9NNB6ifvqwhsrbRMgQFP"} + + size := rand.Intn(256) + parts := make([]string, 0, size) + for i := 0; i < size; i++ { + switch rand.Intn(5) { + case 0, 1, 2: + parts = append(parts, good_corpus[rand.Intn(len(good_corpus))]) + default: + badbuf := make([]byte, rand.Intn(256)) + rand.Read(badbuf) + parts = append(parts, string(badbuf)) + } + } + + return "/" + strings.Join(parts, "/") +} + +func TestFuzzString(t *testing.T) { + rand.Seed(time.Now().UnixNano()) + // Bump up these numbers if you want to stress this + for i := 0; i < 2000; i++ { + + // just checking that it doesnt panic + ma, err := NewMultiaddr(randMaddrString()) + if err == nil { + // for any valid multiaddrs, make sure these calls don't panic + ma.String() + ma.Protocols() + } + } +} From 5734f44c3de57a1df229a87bf8fc9bc2acf28c92 Mon Sep 17 00:00:00 2001 From: Jeromy Date: Wed, 4 May 2016 14:03:16 -0700 Subject: [PATCH 6/6] go vet --- multiaddr_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/multiaddr_test.go b/multiaddr_test.go index d469451..6c7d772 100644 --- a/multiaddr_test.go +++ b/multiaddr_test.go @@ -257,7 +257,7 @@ func TestProtocolsWithString(t *testing.T) { for s, ps1 := range good { ps2, err := ProtocolsWithString(s) if err != nil { - t.Error("ProtocolsWithString(%s) should have succeeded", s) + t.Errorf("ProtocolsWithString(%s) should have succeeded", s) } for i, ps1p := range ps1 { @@ -277,7 +277,7 @@ func TestProtocolsWithString(t *testing.T) { for _, s := range bad { if _, err := ProtocolsWithString(s); err == nil { - t.Error("ProtocolsWithString(%s) should have failed", s) + t.Errorf("ProtocolsWithString(%s) should have failed", s) } } @@ -320,7 +320,7 @@ func assertValueForProto(t *testing.T, a Multiaddr, p int, exp string) { } if fv != exp { - t.Fatalf("expected %q for %d in %d, but got %q instead", exp, p, a, fv) + t.Fatalf("expected %q for %d in %s, but got %q instead", exp, p, a, fv) } } @@ -366,7 +366,7 @@ func TestFuzzBytes(t *testing.T) { ma, err := NewMultiaddrBytes(buf[:l]) if err == nil { // for any valid multiaddrs, make sure these calls don't panic - ma.String() + _ = ma.String() ma.Protocols() } } @@ -400,7 +400,7 @@ func TestFuzzString(t *testing.T) { ma, err := NewMultiaddr(randMaddrString()) if err == nil { // for any valid multiaddrs, make sure these calls don't panic - ma.String() + _ = ma.String() ma.Protocols() } }