Placeholder interpolation #309

arvenil · 2015-02-01T22:56:56Z

@methane did awesome job in #297 and since I'm highly interested in this PR I wanted to test it. However #297 was out of sync with master ("This pull request contains merge conflicts that must be resolved.") so I merged changes from master.
Since I was already testing this. I've also:

added some basic tests for escape functions (100% code coverage)
exported escape functions (in case anyone would like to use them)
added some basic sql injection tests (including sql_mode=NO_BACKSLASH_ESCAPES)
referenced ported code to mysql source code (just in case if anyone like me;) would like to take a look if port is correct)

Feel free to merge this or throw away, I did this mostly to verify correctness.

One more time thanks for this @methane !

Conflicts: connection.go utils_test.go

…urce code

methane · 2015-02-02T02:46:59Z

Good job!

arnehormann · 2015-02-05T16:57:04Z

I'll hopefully get to review this soon.
Some remarks I already have from skimming it:

@arvenil add yourself to the AUTHORS file please.
@methane please close the other PR and reference this one if you are ok with arvenil taking over for you.

I will probably merge #311 first (it's pretty small), as this also hits const.go, you'll have to resync - sorry.

Can you change the tests so SELECT ... CAST is used instead of table writes? I don't know if that's applicable here, but I'd love to reduce the amount of disk IO during testing in the long run. See the DATE/TIME tests for example.

I don't like the Escape... functions, I prefer an externally controllable buffer and quotes embedded in the function.
Have a look at http://play.golang.org/p/ujxynUDVhE (extra points to you for a benchmark gist to compare speed & allocations).
I also think the EscapeQuoting tests are wrong. As a question: how would you be able to upload and retrieve a string foo\x00bar unchanged by the driver?

methane · 2015-02-06T07:46:04Z

I feel "interpolation" is better word than "substitution." But I'm not sure since I'm native English.

@arvenil May I write optimized version of escaping function?

arvenil · 2015-02-06T10:13:44Z

@methane Yes, I've added you to https://github.com/arvenil/mysql/tree/placeholder as collaborator so you could push to this PR but do whatever suits you better.

I will address other questions and issues later, when time allows.

arvenil · 2015-02-07T15:44:52Z

Hi @arnehormann

I also think the EscapeQuoting tests are wrong. As a question: how would you be able to upload and retrieve a string foo\x00bar unchanged by the driver?

For MySQL data is not changed, it is escaped. This is done exactly the same way how MySQL does it. Take a look at https://github.com/mysql/mysql-server/blob/mysql-5.7.5/mysys/charset.c#L823-L932 https://github.com/mysql/mysql-server/blob/mysql-5.7.5/mysys/charset.c#L963-L1038

To prove that data is not changed I've added TestInsertRetrieveEscapedData. You can see there that retrieved value equals inserted foo \x00\n\r\x1a\"'\\.

"DROP TABLE IF EXISTS ..." query fails on fresh database.

arnehormann · 2015-02-08T13:13:31Z

const.go

+const (
+	statusInTrans statusFlag = 1 << iota
+	statusInAutocommit
+	statusUnknown1


statusReserved // not in documentation please

benchmark old ns/op new ns/op delta BenchmarkInterpolation 4065 2533 -37.69% benchmark old allocs new allocs delta BenchmarkInterpolation 15 6 -60.00% benchmark old bytes new bytes delta BenchmarkInterpolation 1144 560 -51.05%

arnehormann · 2015-02-12T09:26:06Z

README.md

+
+When `interpolateParams` is true, calls to `sql.Db.Query()` and `sql.Db.Exec()` with params interpolates placeholders (`?`) with given params. This reduces roundtrips to database compared with `interpolateParams=false` since it uses prapre, exec and close to support parameters.
+
+NOTE: It make SQL injection vulnerability when connection encoding is not utf8.


When the connection encoding is not utf8, the connection becomes vulnerable to SQL injection attacks.

Maybe I misread something, but how does this depend on utf8?

If connection encoding is Shift-JIS, 95 5c 27 20 should be encoded into 95 5c 27 20 since 95 5c represents one multibyte char (表).
But escapeBytesBackslash and escapeStringBackslash encode it into 95 5c 5c 27 20 since 5c is \.
This is famous SQL injection vulnerability in Japan.

julienschmidt · 2015-02-12T09:54:14Z

Let me also do a final review later before merging.

arnehormann · 2015-02-12T09:55:47Z

@julienschmidt I intended to do so. This is huge...

julienschmidt · 2015-02-12T11:29:35Z

README.md

+Default:        false
+```
+
+When `interpolateParams` is true, calls to `sql.Db.Query()` and `sql.Db.Exec()` with params interpolates placeholders (`?`) with given params. This reduces roundtrips to database compared with `interpolateParams=false` since it uses prepare, exec and close to support parameters.


use just db.Query() / db.Exec() instead of sql.Db.Query() / sql.Db.Exec()

If interpolateParams is true, placeholders (?) in calls to db.Query() and db.Exec() are interpolated into a single query string with the given parameters. This reduces the number of roundtrips, since the driver has to prepare a statement, execute it with the given parameters and close the statement again with interpolateParams=false.

…ding

julienschmidt · 2015-02-12T12:18:27Z

utils.go

+		copy(n, buf)
+		buf = n
+	}
+	buf = buf[0:end]


The exact same code from L819-L824 is used 4 times. Move it to a helper function. And rename n to newBuf. n is more or less reserved for numbers 😉

And why pos+end by the way?

did it.
pos+end = len(old)*2 + reserved size. len(buf)*2 is for exponential realloc.

julienschmidt · 2015-02-12T14:06:51Z

utils.go

+		// A whitelist of collations which safe to interpolate parameters.
+		// ASCII and latin-1 are safe since they are single byte encoding.
+		// utf-8 is safe since it doesn't conatins ASCII characters in trailing bytes.
+		safeCollations := []string{"ascii_", "latin1_", "utf8_", "utf8mb4_"}


It might be better to use a blacklist instead. MySQL only supports 5 unsafe collations: big5, cp932, gb2312, gbk and sjis

The blacklist would then be:
"big5_chinese_ci": 1,
"sjis_japanese_ci": 13,
"gbk_chinese_ci": 28,
"big5_bin": 84,
"gb2312_bin": 86,
"gbk_bin": 87,
"sjis_bin": 88,
"cp932_japanese_ci": 95,
"cp932_bin": 96,

You can also directly check the ID instead of the name then

arnehormann · 2015-02-12T15:58:24Z

@methane praise your patience and thank you for doing this... 😀

julienschmidt · 2015-02-12T19:25:21Z

LGTM... almost

julienschmidt · 2015-02-12T19:28:43Z

@arvenil please add yourself to the AUTHORS file

Placeholder interpolation

julienschmidt · 2015-02-14T01:36:16Z

Thank you!

methane · 2015-02-19T11:31:17Z

FYI, I've wrote blog post about optimizing this pull request.
http://methane.github.io/2015/02/reduce-allocation-in-go-code/

methane and others added 8 commits January 1, 2015 05:53

Implement placeholder substitution.

e35fa00

Query() uses client-side placeholder substitution.

c8c9bb1

Don't send text query larger than maxPacketAllowed

cac6129

Add substitutePlaceholder option to DSN

b7c2c47

Merge remote-tracking branch 'upstream/pr/297'

f3b82fd

Conflicts: connection.go utils_test.go

Move escape funcs to utils.go, export them, add references to mysql s…

058ce87

…urce code

Add tests for escaping functions

42956fa

Add basic SQL injection tests, including NO_BACKSLASH_ESCAPES sql_mode

e6bf23a

Test if inserted data is correctly retrieved after being escaped

b473259

arvenil force-pushed the placeholder branch from afa8b62 to b473259 Compare February 7, 2015 15:35

methane added 3 commits February 8, 2015 17:18

Don't stop test on MySQLWarnings

42a1efd

"DROP TABLE IF EXISTS ..." query fails on fresh database.

substitutePlaceholder -> interpolateParams

3c8fa90

Add interpolateParams document to README

6c8484b

arnehormann reviewed Feb 8, 2015
View reviewed changes

Fix nits pointed in pull request.

04866ee

methane force-pushed the placeholder branch from 1bcbdfb to 04866ee Compare February 8, 2015 15:55

methane added 4 commits February 9, 2015 01:16

Add benchmark for interpolateParams()

dd7b87c

Don't write microseconds when Time.Nanosecond() == 0

9faabe5

Fix benchmark

468b9e5

Reduce allocs in interpolateParams.

0297315

benchmark old ns/op new ns/op delta BenchmarkInterpolation 4065 2533 -37.69% benchmark old allocs new allocs delta BenchmarkInterpolation 15 6 -60.00% benchmark old bytes new bytes delta BenchmarkInterpolation 1144 560 -51.05%

arnehormann reviewed Feb 12, 2015
View reviewed changes

methane added 3 commits February 12, 2015 18:36

Fix typo

d65f96a

Inlining mysqlConn.escapeBytes and mysqlConn.escapeString

e11c825

Bit detailed info about vulnerability when using multibyte encoding.

b4f0315

julienschmidt reviewed Feb 12, 2015
View reviewed changes

methane added 2 commits February 12, 2015 21:05

Add link to StackOverflow describe vulnerability using multibyte enco…

1fd0514

…ding

Fix comment

20b75cd

julienschmidt reviewed Feb 12, 2015
View reviewed changes

methane added 2 commits February 12, 2015 22:17

Allow interpolateParams only with ascii, latin1 and utf8 collations

e517683

extract function to reserve buffer

0f22bc2

methane force-pushed the placeholder branch from c6e8369 to 0f22bc2 Compare February 12, 2015 13:34

methane added 2 commits February 12, 2015 22:35

Fix missing db.Close()

52a5860

Fix sentence in interpolateParams document.

2a634df

julienschmidt reviewed Feb 12, 2015
View reviewed changes

Use blacklist to avoid vulnerability with interpolation

90cb6c3

methane force-pushed the placeholder branch from 75e890f to 90cb6c3 Compare February 12, 2015 15:57

Adding myself to AUTHORS (however, 99% work done by @methane ;))

9437b61

julienschmidt added a commit that referenced this pull request Feb 14, 2015

Merge pull request #309 from arvenil/placeholder

200c80b

Placeholder interpolation

julienschmidt merged commit 200c80b into go-sql-driver:master Feb 14, 2015

methane mentioned this pull request Feb 18, 2015

Go: Load balance between multiple connection pools TechEmpower/FrameworkBenchmarks#1164

Closed

Preetam mentioned this pull request Feb 26, 2015

Pull updates from upstream VividCortex/mysql#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Placeholder interpolation #309

Placeholder interpolation #309

arvenil commented Feb 1, 2015

methane commented Feb 2, 2015

arnehormann commented Feb 5, 2015

methane commented Feb 6, 2015

arvenil commented Feb 6, 2015

arvenil commented Feb 7, 2015

arnehormann Feb 8, 2015

methane Feb 8, 2015

arnehormann Feb 12, 2015

methane Feb 12, 2015

julienschmidt commented Feb 12, 2015

arnehormann commented Feb 12, 2015

julienschmidt Feb 12, 2015

julienschmidt Feb 12, 2015

julienschmidt Feb 12, 2015

julienschmidt Feb 12, 2015

methane Feb 12, 2015

julienschmidt Feb 12, 2015

julienschmidt Feb 12, 2015

methane Feb 12, 2015

arnehormann commented Feb 12, 2015

julienschmidt commented Feb 12, 2015

julienschmidt commented Feb 12, 2015

julienschmidt commented Feb 14, 2015

methane commented Feb 19, 2015


		When `interpolateParams` is true, calls to `sql.Db.Query()` and `sql.Db.Exec()` with params interpolates placeholders (`?`) with given params. This reduces roundtrips to database compared with `interpolateParams=false` since it uses prapre, exec and close to support parameters.

		NOTE: It make SQL injection vulnerability when connection encoding is not utf8.

Placeholder interpolation #309

Placeholder interpolation #309

Conversation

arvenil commented Feb 1, 2015

methane commented Feb 2, 2015

arnehormann commented Feb 5, 2015

methane commented Feb 6, 2015

arvenil commented Feb 6, 2015

arvenil commented Feb 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienschmidt commented Feb 12, 2015

arnehormann commented Feb 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnehormann commented Feb 12, 2015

julienschmidt commented Feb 12, 2015

julienschmidt commented Feb 12, 2015

julienschmidt commented Feb 14, 2015

methane commented Feb 19, 2015