expression: handle max_allowed_packet warnings for from_base64 function. #7409

supernan1994 · 2018-08-15T15:13:27Z

What problem does this PR solve?

Return NULL and a warning when the result of from_base64 exceeds max_allowed_packetbs.
to #7153

What is changed and how it works?

Before this PR:

mysql> show variables like 'max_allowed_packet';
+--------------------+-------+
| Variable_name      | Value |
+--------------------+-------+
| max_allowed_packet | 2     |
+--------------------+-------+
1 row in set (0.01 sec)

mysql> select from_base64("YWJj");
+---------------------+
| from_base64("YWJj") |
+---------------------+
| abc                 |
+---------------------+
1 row in set (0.04 sec)

After this PR:

mysql> show variables like 'max_allowed_packet';
+--------------------+-------+
| Variable_name      | Value |
+--------------------+-------+
| max_allowed_packet | 2     |
+--------------------+-------+
1 row in set (0.01 sec)

mysql> select from_base64("YWJj");
+---------------------+
| from_base64("YWJj") |
+---------------------+
| NULL                |
+---------------------+
1 row in set, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+----------------------------------------------------------------------------+
| Level   | Code | Message                                                                    |
+---------+------+----------------------------------------------------------------------------+
| Warning | 1301 | Result of from_base64() was larger than max_allowed_packet (2) - truncated |
+---------+------+----------------------------------------------------------------------------+
1 row in set (0.00 sec)

This PR adds base64NeededDecodedLength function. This function calculates the length of from_base64's result before executing core of from_base64 logic, and compares the length with global variable max_allowed_packet.

Check List

Tests

Unit test

Related changes

Need to be included in the release note
please note:

Return `NULL` when the result of function `FROM_BASE64` is larger than `max_allowed_packet`

…/maxAllowPacketFromBase64

sre-bot · 2018-08-15T15:13:31Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

supernan1994 · 2018-08-15T15:13:48Z

/run-all-tests

supernan1994 · 2018-08-15T15:34:33Z

@zz-jason PTAL

shenli · 2018-08-15T16:11:20Z

@supernan1994 Thanks!

zz-jason · 2018-08-15T16:13:19Z

expression/builtin_string.go

+func base64NeededDecodedLength(n int) int {
+	// Returns -1 indicate the result will overflow.
+	if strconv.IntSize == 64 {
+		if n > 3074457345618258602 {


Is this value equal to math.MaxInt64?

No, it's equals to 0x2AAAAAAAAAAAAAAALL, I copy it from mysql.
https://github.com/mysql/mysql-server/blob/5.7/mysys/base64.c#L69
I haven't figured out how it works, do you have any ideas?

It is better to define a constant than use a magic number.

It is math.MaxInt64/3

how about:

if strconv.IntSize == 64 && n > math.MaxInt64/3 { return -1 } if strconv.IntSize == 32 && n > math.MaxInt32/3 { return -1 }

Sure, Thanks a lot!! I will correct it.
I still confuse about why it is math.MaxInt64/3. Does that mean base64_decode function malloc 3 times of source string length during the process? It doesn't make sense.

This check is to guarantee that the later n*3 instruction, which is part of n*3/4, does not overflow the max int64 or int32 value.

Oh, I get it, thanks~

zz-jason · 2018-08-16T02:59:54Z

expression/builtin_string.go

+			return -1
+		}
+	}
+	return n * 3 / 4


How about n/4*3, then the above validation can be removed?

when n%4 > 4/3 , n/4*3 is not equal to n*3/4.
mysql calculates decode length by n*3/4, I think we'd better use the same formula.

coocood · 2018-08-16T03:03:31Z

expression/builtin_string.go

+			return -1
+		}
+	} else {
+		if n > 715827882 {


How did you get this number?

I copy it from mysql.
https://github.com/mysql/mysql-server/blob/5.7/mysys/base64.c#L69
I haven't figured out how it works, do you have any ideas?

It is math.MaxInt32/3

…/maxAllowPacketFromBase64

supernan1994 · 2018-08-16T16:25:47Z

/run-all-tests

supernan1994 · 2018-08-16T16:46:30Z

@zz-jason DONE. PTAL

zz-jason

LGTM

crazycs520 · 2018-08-17T11:35:49Z

expression/builtin_string_test.go

+		input := chunk.NewChunkWithCapacity(colTypes, 1)
+		input.AppendString(0, test.args)
+		res, isNull, err := fromBase64.evalString(input.GetRow(0))
+		c.Assert(err, IsNil)


add c.Assert(isNull, Equals, test.isNil)
then we can remove line #1653 and else branch in line #1662

…/maxAllowPacketFromBase64

supernan1994 · 2018-08-17T14:17:16Z

/run-all-tests

supernan1994 · 2018-08-17T14:20:10Z

/run-all-tests

crazycs520

LGTM

zz-jason · 2018-08-17T15:51:05Z

@supernan1994 It would be great if you can cherrypick this bugfix to the release-2.0 branch 😸

supernan1994 · 2018-08-18T01:48:38Z

@zz-jason OK!

sisterdong added 3 commits August 15, 2018 22:33

expression: handle max_allowed_packet warnings for from_base64 function.

e57f80f

Merge branch 'master' of https://github.com/pingcap/tidb into feature…

6bfb98f

…/maxAllowPacketFromBase64

remove println

c3c34ff

zz-jason reviewed Aug 15, 2018

View reviewed changes

zz-jason reviewed Aug 16, 2018

View reviewed changes

coocood reviewed Aug 16, 2018

View reviewed changes

zz-jason mentioned this pull request Aug 16, 2018

need to handle "ER_WARN_ALLOWED_PACKET_OVERFLOWED" for some string functions #7153

Open

13 tasks

sisterdong added 2 commits August 17, 2018 00:00

Merge branch 'master' of https://github.com/pingcap/tidb into feature…

1376628

…/maxAllowPacketFromBase64

replace magic num with const

42d0c96

shenli added component/expression type/compatibility status/all tests passed labels Aug 17, 2018

zz-jason reviewed Aug 17, 2018

View reviewed changes

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 17, 2018

crazycs520 reviewed Aug 17, 2018

View reviewed changes

sisterdong added 2 commits August 17, 2018 22:12

check isNull by one assert

ca98355

Merge branch 'master' of https://github.com/pingcap/tidb into feature…

4bc3890

…/maxAllowPacketFromBase64

check isNull by one assert

36d4c41

crazycs520 reviewed Aug 17, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 17, 2018

zz-jason approved these changes Aug 17, 2018

View reviewed changes

zz-jason merged commit 4624785 into pingcap:master Aug 17, 2018

supernan1994 deleted the feature/maxAllowPacketFromBase64 branch August 17, 2018 15:14

supernan1994 mentioned this pull request Aug 18, 2018

expression: handle max_allowed_packet warnings for from_base64 function. #7430

Merged

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: handle max_allowed_packet warnings for from_base64 function. #7409

expression: handle max_allowed_packet warnings for from_base64 function. #7409

supernan1994 commented Aug 15, 2018

sre-bot commented Aug 15, 2018

supernan1994 commented Aug 15, 2018

supernan1994 commented Aug 15, 2018

shenli commented Aug 15, 2018

zz-jason Aug 15, 2018

supernan1994 Aug 16, 2018

shenli Aug 16, 2018 •

edited

Loading

zz-jason Aug 16, 2018

zz-jason Aug 16, 2018

supernan1994 Aug 16, 2018

zz-jason Aug 16, 2018 •

edited

Loading

supernan1994 Aug 16, 2018

zz-jason Aug 16, 2018

zz-jason Aug 16, 2018

supernan1994 Aug 16, 2018

zz-jason Aug 16, 2018

coocood Aug 16, 2018

supernan1994 Aug 16, 2018

zz-jason Aug 16, 2018

supernan1994 commented Aug 16, 2018

supernan1994 commented Aug 16, 2018

zz-jason left a comment

crazycs520 Aug 17, 2018

supernan1994 commented Aug 17, 2018

supernan1994 commented Aug 17, 2018

crazycs520 left a comment

zz-jason commented Aug 17, 2018

supernan1994 commented Aug 18, 2018

expression: handle max_allowed_packet warnings for from_base64 function. #7409

expression: handle max_allowed_packet warnings for from_base64 function. #7409

Conversation

supernan1994 commented Aug 15, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

sre-bot commented Aug 15, 2018

supernan1994 commented Aug 15, 2018

supernan1994 commented Aug 15, 2018

shenli commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli Aug 16, 2018 • edited Loading

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

zz-jason Aug 16, 2018 • edited Loading

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

Choose a reason for hiding this comment

supernan1994 commented Aug 16, 2018

supernan1994 commented Aug 16, 2018

zz-jason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernan1994 commented Aug 17, 2018

supernan1994 commented Aug 17, 2018

crazycs520 left a comment

Choose a reason for hiding this comment

zz-jason commented Aug 17, 2018

supernan1994 commented Aug 18, 2018

shenli Aug 16, 2018 •

edited

Loading

zz-jason Aug 16, 2018 •

edited

Loading