-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
expression: handle max_allowed_packet warnings for from_base64 function. #7409
Conversation
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. |
/run-all-tests |
@zz-jason PTAL |
@supernan1994 Thanks! |
expression/builtin_string.go
Outdated
func base64NeededDecodedLength(n int) int { | ||
// Returns -1 indicate the result will overflow. | ||
if strconv.IntSize == 64 { | ||
if n > 3074457345618258602 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this value equal to math.MaxInt64
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to define a constant than use a magic number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is math.MaxInt64/3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about:
if strconv.IntSize == 64 && n > math.MaxInt64/3 {
return -1
}
if strconv.IntSize == 32 && n > math.MaxInt32/3 {
return -1
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I get it, thanks~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
expression/builtin_string.go
Outdated
return -1 | ||
} | ||
} | ||
return n * 3 / 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about n/4*3
, then the above validation can be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
expression/builtin_string.go
Outdated
return -1 | ||
} | ||
} else { | ||
if n > 715827882 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you get this number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is math.MaxInt32/3
…/maxAllowPacketFromBase64
/run-all-tests |
@zz-jason DONE. PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
input := chunk.NewChunkWithCapacity(colTypes, 1) | ||
input.AppendString(0, test.args) | ||
res, isNull, err := fromBase64.evalString(input.GetRow(0)) | ||
c.Assert(err, IsNil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…/maxAllowPacketFromBase64
/run-all-tests |
/run-all-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@supernan1994 It would be great if you can cherrypick this bugfix to the |
@zz-jason OK! |
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:
After this PR:
This PR adds
base64NeededDecodedLength
function. This function calculates the length offrom_base64
's result before executing core offrom_base64
logic, and compares the length with global variablemax_allowed_packet
.Check List
Tests
Related changes
please note: