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

Incorrect packet parsing for multiStatement=true #602

Closed
rubensayshi opened this issue May 29, 2017 · 9 comments
Closed

Incorrect packet parsing for multiStatement=true #602

rubensayshi opened this issue May 29, 2017 · 9 comments
Labels

Comments

@rubensayshi
Copy link

Issue description

It seems like result messages are being parsed incorrectly for multistatements in combination with auto increment (insertid).
I tried debugging it (see below) but my knowledge of the MySQL packages and this lib is insufficient to properly conclude what is going on.

The 'symptom' that I ran into that lead me to digging deeper was getting back an empty list of MySQLWarnings{} (I tried fixing it with this PR: #555)

Example code

schema.sql

DROP TABLE IF EXISTS `table1`;
CREATE TABLE `table1` (
  `id` INT(11) UNSIGNED NOT NULL,
  PRIMARY KEY (`id`)
);

DROP TABLE IF EXISTS `table2`;
CREATE TABLE `table2` (
  `id` INT(11) UNSIGNED NOT NULL AUTO_INCREMENT,

  PRIMARY KEY (`id`)
);

fixtures.sql

INSERT INTO `table1` VALUES (71398);
INSERT INTO `table2` VALUES (71398);

execute these 2 with ?strict=true&multiStatements=true

I've been trying to look at the handling of the msgs from mysql, when loading the fixtures:

readResultSetHeaderPacket 0001000a000000 
==================
handleOkPacket id=5 data=0001000a000000 
mc.affectedRows=1 (01)(1) mc.insertId=0 (00)(1) 
handleOkPacket m=1 n=1 status=a 
handleOkPacket::discardResults... 
discardResults mc.status=10 discard=true 
readResultSetHeaderPacket 0001fde6160102000000 
==================
handleOkPacket id=6 data=0001fde6160102000000 
mc.affectedRows=1 (01)(1) mc.insertId=71398 (fde61601)(4) 
handleOkPacket m=4 n=1 status=2 
handleOkPacket::discardResults... 
discardResults mc.status=2 discard=false 
handleOkPacket::checkwarnings... id=6 data=0001fde6160102000000 n=1 m=4 pos=8 warningCnt=0000 warningCnt=0 
handleOkPacket::checkwarnings... id=5 data=0001fde6160102 n=1 m=1 pos=5 warningCnt=0102 warningCnt=513 
handleOkPacket::WARNINGS 513 
readResultSetHeaderPacket 03 
discardResults mc.status=2 discard=false 

see the handleOkPacket and handleOkPacket::checkwarnings... (checkwarning... is when it determines if it should do mc.getWarnings()).
the id= is some incrementing id I added each time handleOkPacket is called.
it seems the byte array data is different at the end of the id=5 call...

I'm not really sure what this code is supposed to do, how it works, just debugging, I hope you understand this stuff a little better xD

obviously this ain't the correct fix, but doing data := make([]byte, len(_data)); copy(data, _data) at the start of handleOkPacket fixes the issue (just to 'prove' it has something to do with the underlying byte array of the slice)

Configuration

Driver version (or git SHA): a0583e0143b1624142adab07e0e97fe106d99561

Go version: go version go1.8 linux/amd64

Server version: mysqld Ver 5.7.18-0ubuntu0.16.04.1

Server OS: Ubuntu 16.04.2 LTS

@methane
Copy link
Member

methane commented May 29, 2017

You're right.
multiStatement is mutually exclusive to strict.
I didn't notice about it because I never use strict.

@methane
Copy link
Member

methane commented May 29, 2017

strict section in README says:

This mode should not be used in production as it may lead to data corruption in certain situations.

Warning in multi statement is one of "certain situations."

@julienschmidt
Copy link
Member

@rubensayshi You seem to have some Go program for debugging, please share it with us. The short SQL snippets are not exactly an example code.

And what makes muliStatement and strict mutually exclusive?

@methane
Copy link
Member

methane commented May 29, 2017

And what makes muliStatement and strict mutually exclusive?

strict option try to fetch warnings by querying SHOW WARNINGS.
But next resultsets are remains in the connection.
SHOW WARNINGS is only allowed for last resultset.

@rubensayshi
Copy link
Author

@julienschmidt unfortunately I'm atm so swamped that I had to do the debugging in my own project instead of making a clean reproducible test case, sorry I know it sucks when someone opens a ticket without putting in full effort to provide a completely reproducable test case.

@methane is right though, my testcase (cuz that's where I ran into the issue) passes when I create the connection with strict=false!

@methane
Copy link
Member

methane commented May 30, 2017

I found #376 which adds the "This mode should not be used in production as it may lead to data corruption in certain situations." sentence to README.

But actually speaking, strict=true didn't cause data corruption in the example of #376.
Not using "server side strict mode" caused the corruption.
So it's totally different from this issue.

@rubensayshi Could you try #605 with strict=true?

@rubensayshi
Copy link
Author

yes #605 fixes the issue

@methane
Copy link
Member

methane commented Jun 6, 2017

@rubensayshi Could you write failing test case for this?

julienschmidt added a commit that referenced this issue Sep 29, 2017
@julienschmidt julienschmidt mentioned this issue Sep 29, 2017
5 tasks
julienschmidt added a commit that referenced this issue Oct 3, 2017
* Remove strict mode

Fixes #556 #602 #635
Closes #609

* dsn: panic in case of strict mode
@julienschmidt
Copy link
Member

should be fixed by #676

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants