Make the parsers stricter, and improve unit test coverage of mydump…

… package (pingcap#185)
overvenus · May 14, 2019 · f4512ae · f4512ae
1 parent 973244e
commit f4512ae
Show file tree

Hide file tree

Showing 10 changed files with 3,200 additions and 1,716 deletions.
diff --git a/Makefile b/Makefile
@@ -24,7 +24,7 @@ GOTEST    := GO111MODULE=on CGO_ENABLED=1 $(GO) test -p 3
 ARCH      := "`uname -s`"
 LINUX     := "Linux"
 MAC       := "Darwin"
-PACKAGES  := $$(go list ./...| grep -vE 'vendor|cmd|test|proto|diff|bin')
+PACKAGES  := $$(go list ./...| grep -vE 'vendor|cmd|test|proto|diff|bin|fuzz')
 FILES     := $$(find lightning cmd -name '*.go' -type f -not -name '*.pb.go' -not -name '*_generated.go')
 
 FAILPOINT_ENABLE  := $$($(FAILPOINT_CTL_BIN) enable $$PWD/lightning/)

diff --git a/lightning/config/config.go b/lightning/config/config.go
@@ -276,14 +276,29 @@ func (cfg *Config) Load() error {
 		return errors.Trace(err)
 	}
 
-	if len(cfg.Mydumper.CSV.Separator) != 1 {
+	// Reject problematic CSV configurations.
+	csv := &cfg.Mydumper.CSV
+	if len(csv.Separator) != 1 {
 		return errors.New("invalid config: `mydumper.csv.separator` must be exactly one byte long")
 	}
 
-	if len(cfg.Mydumper.CSV.Delimiter) > 1 {
+	if len(csv.Delimiter) > 1 {
 		return errors.New("invalid config: `mydumper.csv.delimiter` must be one byte long or empty")
 	}
 
+	if csv.Separator == csv.Delimiter {
+		return errors.New("invalid config: cannot use the same character for both CSV delimiter and separator")
+	}
+
+	if csv.BackslashEscape {
+		if csv.Separator == `\` {
+			return errors.New("invalid config: cannot use '\\' as CSV separator when `mydumper.csv.backslash-escape` is true")
+		}
+		if csv.Delimiter == `\` {
+			return errors.New("invalid config: cannot use '\\' as CSV delimiter when `mydumper.csv.backslash-escape` is true")
+		}
+	}
+
 	cfg.TiDB.SQLMode, err = mysql.GetSQLMode(cfg.TiDB.StrSQLMode)
 	if err != nil {
 		return errors.Annotate(err, "invalid config: `mydumper.tidb.sql_mode` must be a valid SQL_MODE")

diff --git a/lightning/config/config_test.go b/lightning/config/config_test.go
@@ -2,10 +2,13 @@ package config_test
 
 import (
 	"fmt"
+	"io/ioutil"
 	"net"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"path"
+	"regexp"
 	"strconv"
 	"testing"
 
@@ -106,3 +109,110 @@ func (s *configTestSuite) TestAdjustWillNotContactServerIfEverythingIsDefined(c
 	c.Assert(cfg.TiDB.Port, Equals, 4567)
 	c.Assert(cfg.TiDB.PdAddr, Equals, "234.56.78.90:12345")
 }
+
+func (s *configTestSuite) TestInvalidCSV(c *C) {
+	d := c.MkDir()
+	p := path.Join(d, "cfg.toml")
+
+	testCases := []struct {
+		input string
+		err   string
+	}{
+		{
+			input: `
+				[mydumper.csv]
+				separator = ''
+			`,
+			err: "invalid config: `mydumper.csv.separator` must be exactly one byte long",
+		},
+		{
+			input: `
+				[mydumper.csv]
+				separator = 'hello'
+			`,
+			err: "invalid config: `mydumper.csv.separator` must be exactly one byte long",
+		},
+		{
+			input: `
+				[mydumper.csv]
+				separator = '\'
+			`,
+			err: "",
+		},
+		{
+			input: `
+				[mydumper.csv]
+				separator = '，'
+			`,
+			err: "invalid config: `mydumper.csv.separator` must be exactly one byte long",
+		},
+		{
+			input: `
+				[mydumper.csv]
+				delimiter = ''
+			`,
+			err: "",
+		},
+		{
+			input: `
+				[mydumper.csv]
+				delimiter = 'hello'
+			`,
+			err: "invalid config: `mydumper.csv.delimiter` must be one byte long or empty",
+		},
+		{
+			input: `
+				[mydumper.csv]
+				delimiter = '\'
+			`,
+			err: "",
+		},
+		{
+			input: `
+				[mydumper.csv]
+				delimiter = '“'
+			`,
+			err: "invalid config: `mydumper.csv.delimiter` must be one byte long or empty",
+		},
+		{
+			input: `
+				[mydumper.csv]
+				separator = '|'
+				delimiter = '|'
+			`,
+			err: "invalid config: cannot use the same character for both CSV delimiter and separator",
+		},
+		{
+			input: `
+				[mydumper.csv]
+				separator = '\'
+				backslash-escape = true
+			`,
+			err: "invalid config: cannot use '\\' as CSV separator when `mydumper.csv.backslash-escape` is true",
+		},
+		{
+			input: `
+				[mydumper.csv]
+				delimiter = '\'
+				backslash-escape = true
+			`,
+			err: "invalid config: cannot use '\\' as CSV delimiter when `mydumper.csv.backslash-escape` is true",
+		},
+	}
+
+	for _, tc := range testCases {
+		comment := Commentf("input = %s", tc.input)
+
+		cfg := config.NewConfig()
+		cfg.ConfigFile = p
+		err := ioutil.WriteFile(p, []byte(tc.input), 0644)
+		c.Assert(err, IsNil, comment)
+
+		err = cfg.Load()
+		if tc.err != "" {
+			c.Assert(err, ErrorMatches, regexp.QuoteMeta(tc.err), comment)
+		} else {
+			c.Assert(err, IsNil, comment)
+		}
+	}
+}
diff --git a/lightning/mydump/csv_parser.rl b/lightning/mydump/csv_parser.rl
@@ -32,19 +32,19 @@ machine csv_parser;
 # 	"5\"6",7
 #
 
-q = ^'\n' when { fc == delim };
+q = ^[\r\n] when { fc == delim };
 bs = '\\' when { parser.escFlavor != backslashEscapeFlavorNone };
-sep = ^'\n' when { fc == sep };
+sep = ^[\r\n] when { fc == sep };
 
-c = (^'\n' - q - bs - sep) | bs any;
+c = (^[\r\n] - q - bs - sep) | bs any;
 
 main := |*
 	sep => {
 		consumedToken = csvTokSep
 		fbreak;
 	};
 
-	q (c | '\n' | sep | q q)* q | c+ => {
+	q (c | [\r\n] | sep | q q)* q | c+ => {
 		consumedToken = csvTokField
 		fbreak;
 	};
@@ -86,7 +86,7 @@ func (parser *CSVParser) lex() (csvToken, []byte, error) {
 				zap.Int64("pos", parser.pos),
 				zap.ByteString("content", data),
 			)
-			return csvTokNil, nil, errors.New("Syntax error")
+			return csvTokNil, nil, errors.New("syntax error")
 		}
 
 		if consumedToken != csvTokNil {