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

fixed decoding LCB value for large feilds #40

Closed
wants to merge 1 commit into from
Closed

Conversation

fsw
Copy link

@fsw fsw commented May 11, 2014

fixed decoding LCB value for fields > 250 bytes.
according to this doc:
http://dev.mysql.com/doc/internals/en/event-content-writing-conventions.html

Current implementation ignored this and was hanging waiting for more packets when VARCHAR was larger than 250 bytes.

There is also parseLCB function that is doing this exactly,
but it seems to be not used since commit 01b30f1

this probably fixes #18

fixed decoding LCB value for fields > 250 bytes.
according to this doc:
http://dev.mysql.com/doc/internals/en/event-content-writing-conventions.html

Current implementation ignored this and was hanging waiting for more packets when VARCHAR was larger than 250 bytes.

There is also parseLCB function that is doing this exactly,
but it seems to be not used since commit 01b30f1

this probably fixes mysql-d#18
@Abscissa Abscissa closed this in 1dd2996 May 30, 2014
@Abscissa
Copy link

Thanks for investigating and filing this pull.

The current implementation actually does attempt to handle that part of the protocol, but has a bug:

The line in question: lcb.value = packet.decode!ulong(lcb.numBytes) calls function T decode(T, ubyte N=T.sizeof)(in ubyte[] packet) which does pretty much implement what you refer to. The problem is, when the number of bytes is >1, the indicator byte of "252", "253" or "254" doesn't get skipped like it should. Your patch works because it doesn't make the same mistake.

Instead of introducing more code that does basically the same thing, I've made a commit that properly skips the indicator byte. The result is the same. I do appreciate the patch though, it did help.

I will also remove the existing parseLCB function, it's dead code like you said.

@fsw
Copy link
Author

fsw commented May 30, 2014

Thanks!

Looks much better, I will test this on my case.
Can you also take a look at this line:

https://github.com/rejectedsoftware/mysql-native/blob/1dd2996ca8331e8de735d022b99eb1955e326bdb/source/mysql/connection.d#L1322

It is in "consumeIfComplete" function:

    lcb.value = packet.consume!ulong(lcb.numBytes);

I am not sure when this is called and dont know how to test this but it seems that there is exactly the same mistake there?

cheers.

@Abscissa
Copy link

That line is fine, because just a few lines prior:

https://github.com/rejectedsoftware/mysql-native/blob/1dd2996ca8331e8de735d022b99eb1955e326bdb/source/mysql/connection.d#L1313

It already does exactly what the other function had been missing before we fixed it: If there's at least two bytes, then it skips past the initial "251/252/253/254" length byte using packet.popFront() (which pretty much the same as packet = packet[1..$]).

Admittedly, that consumeIfComplete function is nearly identical to the one we fixed (only difference is that consumeIfComplete consumes the input, whereas the other does not). So I don't know why both exist. Seems messy. I'm sure that could get cleaned up at some point, although I kinda don't wanna mess with it just yet.

@fsw fsw deleted the patch-1 branch May 30, 2014 07:16
@fsw
Copy link
Author

fsw commented May 30, 2014

Great, I see it now.
Thank You.

Abscissa pushed a commit to Abscissa/mysql-native that referenced this pull request Jun 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

select varchar - thinks the package is incomplete while it's actually complete
2 participants