discovery/ble: avoid panic for invalid characteristics
Sometimes Gatt reader reads invalid characteristics causing panic.
I couldn't figure out what the root cause was. It can be a bug in
our code or a bug in Android.
This CL added more checks for invalid characteristics to avoid panic.
Change-Id: I54ad09dacd31181e84ab683ed68204e5a32daaa4
diff --git a/lib/discovery/plugins/ble/encoding.go b/lib/discovery/plugins/ble/encoding.go
index eb9f3fa..3b59a8e 100644
--- a/lib/discovery/plugins/ble/encoding.go
+++ b/lib/discovery/plugins/ble/encoding.go
@@ -108,8 +108,9 @@
return buf.Bytes(), nil
}
-func unpackFromCharacteristics(cs map[string][]byte) [][]byte {
+func unpackFromCharacteristics(cs map[string][]byte) ([][]byte, error) {
var unpacked [][]byte
+ used := 0
for i := 0; i < maxNumPackedServices; i++ {
if _, ok := cs[cUuid(i, 0)]; !ok {
break
@@ -117,11 +118,13 @@
var splitted [][]byte
for j := 0; j < maxNumPackedCharacteristicsPerService; j++ {
- c, ok := cs[cUuid(i, j)]
+ uuid := cUuid(i, j)
+ c, ok := cs[uuid]
if !ok {
break
}
splitted = append(splitted, c)
+ used++
}
if len(splitted) == 1 {
@@ -129,7 +132,10 @@
unpacked = append(unpacked, splitted[0])
} else {
n := 0
- for _, d := range splitted {
+ for j, d := range splitted {
+ if j < len(splitted)-1 && len(d) != maxCharacteristicValueLen {
+ return nil, fmt.Errorf("invalid characteristics: %v", cs)
+ }
n += len(d)
}
merged := make([]byte, n)
@@ -140,7 +146,10 @@
unpacked = append(unpacked, merged)
}
}
- return unpacked
+ if len(cs) != used {
+ return nil, fmt.Errorf("invalid characteristics: %v", cs)
+ }
+ return unpacked, nil
}
func decodeAdInfo(encoded []byte) (*idiscovery.AdInfo, error) {
diff --git a/lib/discovery/plugins/ble/encoding_test.go b/lib/discovery/plugins/ble/encoding_test.go
index 1d5bee3..86c7224 100644
--- a/lib/discovery/plugins/ble/encoding_test.go
+++ b/lib/discovery/plugins/ble/encoding_test.go
@@ -5,6 +5,7 @@
package ble
import (
+ "encoding/hex"
"math/rand"
"reflect"
"testing"
@@ -87,7 +88,11 @@
}
cs := packToCharacteristics(encodedAdinfos)
- unpacked := unpackFromCharacteristics(cs)
+ unpacked, err := unpackFromCharacteristics(cs)
+ if err != nil {
+ t.Errorf("unpack failed: %v", err)
+ continue
+ }
for _, encoded := range unpacked {
adinfo, err := decodeAdInfo(encoded)
@@ -102,3 +107,38 @@
}
}
}
+
+func TestDecodeInvalid(t *testing.T) {
+ tests := []map[string]string{
+ {"1234": "00"}, // Invalid uuid.
+ {"31ca10d5-0195-54fa-9344-25fcd7072f00": "00"}, // Invalid characteristic uuid.
+ {"31ca10d5-0195-54fa-9344-25fcd7072e10": "00"}, // Invalid characteristic uuid sequence.
+ { // Invalid characteristic split.
+ "31ca10d5-0195-54fa-9344-25fcd7072e00": "08eff4d54d979febed94a1b551b0316733762e696f2f782f7265662f73657276696365732f73796e63626173652f7365727665722f696e74657266616365732f53796e63fd01d4ce4d4ec33010c57171205c3b76c633b3321b2e8158f8b30db471459cf4442c90e00270390a4854e204416ff7167ffd5eaf360e5c68ee46322906622296926fe5069c030c9474510a7456bdf15afa10256934600329e92697f222163154ae0c12c92894d8f5706df4d2ca98729b2c3d967ee7230e797e6815515b0029fcf138896daddb7d9ea7fc14ebd8f2d844ac074efe348ac58f3e0df3c16d0f7ed87ffdcebd7c6b4fd3cea9ce0a799e627deee1faa86f172a7542010a044176addce75fee1db3ba5f2bf3e3c22c1925b3c13eb189a570c9bab0eda85b2dfefd2f1e2044feb11bc3d123fc1f7bc8bde5dc81e292348309b85afb67000000ffff01050164026462026462536465762e762e696f3a6f3a3630383934313830383235362d34337674666e6465747337396b6635686163386965756a746f383833373636302e617070732e676f6f676c6575736572636f6e74656e742e636f6d01730c73675f3535313632333837380273626b6465762e762e696f3a6f3a3630383934313830383235362d34337674666e6465747337396b6635686163386965756a746f383833373636302e617070732e676f6f676c6575736572636f6e74",
+ "31ca10d5-0195-54fa-9344-25fcd7072e01": "656e742e636f6d3a6461776e2e76616e616469756d40676d61696c2e636f6d0376697390016465762e762e696f3a6f3a3630383934313830383235362d34337674666e6465747337396b6635686163386965756a746f383833373636302e617070732e676f6f676c6575736572636f6e74656e742e636f6d3a6461776e2e76616e616469756d40676d61696c2e636f6d2c6465762e762e696f3a753a616c657866616e647269616e746f40676f6f676c652e636f6d005cb840d035a10ceaa1e9b7f238c057140000",
+ },
+ { // Invalid characteristic value.
+ "31ca10d5-0195-54fa-9344-25fcd7072e00": "7337396b6635686163386965756a746f383833373636302e617070732e676f6f676c6575736572636f6e74656e742e636f6d3a6461776e2e76616e616469756d40676d61696c2e636f6d0164026462026462536465762e762e696f3a6f3a3630383934313830383235362d34337674666e6465747337396b6635686163386965756a746f383833373636302e617070732e676f6f676c6575736572636f6e74656e742e636f6d01732c6c6973745f6c697374735f4a4d786659727655694d364f596d4f706b356b797350425048324e366b77536f480013c6163e316e5359a9d3ca49fbbc57140000",
+ },
+ }
+
+ for i, test := range tests {
+ cs := make(map[string][]byte)
+ for k, v := range test {
+ c, err := hex.DecodeString(v)
+ if err != nil {
+ t.Fatal(err)
+ }
+ cs[k] = c
+ }
+
+ if unpacked, err := unpackFromCharacteristics(cs); err == nil {
+ for _, encoded := range unpacked {
+ _, err := decodeAdInfo(encoded)
+ if err == nil {
+ t.Errorf("[%d]: expect an error; but got none", i)
+ }
+ }
+ }
+ }
+}
diff --git a/lib/discovery/plugins/ble/scanner.go b/lib/discovery/plugins/ble/scanner.go
index bf0bd43..4f47040 100644
--- a/lib/discovery/plugins/ble/scanner.go
+++ b/lib/discovery/plugins/ble/scanner.go
@@ -145,11 +145,16 @@
func (s *scanner) OnDiscovered(uuid string, characteristics map[string][]byte, rssi int) {
// TODO(jhahn): Add rssi to adinfo.
- unpacked := unpackFromCharacteristics(characteristics)
+ unpacked, err := unpackFromCharacteristics(characteristics)
+ if err != nil {
+ s.ctx.Errorf("failed to unpack characteristics for %v: %v", uuid, err)
+ return
+ }
+
for _, encoded := range unpacked {
adinfo, err := decodeAdInfo(encoded)
if err != nil {
- s.ctx.Error(err)
+ s.ctx.Errorf("failed to decode characteristics for %v: %v", uuid, err)
continue
}
