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.

Sign up for GitHub

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

Jump to bottom

server: ignore ClientPluginAuthLenencClientData temporarily #1772

Merged
merged 4 commits into from
Sep 29, 2016
Merged

server: ignore ClientPluginAuthLenencClientData temporarily #1772

merged 4 commits into from
Sep 29, 2016

Conversation

tiancaiamao
Copy link
Contributor

This is a hot fix, we need investigate it for more detail
#1768

@tiancaiamao
server: ignore ClientPluginAuthLenencClientData temporarily
4465394 
This is a hot fix, we need investigate it for more detail
#1768
@shenli
Copy link
Member

shenli commented Sep 28, 2016

Please add a test case.

@shenli
Copy link
Member

shenli commented Sep 28, 2016

We could follow the logic in mysql: https://github.com/mysql/mysql-server/blob/e0e0ae2ea27c9bb76577664845507ef224d362e4/sql-common/client.c#L3694

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Sep 28, 2016 •
edited
Loading

I think we should do it in a real fix, not here, this PR is just a hot fix if necessary.

we should find why it fail, what's the best way to solve it;
if mysql client doesn't follow its document, we should feed back an issue;
we should also think how to make our code more robust to handle incorrect behavior, including test cases to cover that.

@shenli
Copy link
Member

shenli commented Sep 28, 2016

I mean any PR should prove its correctness. At lease you should add a test case.

@tiancaiamao
Copy link
Contributor Author

According to document ,

The client should only announce the capabilities in the Handshake Response Packet that it has in common with the server.

It's a bug of mysql client, here it is: https://github.com/mysql/mysql-server/blob/5.7/sql-common/client.c#L3478

After ~(CLIENT_COMPRESS | CLIENT_SSL | CLIENT_PROTOCOL_41) | mysql->server_capabilities, it turn on many bits to 0x1 that should not.

@shenli shenli mentioned this pull request Sep 28, 2016
Closed
@tiancaiamao
Copy link
Contributor Author

PTAL @shenli

@shenli
Copy link
Member

shenli commented Sep 28, 2016

LGTM
@tiancaiamao @zimulala PTAL

zimulala
zimulala reviewed Sep 29, 2016
View reviewed changes
server/conn.go
@@ -193,9 +193,13 @@ func handshakeResponseFromData(packet *handshakeResponse41, data []byte) error {
pos += len(packet.User) + 1

if capability&mysql.ClientPluginAuthLenencClientData > 0 {
// TODO: Support mysql.ClientPluginAuthLenencClientData, skip it now
// MySQL client set the wrong capability, it will set this bit even server doesn't
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/set/sets

@zimulala
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao merged commit e472f5a into master Sep 29, 2016
@tiancaiamao tiancaiamao deleted the tiancaiamao/fix branch September 29, 2016 02:38
@tiancaiamao tiancaiamao mentioned this pull request Dec 29, 2016
Closed
@djshow832 djshow832 mentioned this pull request Sep 23, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimulala zimulala zimulala left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tiancaiamao @shenli @zimulala