From b497256dc7c88dec43efa001e70b333b5a716076 Mon Sep 17 00:00:00 2001 From: Jason Wilder Date: Thu, 11 Jun 2015 11:25:25 -0600 Subject: [PATCH 1/4] Fix panic when no value is passed to a field Fixes #2927 --- tsdb/points.go | 13 +++++++++++++ tsdb/points_test.go | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/tsdb/points.go b/tsdb/points.go index 8c4406ed10..09c179debc 100644 --- a/tsdb/points.go +++ b/tsdb/points.go @@ -318,6 +318,19 @@ func scanFields(buf []byte, i int) (int, []byte, error) { continue } + // If we see an =, ensure that there is at least on char after it + if buf[i] == '=' { + // check for "... value=" + if i+1 >= len(buf) { + return i, buf[start:i], fmt.Errorf("missing field value") + } + + // check for "... value=,value2=..." + if buf[i+1] == ',' || buf[i+1] == ' ' { + return i, buf[start:i], fmt.Errorf("missing field value") + } + } + // reached end of block? if buf[i] == ' ' && !quoted { break diff --git a/tsdb/points_test.go b/tsdb/points_test.go index 5f9b1685ea..99707cea35 100644 --- a/tsdb/points_test.go +++ b/tsdb/points_test.go @@ -159,6 +159,24 @@ func TestParsePointMissingTagValue(t *testing.T) { } } +func TestParsePointMissingFieldValue(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=`) + if err == nil { + t.Errorf(`ParsePoints("%s") mismatch. got nil, exp error`, "cpu") + } + + _, err = ParsePointsString(`cpu,host=serverA,region=us-west value= 1000000000`) + if err == nil { + t.Errorf(`ParsePoints("%s") mismatch. got nil, exp error`, "cpu") + } + + _, err = ParsePointsString(`cpu,host=serverA,region=us-west value=,value2=1`) + if err == nil { + t.Errorf(`ParsePoints("%s") mismatch. got nil, exp error`, "cpu") + } + +} + func TestParsePointUnescape(t *testing.T) { // commas in measuremnt name test(t, `cpu\,main,regions=east\,west value=1.0`, From 0d82acfde776d08e1179142c102c440701b91748 Mon Sep 17 00:00:00 2001 From: Jason Wilder Date: Thu, 11 Jun 2015 13:06:09 -0600 Subject: [PATCH 2/4] Prevent invalid numbers from being parsed via line protocol Adds more tests for invalid numbers such as 0.1a, -2.-4, as well test for supported formats for negative and positive integers/floats as well as scientific notation. --- tsdb/points.go | 69 ++++++++++++++++++++++++++++++++++++++-- tsdb/points_test.go | 77 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 2 deletions(-) diff --git a/tsdb/points.go b/tsdb/points.go index 09c179debc..7dbc02bdf2 100644 --- a/tsdb/points.go +++ b/tsdb/points.go @@ -329,6 +329,16 @@ func scanFields(buf []byte, i int) (int, []byte, error) { if buf[i+1] == ',' || buf[i+1] == ' ' { return i, buf[start:i], fmt.Errorf("missing field value") } + + if isNumeric(buf[i+1]) || buf[i+1] == '-' { + var err error + i, _, err = scanNumber(buf, i+1) + if err != nil { + return i, buf[start:i], err + } else { + continue + } + } } // reached end of block? @@ -370,6 +380,62 @@ func scanTime(buf []byte, i int) (int, []byte, error) { return i, buf[start:i], nil } +func isNumeric(b byte) bool { + return (b >= '0' && b <= '9') || b == '.' +} + +// scanNumber returns the end position within buf, start at i after +// scanning over buf for an integer, or float. It returns an +// error if a invalid number is scanned. +func scanNumber(buf []byte, i int) (int, []byte, error) { + start := i + + // Is negative number? + if i < len(buf) && buf[i] == '-' { + i += 1 + } + + decimals := 0 + + for { + if i >= len(buf) { + break + } + + if buf[i] == ',' || buf[i] == ' ' { + break + } + + if buf[i] == '.' { + decimals += 1 + } + + // Can't have more than 1 decimal (e.g. 1.1.1 should fail) + if decimals > 1 { + return i, buf[start:i], fmt.Errorf("invalid number: %s", string(buf[start:i+1])) + } + + // `e` is valid for floats but not as the first char + if i > start && (buf[i] == 'e') { + i += 1 + continue + } + + // + and - are only valid at this point if they follow an e (scientific notation) + if (buf[i] == '+' || buf[i] == '-') && buf[i-1] == 'e' { + i += 1 + continue + } + + if !isNumeric(buf[i]) { + return i, buf[start:i], fmt.Errorf("invalid number: %s", string(buf[start:i+1])) + } + i += 1 + } + + return i, buf[start:i], nil +} + // skipWhitespace returns the end position within buf, starting at i after // scanning over spaces in tags func skipWhitespace(buf []byte, i int) int { @@ -753,8 +819,7 @@ func newFieldsFromBinary(buf []byte) Fields { } else if (valueBuf[0] >= '0' && valueBuf[0] <= '9') || valueBuf[0] == '-' || valueBuf[0] == '.' { value, err = parseNumber(valueBuf) if err != nil { - fmt.Printf("unable to parse number value '%v': %v\n", string(valueBuf), err) - value = float64(0) + panic(fmt.Sprintf("unable to parse number value '%v': %v", string(valueBuf), err)) } // Otherwise parse it as bool diff --git a/tsdb/points_test.go b/tsdb/points_test.go index 99707cea35..7e840976cf 100644 --- a/tsdb/points_test.go +++ b/tsdb/points_test.go @@ -174,7 +174,84 @@ func TestParsePointMissingFieldValue(t *testing.T) { if err == nil { t.Errorf(`ParsePoints("%s") mismatch. got nil, exp error`, "cpu") } +} +func TestParsePointBadNumber(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=1a`) + if err == nil { + t.Errorf(`ParsePoints("%s") mismatch. got nil, exp error`, `cpu,host=serverA,region=us-west value=1a`) + } +} + +func TestParsePointNumberNonNumeric(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=.1a`) + if err == nil { + t.Errorf(`ParsePoints("%s") mismatch. got nil, exp error`, `cpu,host=serverA,region=us-west value=.1a`) + } +} + +func TestParsePointNegativeWrongPlace(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=0.-1`) + if err == nil { + t.Errorf(`ParsePoints("%s") mismatch. got nil, exp error`, `cpu,host=serverA,region=us-west value=0.-1`) + } +} + +func TestParsePointFloatMultipleDecimals(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=1.1.1`) + if err == nil { + t.Errorf(`ParsePoints("%s") mismatch. got nil, exp error`, `cpu,host=serverA,region=us-west value=1.1.1`) + } + println(err.Error()) +} + +func TestParsePointInteger(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=1`) + if err != nil { + t.Errorf(`ParsePoints("%s") mismatch. got %v, exp nil`, `cpu,host=serverA,region=us-west value=1`, err) + } +} + +func TestParsePointNegativeInteger(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=-1`) + if err != nil { + t.Errorf(`ParsePoints("%s") mismatch. got %v, exp nil`, `cpu,host=serverA,region=us-west value=-1`, err) + } +} + +func TestParsePointNegativeFloat(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=-1.0`) + if err != nil { + t.Errorf(`ParsePoints("%s") mismatch. got %v, exp nil`, `cpu,host=serverA,region=us-west value=-1.0`, err) + } +} + +func TestParsePointFloatNoLeadingDigit(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=.1`) + if err != nil { + t.Errorf(`ParsePoints("%s") mismatch. got %v, exp nil`, `cpu,host=serverA,region=us-west value=-1.0`, err) + } +} + +func TestParsePointFloatScientific(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=1.0e4`) + if err != nil { + t.Errorf(`ParsePoints("%s") mismatch. got %v, exp nil`, `cpu,host=serverA,region=us-west value=1.0e4`, err) + } +} + +func TestParsePointFloatScientificDecimal(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=1.0e-4`) + if err != nil { + t.Errorf(`ParsePoints("%s") mismatch. got %v, exp nil`, `cpu,host=serverA,region=us-west value=1.0e-4`, err) + } +} + +func TestParsePointFloatNegativeScientific(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=-1.0e-4`) + if err != nil { + t.Errorf(`ParsePoints("%s") mismatch. got %v, exp nil`, `cpu,host=serverA,region=us-west value=-1.0e-4`, err) + } } func TestParsePointUnescape(t *testing.T) { From 6d115a7552943cf82b54e095af7e7076a5d95cbe Mon Sep 17 00:00:00 2001 From: Jason Wilder Date: Thu, 11 Jun 2015 13:59:30 -0600 Subject: [PATCH 3/4] Return the line that failed to parse Make it easier to find a bad line in a batch when writing points. --- tsdb/points.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tsdb/points.go b/tsdb/points.go index 7dbc02bdf2..b324804513 100644 --- a/tsdb/points.go +++ b/tsdb/points.go @@ -95,7 +95,7 @@ func ParsePointsWithPrecision(buf []byte, defaultTime time.Time, precision strin pt, err := parsePoint(block, defaultTime, precision) if err != nil { - return nil, err + return nil, fmt.Errorf("unable to parse '%s': %v", string(block), err) } points = append(points, pt) @@ -412,7 +412,7 @@ func scanNumber(buf []byte, i int) (int, []byte, error) { // Can't have more than 1 decimal (e.g. 1.1.1 should fail) if decimals > 1 { - return i, buf[start:i], fmt.Errorf("invalid number: %s", string(buf[start:i+1])) + return i, buf[start:i], fmt.Errorf("invalid number") } // `e` is valid for floats but not as the first char @@ -428,7 +428,7 @@ func scanNumber(buf []byte, i int) (int, []byte, error) { } if !isNumeric(buf[i]) { - return i, buf[start:i], fmt.Errorf("invalid number: %s", string(buf[start:i+1])) + return i, buf[start:i], fmt.Errorf("invalid number") } i += 1 } From c97dedfa3d6d8e9b9d5ec1e081b19bb628117c35 Mon Sep 17 00:00:00 2001 From: Jason Wilder Date: Thu, 11 Jun 2015 14:53:53 -0600 Subject: [PATCH 4/4] Be more explicit about boolean value parsing Supported boolean values are now t, T, true, TRUE, f, F, false, and FALSE. This is what the strconv.ParseBool function supports with the exception of 1 and 0. 1 and 0 would be parsed as ints in the line protocol. Previously, any non-true value would be parsed as false. e.g. value=blah would parse to false. This will now return an error as parsing time. --- tsdb/points.go | 73 +++++++++++++++++++++++++++++++++++++++++++-- tsdb/points_test.go | 21 +++++++++---- 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/tsdb/points.go b/tsdb/points.go index b324804513..b83226c43b 100644 --- a/tsdb/points.go +++ b/tsdb/points.go @@ -319,7 +319,7 @@ func scanFields(buf []byte, i int) (int, []byte, error) { } // If we see an =, ensure that there is at least on char after it - if buf[i] == '=' { + if buf[i] == '=' && !quoted { // check for "... value=" if i+1 >= len(buf) { return i, buf[start:i], fmt.Errorf("missing field value") @@ -338,6 +338,15 @@ func scanFields(buf []byte, i int) (int, []byte, error) { } else { continue } + // If next byte is not a double-quote, the value must be a boolean + } else if buf[i+1] != '"' { + var err error + i, _, err = scanBoolean(buf, i+1) + if err != nil { + return i, buf[start:i], err + } else { + continue + } } } @@ -436,6 +445,65 @@ func scanNumber(buf []byte, i int) (int, []byte, error) { return i, buf[start:i], nil } +// scanBoolean returns the end position within buf, start at i after +// scanning over buf for boolean. Valid values for a boolean are +// t, T, true, TRUE, f, F, false, FALSE. It returns an error if a invalid boolean +// is scanned. +func scanBoolean(buf []byte, i int) (int, []byte, error) { + start := i + + if i < len(buf) && (buf[i] != 't' && buf[i] != 'f' && buf[i] != 'T' && buf[i] != 'F') { + return i, buf[start:i], fmt.Errorf("invalid boolean") + } + + i += 1 + for { + if i >= len(buf) { + break + } + + if buf[i] == ',' || buf[i] == ' ' { + break + } + i += 1 + } + + // Single char bool (t, T, f, F) is ok + if i-start == 1 { + return i, buf[start:i], nil + } + + // length must be 4 for true or TRUE + if (buf[start] == 't' || buf[start] == 'T') && i-start != 4 { + return i, buf[start:i], fmt.Errorf("invalid boolean") + } + + // length must be 5 for false or FALSE + if (buf[start] == 'f' || buf[start] == 'F') && i-start != 5 { + return i, buf[start:i], fmt.Errorf("invalid boolean") + } + + // Otherwise + valid := false + switch buf[start] { + case 't': + valid = bytes.Equal(buf[start:i], []byte("true")) + case 'f': + valid = bytes.Equal(buf[start:i], []byte("false")) + case 'T': + valid = bytes.Equal(buf[start:i], []byte("TRUE")) + case 'F': + valid = bytes.Equal(buf[start:i], []byte("FALSE")) + } + + if !valid { + return i, buf[start:i], fmt.Errorf("invalid boolean") + } + + return i, buf[start:i], nil + +} + // skipWhitespace returns the end position within buf, starting at i after // scanning over spaces in tags func skipWhitespace(buf []byte, i int) int { @@ -826,8 +894,7 @@ func newFieldsFromBinary(buf []byte) Fields { } else { value, err = strconv.ParseBool(string(valueBuf)) if err != nil { - fmt.Printf("unable to parse bool value '%v': %v\n", string(valueBuf), err) - value = false + panic(fmt.Sprintf("unable to parse bool value '%v': %v\n", string(valueBuf), err)) } } fields[string(unescape(name))] = value diff --git a/tsdb/points_test.go b/tsdb/points_test.go index 7e840976cf..7b6581382e 100644 --- a/tsdb/points_test.go +++ b/tsdb/points_test.go @@ -254,6 +254,13 @@ func TestParsePointFloatNegativeScientific(t *testing.T) { } } +func TestParsePointBooleanInvalid(t *testing.T) { + _, err := ParsePointsString(`cpu,host=serverA,region=us-west value=a`) + if err == nil { + t.Errorf(`ParsePoints("%s") mismatch. got nil, exp error`, `cpu,host=serverA,region=us-west value=a`) + } +} + func TestParsePointUnescape(t *testing.T) { // commas in measuremnt name test(t, `cpu\,main,regions=east\,west value=1.0`, @@ -479,7 +486,7 @@ func TestParsePointWithStringWithEquals(t *testing.T) { } func TestParsePointWithBoolField(t *testing.T) { - test(t, `cpu,host=serverA,region=us-east bool=true,boolTrue=t,false=false,falseVal=f 1000000000`, + test(t, `cpu,host=serverA,region=us-east true=true,t=t,T=T,TRUE=TRUE,false=false,f=f,F=F,FALSE=FALSE 1000000000`, NewPoint( "cpu", Tags{ @@ -487,10 +494,14 @@ func TestParsePointWithBoolField(t *testing.T) { "region": "us-east", }, Fields{ - "bool": true, - "boolTrue": true, - "false": false, - "falseVal": false, + "t": true, + "T": true, + "true": true, + "TRUE": true, + "f": false, + "F": false, + "false": false, + "FALSE": false, }, time.Unix(1, 0)), )