From fc4246d7f50daa164c69f3b208fcf0f82db9aec7 Mon Sep 17 00:00:00 2001 From: Daniel Morsing Date: Wed, 5 Aug 2015 17:40:42 +0100 Subject: [PATCH 1/2] be more strict about identifier printing When stringifying a query, we would print the identifier bare most of the time. This caused issues when stringifying an identifier that contained elements of syntax. For example, querying for the value "in-bytes" would fail because the mapper would serialize it to in-bytes and would parse it as an expression. Same problem occured when using keywords as identifier names, such as select or in. Fixes #3547 --- influxql/ast.go | 24 +++--------------------- influxql/ast_test.go | 10 +++++----- influxql/parser.go | 5 +++++ influxql/parser_test.go | 2 ++ 4 files changed, 15 insertions(+), 26 deletions(-) diff --git a/influxql/ast.go b/influxql/ast.go index 0942b77929..a0559f9023 100644 --- a/influxql/ast.go +++ b/influxql/ast.go @@ -1992,26 +1992,6 @@ func (f *Field) Name() string { func (f *Field) String() string { str := f.Expr.String() - switch f.Expr.(type) { - case *VarRef: - quoted := false - // Escape any double-quotes in the field - if strings.Contains(str, `"`) { - str = strings.Replace(str, `"`, `\"`, -1) - quoted = true - } - - // Escape any single-quotes in the field - if strings.Contains(str, `'`) { - quoted = true - } - - // Double-quote field names with spaces or that were previously escaped - if strings.Contains(str, " ") || quoted { - str = fmt.Sprintf("\"%s\"", str) - } - } - if f.Alias == "" { return str } @@ -2132,7 +2112,9 @@ type VarRef struct { } // String returns a string representation of the variable reference. -func (r *VarRef) String() string { return r.Val } +func (r *VarRef) String() string { + return QuoteIdent(r.Val) +} // Call represents a function call. type Call struct { diff --git a/influxql/ast_test.go b/influxql/ast_test.go index 3d074d5f1d..ce45345f51 100644 --- a/influxql/ast_test.go +++ b/influxql/ast_test.go @@ -44,35 +44,35 @@ func TestSelectStatement_Substatement(t *testing.T) { { stmt: `SELECT sum(aa.value) + sum(bb.value) FROM aa, bb`, expr: &influxql.VarRef{Val: "aa.value"}, - sub: `SELECT aa.value FROM aa`, + sub: `SELECT "aa.value" FROM aa`, }, // 2. Simple merge { stmt: `SELECT sum(aa.value) + sum(bb.value) FROM aa, bb`, expr: &influxql.VarRef{Val: "bb.value"}, - sub: `SELECT bb.value FROM bb`, + sub: `SELECT "bb.value" FROM bb`, }, // 3. Join with condition { stmt: `SELECT sum(aa.value) + sum(bb.value) FROM aa, bb WHERE aa.host = 'servera' AND bb.host = 'serverb'`, expr: &influxql.VarRef{Val: "bb.value"}, - sub: `SELECT bb.value FROM bb WHERE bb.host = 'serverb'`, + sub: `SELECT "bb.value" FROM bb WHERE "bb.host" = 'serverb'`, }, // 4. Join with complex condition { stmt: `SELECT sum(aa.value) + sum(bb.value) FROM aa, bb WHERE aa.host = 'servera' AND (bb.host = 'serverb' OR bb.host = 'serverc') AND 1 = 2`, expr: &influxql.VarRef{Val: "bb.value"}, - sub: `SELECT bb.value FROM bb WHERE (bb.host = 'serverb' OR bb.host = 'serverc') AND 1.000 = 2.000`, + sub: `SELECT "bb.value" FROM bb WHERE ("bb.host" = 'serverb' OR "bb.host" = 'serverc') AND 1.000 = 2.000`, }, // 5. 4 with different condition order { stmt: `SELECT sum(aa.value) + sum(bb.value) FROM aa, bb WHERE ((bb.host = 'serverb' OR bb.host = 'serverc') AND aa.host = 'servera') AND 1 = 2`, expr: &influxql.VarRef{Val: "bb.value"}, - sub: `SELECT bb.value FROM bb WHERE ((bb.host = 'serverb' OR bb.host = 'serverc')) AND 1.000 = 2.000`, + sub: `SELECT "bb.value" FROM bb WHERE (("bb.host" = 'serverb' OR "bb.host" = 'serverc')) AND 1.000 = 2.000`, }, } diff --git a/influxql/parser.go b/influxql/parser.go index f995441ace..b2c51e5959 100644 --- a/influxql/parser.go +++ b/influxql/parser.go @@ -2217,6 +2217,11 @@ func QuoteIdent(segments ...string) string { // IdentNeedsQuotes returns true if the ident string given would require quotes. func IdentNeedsQuotes(ident string) bool { + // check if this identifier is a keyword + tok := Lookup(ident) + if tok != IDENT { + return true + } for i, r := range ident { if i == 0 && !isIdentFirstChar(r) { return true diff --git a/influxql/parser_test.go b/influxql/parser_test.go index 2ea53a0ed2..3d7b55ac0c 100644 --- a/influxql/parser_test.go +++ b/influxql/parser_test.go @@ -1664,6 +1664,8 @@ func TestQuoteIdent(t *testing.T) { s string }{ {[]string{``}, ``}, + {[]string{`select`}, `"select"`}, + {[]string{`in-bytes`}, `"in-bytes"`}, {[]string{`foo`, `bar`}, `"foo".bar`}, {[]string{`foo`, ``, `bar`}, `"foo"..bar`}, {[]string{`foo bar`, `baz`}, `"foo bar".baz`}, From 38b70b2cfddbdb3cc607462d09ef7565500c72d1 Mon Sep 17 00:00:00 2001 From: Daniel Morsing Date: Wed, 5 Aug 2015 19:41:00 +0100 Subject: [PATCH 2/2] add test for evil identifiers that stress the parser --- cmd/influxd/run/server_test.go | 42 ++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/cmd/influxd/run/server_test.go b/cmd/influxd/run/server_test.go index 2b94fac47f..67e3e22010 100644 --- a/cmd/influxd/run/server_test.go +++ b/cmd/influxd/run/server_test.go @@ -3066,3 +3066,45 @@ func TestServer_Query_CreateContinuousQuery(t *testing.T) { } } } + +func TestServer_Query_EvilIdentifiers(t *testing.T) { + t.Parallel() + s := OpenServer(NewConfig(), "") + defer s.Close() + + if err := s.CreateDatabaseAndRetentionPolicy("db0", newRetentionPolicyInfo("rp0", 1, 0)); err != nil { + t.Fatal(err) + } + if err := s.MetaStore.SetDefaultRetentionPolicy("db0", "rp0"); err != nil { + t.Fatal(err) + } + + test := NewTest("db0", "rp0") + test.write = fmt.Sprintf("cpu select=1,in-bytes=2 %d", mustParseTime(time.RFC3339Nano, "2000-01-01T00:00:00Z").UnixNano()) + + test.addQueries([]*Query{ + &Query{ + name: `query evil identifiers`, + command: `SELECT "select", "in-bytes" FROM cpu`, + exp: `{"results":[{"series":[{"name":"cpu","columns":["time","in-bytes","select"],"values":[["2000-01-01T00:00:00Z",2,1]]}]}]}`, + params: url.Values{"db": []string{"db0"}}, + }, + }...) + + for i, query := range test.queries { + if i == 0 { + if err := test.init(s); err != nil { + t.Fatalf("test init failed: %s", err) + } + } + if query.skip { + t.Logf("SKIP:: %s", query.name) + continue + } + if err := query.Execute(s); err != nil { + t.Error(query.Error(err)) + } else if !query.success() { + t.Error(query.failureMessage()) + } + } +}