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

Fixing packet sequenceId bug and incorrect incoming packet lengh calculations #122

Merged
merged 9 commits into from Feb 13, 2018

Conversation

Projects
None yet
3 participants
@SandorDobi
Copy link
Contributor

SandorDobi commented Feb 11, 2018

Hi,

  1. The outgoing packet sequenceId was broken, it got optimized out from the source, so when it allocates packet the sequenceid field (4. byte of the buffer) contains random data. This PR fix this and set it to zero. I don't touch the handshake sequenceid generation where it is used (yet).
    Later on the packet fragmentation needs to be adressed thought because the servers are using this even when it has no practical usage.

  2. The incoming packet length was incorrectly calculated, therefore all packets bigger than 255 byte likely causing memory errors

  3. fixing dependencies to beta.1 tag

@gwynne
Copy link

gwynne left a comment

These are all largely style notes.

let byte0: UInt32 = numericCast(buffer[0])
let byte1: UInt32 = numericCast(buffer[1])
let byte2: UInt32 = numericCast(buffer[2])
let byte0: UInt32 = (numericCast(buffer[0]) as UInt32).littleEndian

This comment has been minimized.

@gwynne

gwynne Feb 11, 2018

Since you're immediately casting to a specific numeric type, it would be more concise here to write let byte0 = UInt32(buffer[0]). Additionally, the .littleEndian call is a no-op on all little-endian platforms (and thus all platforms supported by Swift).

@@ -46,7 +46,12 @@ extension Packet {

// Require decimal `10` to be the protocol version
guard try parser.byte() == 10 else {
throw MySQLError(.invalidHandshake)
// if the first byte is 0xff then we got an error packet
if(parser.payload[0] == 0xff){

This comment has been minimized.

@gwynne

gwynne Feb 11, 2018

Code style: To match the style found elsewhere in this file, write this line as if parser.payload[0] == 0xff {

@@ -81,13 +85,14 @@ internal final class Packet: ExpressibleByArrayLiteral {
convenience init(arrayLiteral elements: UInt8...) {
let pointer = MutableBytesPointer.allocate(capacity: 4 &+ elements.count)

let packetSizeBytes = [
let packetSizeAndSequenceIdBytes = [
UInt8((elements.count) & 0xff),

This comment has been minimized.

@gwynne

gwynne Feb 11, 2018

Preferentially to adding the (fake) sequence ID to this array, I recommend keeping the original name and memcpy and adding

var sequenceId = UInt8(0)

memcpy(pointer.advanced(by: 3), &sequenceId, 1)

after the original line 90

This comment has been minimized.

@Joannis

Joannis Feb 11, 2018

Member

@gwynne can't you just do pointer[3] = sequenceId ?
Or pointer.avanced(by: 3).pointee = sequenceId

This comment has been minimized.

@gwynne

gwynne Feb 11, 2018

Actually, yes! Though the compiler will generate the same code either way in the end.

SandorDobi added some commits Feb 11, 2018

@gwynne

gwynne approved these changes Feb 11, 2018

SandorDobi and others added some commits Feb 11, 2018

@Joannis Joannis merged commit 64b5253 into vapor:beta Feb 13, 2018

1 of 2 checks passed

ci/circleci: macos Your tests failed on CircleCI
Details
ci/circleci: linux Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment