From 88937ab0f73c8e6ba75566ead88dbeca4e8c30c7 Mon Sep 17 00:00:00 2001 From: Edd Robinson Date: Tue, 16 Feb 2016 12:15:26 +0000 Subject: [PATCH 1/3] Fixes #5664 --- models/points.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/points.go b/models/points.go index 394b8f6997f..6df1b22184c 100644 --- a/models/points.go +++ b/models/points.go @@ -888,8 +888,8 @@ func scanTo(buf []byte, i int, stop byte) (int, []byte) { break } - // reached end of block? - if buf[i] == stop && buf[i-1] != '\\' { + // Reached unescaped stop value? + if buf[i] == stop && (i == 0 || buf[i-1] != '\\') { break } i++ From bfb361e85417ecbc5ddb3d0a30ed496622bdd313 Mon Sep 17 00:00:00 2001 From: Jon Seymour Date: Thu, 18 Feb 2016 02:47:29 +1100 Subject: [PATCH 2/3] models: add tests for point construction These tests check that NewPoint and NewPointFromBytes return an error if: * arguments specify a point with no fields * arguments specify a point with a field that has an empty name These tests also check that Point.Fields() always returns, even in the presence of corrupt binary data read with NewPointFromBytes. These tests fail at this commit and are fixed by a subsequent commit. Signed-off-by: Jon Seymour --- models/points_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/models/points_test.go b/models/points_test.go index db3065ae338..b8afae9ee5c 100644 --- a/models/points_test.go +++ b/models/points_test.go @@ -1772,3 +1772,27 @@ t159,label=another a=2i,value=1i 1` t.Fatalf("expected 2 points, got %d", len(points)) } } + +func TestNewPointsWithBytesWithCorruptData(t *testing.T) { + ch := make(chan error) + go func() { + corrupted := []byte{0, 0, 0, 3, 102, 111, 111, 0, 0, 0, 4, 61, 34, 65, 34, 1, 0, 0, 0, 14, 206, 86, 119, 24, 32, 72, 233, 168, 2, 148} + p, err := models.NewPointFromBytes(corrupted) + p.Fields() // this method should always return, even if the data is corrupt. + ch <- err + }() + select { + case err := <-ch: + if err != nil { + t.Fatalf("unexpected error: got: %v, expected: nil", err) + } + case _ = <-time.NewTimer(time.Second).C: + t.Fatalf("probable infite loop. got: timeout, expected: return") + } +} + +func TestNewPointsRejectsEmptyFieldNames(t *testing.T) { + if _, err := models.NewPoint("foo", nil, models.Fields{"": 1}, time.Now()); err == nil { + t.Fatalf("new point with empty field name. got: nil, expected: error") + } +} From 9491846047859774273faae66f60640dd531fe30 Mon Sep 17 00:00:00 2001 From: Jon Seymour Date: Sat, 20 Feb 2016 22:22:26 +1100 Subject: [PATCH 3/3] models: improve handling of points with empty field names or with no fields Influx does not support fields with empty names or points with no fields. NewPoint is changed to validate that all field names are non-empty. AddField is removed because we now require that all fields are specified on construction. NewPointFromByte is changed to return an error if a unmarshaled binary point does not have any fields. newFieldsFromBinary is changed to prevent an infinite loop that can arise while attempting to parse corrupt binary point data. TestNewPointsWithBytesWithCorruptData is changed to reflect the change in the behaviour of NewPointFromByte. Signed-off-by: Jon Seymour --- models/points.go | 67 +++++++++++++++++++++---------------------- models/points_test.go | 18 +++--------- 2 files changed, 37 insertions(+), 48 deletions(-) diff --git a/models/points.go b/models/points.go index 6df1b22184c..336dc4f2836 100644 --- a/models/points.go +++ b/models/points.go @@ -3,6 +3,7 @@ package models import ( "bytes" "encoding/binary" + "errors" "fmt" "hash/fnv" "math" @@ -25,6 +26,8 @@ var ( ' ': []byte(`\ `), '=': []byte(`\=`), } + + ErrPointMustHaveAField = errors.New("point without fields is unsupported") ) // Point defines the values that will be written to the database @@ -37,7 +40,6 @@ type Point interface { SetTags(tags Tags) Fields() Fields - AddField(name string, value interface{}) Time() time.Time SetTime(t time.Time) @@ -1063,7 +1065,7 @@ func unescapeStringField(in string) string { // an unsupported field value (NaN) or out of range time is passed, this function returns an error. func NewPoint(name string, tags Tags, fields Fields, time time.Time) (Point, error) { if len(fields) == 0 { - return nil, fmt.Errorf("Point without fields is unsupported") + return nil, ErrPointMustHaveAField } if !time.IsZero() { if err := CheckTime(time); err != nil { @@ -1078,6 +1080,9 @@ func NewPoint(name string, tags Tags, fields Fields, time time.Time) (Point, err return nil, fmt.Errorf("NaN is an unsupported value for field %s", key) } } + if len(key) == 0 { + return nil, fmt.Errorf("all fields must have non-empty names") + } } return &point{ @@ -1092,6 +1097,9 @@ func NewPointFromBytes(b []byte) (Point, error) { if err := p.UnmarshalBinary(b); err != nil { return nil, err } + if len(p.Fields()) == 0 { + return nil, ErrPointMustHaveAField + } return p, nil } @@ -1212,14 +1220,6 @@ func (p *point) Fields() Fields { return p.cachedFields } -// AddField adds or replaces a field value for a point -func (p *point) AddField(name string, value interface{}) { - fields := p.Fields() - fields[name] = value - p.fields = fields.MarshalBinary() - p.cachedFields = nil -} - // SetPrecision will round a time to the specified precision func (p *point) SetPrecision(precision string) { switch precision { @@ -1394,38 +1394,37 @@ func newFieldsFromBinary(buf []byte) Fields { } i, name = scanTo(buf, i, '=') - if len(name) == 0 { - continue - } name = escape.Unescape(name) i, valueBuf = scanFieldValue(buf, i+1) - if len(valueBuf) == 0 { - fields[string(name)] = nil - continue - } + if len(name) > 0 { + if len(valueBuf) == 0 { + fields[string(name)] = nil + continue + } - // If the first char is a double-quote, then unmarshal as string - if valueBuf[0] == '"' { - value = unescapeStringField(string(valueBuf[1 : len(valueBuf)-1])) - // Check for numeric characters and special NaN or Inf - } else if (valueBuf[0] >= '0' && valueBuf[0] <= '9') || valueBuf[0] == '-' || valueBuf[0] == '+' || valueBuf[0] == '.' || - valueBuf[0] == 'N' || valueBuf[0] == 'n' || // NaN - valueBuf[0] == 'I' || valueBuf[0] == 'i' { // Inf + // If the first char is a double-quote, then unmarshal as string + if valueBuf[0] == '"' { + value = unescapeStringField(string(valueBuf[1 : len(valueBuf)-1])) + // Check for numeric characters and special NaN or Inf + } else if (valueBuf[0] >= '0' && valueBuf[0] <= '9') || valueBuf[0] == '-' || valueBuf[0] == '+' || valueBuf[0] == '.' || + valueBuf[0] == 'N' || valueBuf[0] == 'n' || // NaN + valueBuf[0] == 'I' || valueBuf[0] == 'i' { // Inf - value, err = parseNumber(valueBuf) - if err != nil { - panic(fmt.Sprintf("unable to parse number value '%v': %v", string(valueBuf), err)) - } + value, err = parseNumber(valueBuf) + if err != nil { + panic(fmt.Sprintf("unable to parse number value '%v': %v", string(valueBuf), err)) + } - // Otherwise parse it as bool - } else { - value, err = strconv.ParseBool(string(valueBuf)) - if err != nil { - panic(fmt.Sprintf("unable to parse bool value '%v': %v\n", string(valueBuf), err)) + // Otherwise parse it as bool + } else { + value, err = strconv.ParseBool(string(valueBuf)) + if err != nil { + panic(fmt.Sprintf("unable to parse bool value '%v': %v\n", string(valueBuf), err)) + } } + fields[string(name)] = value } - fields[string(name)] = value i++ } return fields diff --git a/models/points_test.go b/models/points_test.go index b8afae9ee5c..49f6079bfa5 100644 --- a/models/points_test.go +++ b/models/points_test.go @@ -1774,20 +1774,10 @@ t159,label=another a=2i,value=1i 1` } func TestNewPointsWithBytesWithCorruptData(t *testing.T) { - ch := make(chan error) - go func() { - corrupted := []byte{0, 0, 0, 3, 102, 111, 111, 0, 0, 0, 4, 61, 34, 65, 34, 1, 0, 0, 0, 14, 206, 86, 119, 24, 32, 72, 233, 168, 2, 148} - p, err := models.NewPointFromBytes(corrupted) - p.Fields() // this method should always return, even if the data is corrupt. - ch <- err - }() - select { - case err := <-ch: - if err != nil { - t.Fatalf("unexpected error: got: %v, expected: nil", err) - } - case _ = <-time.NewTimer(time.Second).C: - t.Fatalf("probable infite loop. got: timeout, expected: return") + corrupted := []byte{0, 0, 0, 3, 102, 111, 111, 0, 0, 0, 4, 61, 34, 65, 34, 1, 0, 0, 0, 14, 206, 86, 119, 24, 32, 72, 233, 168, 2, 148} + p, err := models.NewPointFromBytes(corrupted) + if p != nil || err == nil { + t.Fatalf("NewPointFromBytes: got: (%v, %v), expected: (nil, error)", p, err) } }