error while parsing: "should not reach here"

[bradzacher]

~Snip stack:

    at Packet.readLengthCodedNumberExt (/node_modules/mysql2/lib/packets/packet.js:207:11)
    at Packet.readLengthCodedNumber (/node_modules/mysql2/lib/packets/packet.js:157:15)
    at Packet.parseLengthCodedInt (/node_modules/mysql2/lib/packets/packet.js:668:29)

I can't tell exactly what the problem is, but it seems like there's no bounds check on the Packet.readLengthCodedNumber function.

https://github.com/sidorares/node-mysql2/blob/b098d4969a000644d110e29b9aa0bee33cb1dfcb/lib/packets/packet.js#L152-L158

if offset is greater than buffer's length, then byte2 is set to undefined, which breaks the rest of the code.

[bradzacher]

upon inspection - it seems like it occurs if you call one of the conversion functions (string/buffer/geometry) more than once for a field.

If you call one a second time then it essentially triggers the buffer read again without resetting the offset, which screws up the offset for future fields.

There should be handling in place this to prevent it (or an error message).

[sidorares]

Thanks for report @bradzacher

What's the reason you are using typeCast ? I agree in it's current form it's very easy to make mistake and crash everything

[bradzacher]

I use mysql2 to access data for my company's graphql API.

Right now I have two cases:

To reduce code repetition (i.e. having to copy paste a conversion function in every schema), we just handle a boolean conversion in the typecast function. We store booleans as either TINYINT(1) or BIT(1) (and we don't use those types anywhere else in the DB), so typecast checks for either of these and does a conversion to boolean:

if (type === 'TINY' && field.length === 1) {
    return (field.string() !== '0')
} else if (type === 'BIT' && field.length === 1) {
    const buf = field.buffer()

    // handle the case where field is null
    if (buf.length > 0) {
        return (buf.readInt8(0) !== 0)
    }

    return false
}

We have created a custom set of date types in graphql, long story short here - when I do a custom conversion from Date -> string, it has to return a non-null value. However, if the value is null before it reaches my conversion function, graphql lets it through as null.

Unfortunately, if you have a non-null DATE/DATETIME/etc type, which doesn't get set somehow - mysql will auto assign it the value of 0000-00-00 00:00. When mysql2 decodes this value, it passes that string into the JS Date constructor, which returns Invalid Date. Obviously Invalid Date !== null, so my custom function gets called, which can't return null. My work around is to use typeCast to handle this up front (I use moment to make it easier):

const mysqlDateFormats = {
    DATETIME: 'YYYY-MM-DD HH:mm:ss',
    TIMESTAMP: 'YYYY-MM-DD HH:mm:ss',
    DATE: 'YYYY-MM-DD',
    TIME: 'HH:mm:ss',
    YEAR: 'YYYY',
}
const mysqlDateTypes = Object.keys(mysqlDateFormats)

if (mysqlDateTypes.includes(type)) {
    const strVal = field.string()
    const parsed = moment(strVal, mysqlDateFormats[type as keyof typeof mysqlDateFormats], true)

    if (!parsed.isValid()) {
        return null
    }

    return parsed
}

[sidorares]

I think currently what we have as typeCast sits at wrong level. It should be at 'presentation', result building layer, not at de-serealization layer.

Short term solutions to issues you mention: 1) add more boundary checks to all packet functions ( overall good but might slow down speed ) or 2) have special boundary checked .string(), .buffer(), .geometry() readers exposed to typeCast

Long term solution - probably backward incompatible, add some more ways to affect presentation ( similar to rowsAsArray flag ) so there is no need for low level typeCast

[bradzacher]

Personally, I think type cast works fine where it is, because it allows you to save cycles by custom casting before mysql2 even tries to cast.

You probably don't need the bounds checks if you prevent multiple calls to the cast functions for a single field.

[sidorares]

You probably don't need the bounds checks if you prevent multiple calls to the cast functions for a single field.

that might work actually. Still, if you call wrong function ( geometry() for example when string is expected ) you end up with parser in undefined state

[hcd660]

use mysql 5.7