-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Fix multibyte bugs in my_instr_mb #515
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
Fix multibyte bugs in my_instr_mb #515
Conversation
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.
Looks great!
c85146b
to
b3569f0
Compare
Hi, thank you for your contribution. Please confirm this code is submitted under the terms of the OCA (Oracle's Contribution Agreement) you have previously signed by cutting and pasting the following text as a comment: |
I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it. |
The my_instr_mb function makes the incorrect assumption in multiple places that byte lengths for the input strings can be used to short cut certain decisions. In the case of multibyte character sets and collations, this can't be done though since characters with a different byte length can be considered equal under collation rules. Take for example 'a' and 'Å' which are considered equal under the default MySQL 8 collation utf8mb4_0900_ai_ci: mysql> select 'a' = 'Å'; +-------------+ | 'a' = 'Å' | +-------------+ | 1 | +-------------+ 1 row in set (0.00 sec) The character 'a' is though encoded with 1 byte, but 'Å' is encoded with 3 bytes. When using LOCATE, it should also find the character: mysql> select locate('a', 'Å'); +--------------------+ | locate('a', 'Å') | +--------------------+ | 0 | +--------------------+ 1 row in set (0.00 sec) It shows here that it's wrong, and it doesn't consider the character a substring of the other character, which mismatches the behavior seen when checking equality above. This is due to a number of bugs. First of all, there is this check at the beginning: if (s_length <= b_length) { To be explicit, both length variables here are byte lengths, not character lengths of the inputs. When this check does not match and the byte length of needle is longer than the haystack, the assumption is made that no result can be found. This is wrong, because in case here of a needle of 'Å' it will have 3 bytes, but the haystack of 'a' has 1 byte. We still need to search through with the proper collation settings. Therefore the change here removes this check as it's an incorrect optimization trying to short circuit. There's another attempt at optimization here to reduce the maximum string length searched: end = b + b_length - s_length + 1; This is also not correct because of the same reason. We can't assume byte lengths match character lengths, so this is changed to: end = b + b_length to ensure that the whole haystack string is searched. Lastly, there's another bug. The call to strnncoll again assumes that you can truncate the haystack to the byte length. This goes wrong if the needle is 'a' and the haystack is 'Å'. In that case, the haystack string of 'Å' is 3 bytes long and will be cut off at 1 byte. This results in an invalid UTF-8 input and no match on the search. Instead, strnncoll has a prefix option, so we use that and pass in the full string lenghts so that a proper search for a prefix is made. The higher level Item_func_locate function also has a bug where incorrectly byte length is used. The following is incorrect: return start + 1; As start here is just updated from the character position to the byte position. Instead, we need to use start0 here to return the correct offset. These changes resolve the given bugs around multibyte characters and substring searching.
b3569f0
to
e559fde
Compare
I've also identified another bug here that I also fixed described with this part:
|
Hi, thank you for your contribution. Your code has been assigned to an internal queue. Please follow |
The
my_instr_mb
function makes the incorrect assumption in multiple places that byte lengths for the input strings can be used to short cut certain decisions. In the case of multibyte character sets and collations, this can't be done though since characters with a different byte length can be considered equal under collation rules.Take for example
a
andÅ
which are considered equal under the default MySQL 8 collationutf8mb4_0900_ai_ci
:The character 'a' is though encoded with 1 byte, but 'Å' is encoded with 3 bytes. When using
LOCATE
, it should also find the character:It shows here that it's wrong, and it doesn't consider the character a substring of the other character, which mismatches the behavior seen when checking equality above.
This is due to a number of bugs. First of all, there is this check at the beginning:
To be explicit, both length variables here are byte lengths, not character lengths of the inputs. When this check does not match and the byte length of needle is longer than the haystack, the assumption is made that no result can be found. This is wrong, because in case here of a needle of 'Å' it will have 3 bytes, but the haystack of 'a' has 1 byte. We still need to search through with the proper collation settings. Therefore the change here removes this check as it's an incorrect optimization trying to short circuit.
There's another attempt at optimization here to reduce the maximum string length searched:
This is also not correct because of the same reason. We can't assume byte lengths match character lengths, so this is changed to:
to ensure that the whole haystack string is searched.
Lastly, there's another bug. The call to strnncoll again assumes that you can truncate the haystack to the byte length. This goes wrong if the needle is
a
and the haystack isÅ
. In that case, the haystack string ofÅ
is 3 bytes long and will be cut off at 1 byte. This results in an invalid UTF-8 input and no match on the search.Instead,
strnncoll
has a prefix option, so we use that and pass in the full string lenghts so that a proper search for a prefix is made.The higher level Item_func_locate function also has a bug where incorrectly byte length is used. The following is incorrect:
As start here is just updated from the character position to the byte position. Instead, we need to use
start0
here to return the correct offset.These changes resolve the given bugs around multibyte characters and substring searching.