Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix string field value escaping #3088

Merged
merged 2 commits into from
Jun 22, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### Features

- [2650](https://github.com/influxdb/influxdb/pull/2650): Add SHOW GRANTS FOR USER statement. Thanks @n1tr0g.
- [3013](https://github.com/influxdb/influxdb/issues/3013): Panic error with inserting values with commas

### Bugfixes

Expand Down
26 changes: 23 additions & 3 deletions tsdb/points.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"hash/fnv"
"math"
"regexp"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -55,6 +56,9 @@ type point struct {
data []byte
}

// Compile the regex that detects unquoted double quote sequences
var quoteReplacer = regexp.MustCompile(`([^\\])"`)

var escapeCodes = map[byte][]byte{
',': []byte(`\,`),
'"': []byte(`\"`),
Expand Down Expand Up @@ -604,7 +608,8 @@ func scanFieldValue(buf []byte, i int) (int, []byte) {
break
}

if buf[i] == '"' {
// If we see a double quote, makes sure it is not escaped
if buf[i] == '"' && buf[i-1] != '\\' {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If i is zero, this might blow up, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, yes, if you call scanFieldValue(buf, 0) directly then it can blow up but i can't be zero during parsing because we need to scan the the field name first and it must be non-zero in length so the function is always called with a non-zero value for i. I could special case it here too but didn't think it was necessary since the func is private and only called in one place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool on that reasoning -- thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i += 1
quoted = !quoted
continue
Expand Down Expand Up @@ -651,6 +656,21 @@ func unescapeString(in string) string {
return in
}

// escapeQuoteString returns a copy of in with any double quotes that
// have not been escaped with escaped quotes
func escapeQuoteString(in string) string {
if strings.IndexAny(in, `"`) == -1 {
return in
}
return quoteReplacer.ReplaceAllString(in, `$1\"`)
}

// unescapeQuoteString returns a copy of in with any escaped double-quotes
// with unescaped double quotes
func unescapeQuoteString(in string) string {
return strings.Replace(in, `\"`, `"`, -1)
}

// NewPoint returns a new point with the given measurement name, tags, fiels and timestamp
func NewPoint(name string, tags Tags, fields Fields, time time.Time) Point {
return &point{
Expand Down Expand Up @@ -895,7 +915,7 @@ func newFieldsFromBinary(buf []byte) Fields {

// If the first char is a double-quote, then unmarshal as string
if valueBuf[0] == '"' {
value = unescapeString(string(valueBuf[1 : len(valueBuf)-1]))
value = unescapeQuoteString(string(valueBuf[1 : len(valueBuf)-1]))
// Check for numeric characters
} else if (valueBuf[0] >= '0' && valueBuf[0] <= '9') || valueBuf[0] == '-' || valueBuf[0] == '.' {
value, err = parseNumber(valueBuf)
Expand Down Expand Up @@ -955,7 +975,7 @@ func (p Fields) MarshalBinary() []byte {
b = append(b, t...)
case string:
b = append(b, '"')
b = append(b, []byte(t)...)
b = append(b, []byte(escapeQuoteString(t))...)
b = append(b, '"')
case nil:
// skip
Expand Down
33 changes: 32 additions & 1 deletion tsdb/points_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ func TestParsePointWithStringWithCommas(t *testing.T) {
},
Fields{
"value": 1.0,
"str": "foo,bar", // commas in string value
"str": `foo\,bar`, // commas in string value
},
time.Unix(1, 0)),
)
Expand All @@ -475,6 +475,37 @@ func TestParsePointWithStringWithCommas(t *testing.T) {

}

func TestParsePointEscapedStringsAndCommas(t *testing.T) {
// non-escaped comma and quotes
test(t, `cpu,host=serverA,region=us-east value="{Hello\"{,}\" World}" 1000000000`,
NewPoint(
"cpu",
Tags{
"host": "serverA",
"region": "us-east",
},
Fields{
"value": `{Hello"{,}" World}`,
},
time.Unix(1, 0)),
)

// escaped comma and quotes
test(t, `cpu,host=serverA,region=us-east value="{Hello\"{\,}\" World}" 1000000000`,
NewPoint(
"cpu",
Tags{
"host": "serverA",
"region": "us-east",
},
Fields{
"value": `{Hello"{\,}" World}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the comma still preceded by a \? I don't follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only " need to be escaped/unescaped for string fields. Everything else is left as is. We were escaping/unescaping commas and spaces incorrectly here before as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

},
time.Unix(1, 0)),
)

}

func TestParsePointWithStringWithEquals(t *testing.T) {
test(t, `cpu,host=serverA,region=us-east str="foo=bar",value=1.0, 1000000000`,
NewPoint(
Expand Down