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

expression: handle max_allowed_packet warnings for from_base64 function. #7409

Merged

Conversation

@supernan1994
Copy link
Contributor

commented Aug 15, 2018

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`
@sre-bot

This comment has been minimized.

Copy link

commented Aug 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2018

/run-all-tests

@supernan1994

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2018

@zz-jason PTAL

@shenli

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

@supernan1994 Thanks!

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

This comment has been minimized.

Copy link
@zz-jason

zz-jason Aug 15, 2018

Member

Is this value equal to math.MaxInt64?

This comment has been minimized.

Copy link
@supernan1994

supernan1994 Aug 16, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@shenli

shenli Aug 16, 2018

Member

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

This comment has been minimized.

Copy link
@zz-jason

zz-jason Aug 16, 2018

Member

It is math.MaxInt64/3

This comment has been minimized.

Copy link
@zz-jason

zz-jason Aug 16, 2018

Member

how about:

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

This comment has been minimized.

Copy link
@supernan1994

supernan1994 Aug 16, 2018

Author Contributor

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 comment has been minimized.

Copy link
@zz-jason

zz-jason Aug 16, 2018

Member

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.

This comment has been minimized.

Copy link
@supernan1994

supernan1994 Aug 16, 2018

Author Contributor

Oh, I get it, thanks~

This comment has been minimized.

Copy link
@zz-jason
return -1
}
}
return n * 3 / 4

This comment has been minimized.

Copy link
@zz-jason

zz-jason Aug 16, 2018

Member

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

This comment has been minimized.

Copy link
@supernan1994

supernan1994 Aug 16, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@zz-jason
return -1
}
} else {
if n > 715827882 {

This comment has been minimized.

Copy link
@coocood

coocood Aug 16, 2018

Member

How did you get this number?

This comment has been minimized.

Copy link
@supernan1994

supernan1994 Aug 16, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@zz-jason

zz-jason Aug 16, 2018

Member

It is math.MaxInt32/3

@supernan1994

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

/run-all-tests

@supernan1994

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

@zz-jason DONE. PTAL

Copy link
Member

left a comment

LGTM

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

This comment has been minimized.

Copy link
@crazycs520

crazycs520 Aug 17, 2018

Contributor

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

@supernan1994

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2018

/run-all-tests

@supernan1994

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2018

/run-all-tests

Copy link
Contributor

left a comment

LGTM

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Aug 17, 2018
@zz-jason zz-jason merged commit 4624785 into pingcap:master Aug 17, 2018
11 checks passed
11 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@supernan1994 supernan1994 deleted the supernan1994:feature/maxAllowPacketFromBase64 branch Aug 17, 2018
supernan1994 added a commit to supernan1994/tidb that referenced this pull request Aug 17, 2018
supernan1994 added a commit to supernan1994/tidb that referenced this pull request Aug 17, 2018
@zz-jason

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

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

@supernan1994

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2018

@zz-jason OK!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.