Repository navigation
Conversation
|
|
|
Alakesh Boro seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
bbeafe1 to
fa662d0
Compare
ParadoxV5
added
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Please work through the CLA assistant. For it to be cleared the following things need to align:
- you need have a github account
- your github account needs to have your commit email that you've used
Please verify what of the above doesn't hold and then click the CLA button.
FWIW, I've seen your email to the mailing list. But there is a better way :)
Some more comments below.
b5efa54 to
af3fcb6
Compare
CHANGE MASTER string option values break when encountering null (\0) or line-break (\n) bytes because they are stored null-terminated in memory and as plain-text lines on disk. This patch adds a validation block directly in the parser to explicitly reject strings containing \0 or \n for these options, immediately throwing ER_WRONG_ARGUMENTS (1210).
af3fcb6 to
bd7a361
Compare
|
Hi @gkodinov, I've updated the PR based on the feedback: Moved the validation logic out of the parser and into an inline helper function in sql_lex.h. Renamed the new token to uppercase (TEXT_STRING_SYS_NO_LINEBREAK_NULL). Moved the test cases into rpl_change_master.test and formatted the headers as requested. Let me know if you need any further changes! |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Please stand by for the final review.
|
Hi @gkodinov, |
| --error ER_WRONG_ARGUMENTS | ||
| CHANGE MASTER TO MASTER_HOST='127.0\n.0.1'; | ||
|
|
||
| --echo # Test 4: String with carriage return (\r) in the middle |
There was a problem hiding this comment.
In particular, although any control code is questionable when appearing in these strings, I don’t think \r and others actually break anything.
There was a problem hiding this comment.
Should I remove the check for \r and only restrict \n and \0, or keep it blocked just to be safe?
…l_mi.h, and migrate tests to main suite
346f8d1 to
886985c
Compare
CHANGE MASTER string option values break when encountering null (\0) or line-break (\n) bytes because they are stored null-terminated in memory and as plain-text lines on disk.
This patch adds a validation block directly in the parser to explicitly reject strings containing \0 or \n for these options, immediately throwing ER_WRONG_ARGUMENTS (1210).