Skip to content

Commit

Permalink
fix: handle prepare response with actual number of parameter definiti…
Browse files Browse the repository at this point in the history
…on less than reported in the prepare header. Fixes #2052

* chore: route synchronous exceptions from packet deserialisation as a fatal connection error

* chore: initial steps in migration to builtin node test runner

* chore: add test:builtin-node-runner npm script

* add a test case reproducing #2052 issue

* add mysql:8.0.33 to linux matrix

* debug test execution

* cleanup debug log

* simple connection test

* simple connection test

* end connection in the test

* add timeout

* fix: handle prepare response with actual number of parameter definition less than reported in the prepare header. Fixes #2052

* test commit

* lint
  • Loading branch information
sidorares authored Jul 5, 2023
1 parent 2c27961 commit b658be0
Show file tree
Hide file tree
Showing 5 changed files with 163 additions and 24 deletions.
6 changes: 3 additions & 3 deletions .github/workflows/ci-linux.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,20 @@ jobs:
fail-fast: false
matrix:
node-version: [16.x, 18.x, 20.x]
mysql-version: ["mysql:8.0.18", "mysql:8.0.22", "mysql:5.7"]
mysql-version: ["mysql:8.0.18", "mysql:8.0.22", "mysql:8.0.33", "mysql:5.7"]
use-compression: [0]
use-tls: [0]
mysql_connection_url_key: [""]
# TODO - add mariadb to the matrix. currently few tests are broken due to mariadb incompatibilities
include:
# 20.x
- node-version: "20.x"
mysql-version: "mysql:8.0.18"
mysql-version: "mysql:8.0.33"
use-compression: 1
use-tls: 0
use-builtin-test-runner: 1
- node-version: "20.x"
mysql-version: "mysql:8.0.18"
mysql-version: "mysql:8.0.33"
use-compression: 0
use-tls: 1
use-builtin-test-runner: 1
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
## Node MySQL 2
## MySQL 2

[![Greenkeeper badge](https://badges.greenkeeper.io/sidorares/node-mysql2.svg)](https://greenkeeper.io/)
[![NPM Version][npm-image]][npm-url]
Expand Down
14 changes: 13 additions & 1 deletion lib/commands/prepare.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,19 @@ class Prepare extends Command {
return Prepare.prototype.readField;
}
return this.prepareDone(connection);

}

readParameter(packet, connection) {
// there might be scenarios when mysql server reports more parameters than
// are actually present in the array of parameter definitions.
// if EOF packet is received we switch to "read fields" state if there are
// any fields reported by the server, otherwise we finish the command.
if (packet.isEOF()) {
if (this.fieldCount > 0) {
return Prepare.prototype.readField;
}
return this.prepareDone(connection);
}
const def = new Packets.ColumnDefinition(packet, connection.clientEncoding);
this.parameterDefinitions.push(def);
if (this.parameterDefinitions.length === this.parameterCount) {
Expand All @@ -86,6 +95,9 @@ class Prepare extends Command {
}

readField(packet, connection) {
if (packet.isEOF()) {
return this.prepareDone(connection);
}
const def = new Packets.ColumnDefinition(packet, connection.clientEncoding);
this.fields.push(def);
if (this.fields.length === this.fieldCount) {
Expand Down
23 changes: 16 additions & 7 deletions lib/connection.js
Original file line number Diff line number Diff line change
Expand Up @@ -489,14 +489,23 @@ class Connection extends EventEmitter {
}
this._bumpSequenceId(packet.numPackets);
}
const done = this._command.execute(packet, this);
if (done) {
this._command = this._commands.shift();
if (this._command) {
this.sequenceId = 0;
this.compressedSequenceId = 0;
this.handlePacket();
try {
if (this._fatalError) {
// skip remaining packets after client is in the error state
return;
}
const done = this._command.execute(packet, this);
if (done) {
this._command = this._commands.shift();
if (this._command) {
this.sequenceId = 0;
this.compressedSequenceId = 0;
this.handlePacket();
}
}
} catch (err) {
this._handleFatalError(err);
this.stream.destroy();
}
}

Expand Down
142 changes: 130 additions & 12 deletions test/builtin-runner/regressions/2052.test.mjs
Original file line number Diff line number Diff line change
@@ -1,29 +1,147 @@
import { describe, it } from 'node:test';
import { describe, it, beforeEach, afterEach } from 'node:test';
import assert from 'node:assert';
// import common from '../../common.js';

describe('Prepare result when CLIENT_OPTIONAL_RESULTSET_METADATA is set or metadata_follows is unset', () => {
it('should not throw an exception', (t, done) => {
assert(true);
done(null);
/*
const connection = common.createConnection({
database: 'mysql',
import common from '../../common.js';
import PrepareCommand from '../../../lib/commands/prepare.js';
import packets from '../../../lib/packets/index.js';

describe(
'Unit test - prepare result with number of parameters incorrectly reported by the server',
{ timeout: 1000 },
() => {
it('should report 0 actual parameters when 1 placeholder is used in ORDER BY ?', (t, done) => {
const connection = {
constructor: {
statementKey: () => 0,
},
_handshakePacket: {},
_resetSequenceId: () => {},
_statements: new Map(),
serverEncoding: 'utf8',
clientEncoding: 'utf8',
config: {
charsetNumber: 33,
},
writePacket: (packet) => {
// client -> server COM_PREPARE
assert.equal(
packet.buffer.toString('hex'),
'000000001673656c656374202a2066726f6d207573657273206f72646572206279203f'
);
},
};
const prepareCommand = new PrepareCommand(
{ sql: 'select * from users order by ?' },
(err, result) => {
assert.equal(err, null);
debugger;
assert.equal(result.parameters.length, 0);
assert.equal(result.columns.length, 51);
assert.equal(result.id, 1);
done(null);
}
);

prepareCommand.execute(null, connection);
const headerPacket = new packets.Packet(
0,
Buffer.from('0000000000010000003300010000000005000002', 'hex'),
0,
20
);
prepareCommand.execute(headerPacket, connection);
const paramsEofPacket = new packets.Packet(0, Buffer.from('00000000fe000002002b000004', 'hex'), 0, 11);
prepareCommand.execute(paramsEofPacket, connection);
for (let i = 0; i < 51; ++i) {
const columnDefinitionPacket = new packets.Packet(
0,
Buffer.from(
'0000000003646566056d7973716c0475736572047573657204486f737404486f73740ce000fc030000fe034000000005000005', 'hex'
),
0,
47
);
prepareCommand.execute(columnDefinitionPacket, connection);
}
const columnsEofPacket = new packets.Packet(0, Buffer.from('00000000fe000002002b000004', 'hex'), 0, 11);
prepareCommand.execute(columnsEofPacket, connection);
});
}
);

describe('E2E Prepare result with number of parameters incorrectly reported by the server', { timeout: 1000 }, () => {
let connection;

function isNewerThan8_0_22() {
const { serverVersion } = connection._handshakePacket;
const [major, minor, patch] = serverVersion.split('.').map((x) => parseInt(x, 10));
if (major > 8) {
return true;
}
if (major === 8 && minor > 0) {
return true;
}
if (major === 8 && minor === 0 && patch >= 22) {
return true;
}
return false;
}

beforeEach((t, done) => {
connection = common.createConnection({
database: 'mysql',
});
connection.on('error', (err) => {
done(err);
});
connection.on('connect', () => {
done(null);
});
});

afterEach((t, done) => {
connection.end(err => {
done(null)
});
});

// see https://github.com/sidorares/node-mysql2/issues/2052#issuecomment-1620318928
it('should report 0 actual parameters when 1 placeholder is used in ORDER BY ?', (t, done) => {
connection.prepare('select * from user order by ?', (err, stmt) => {
console.log(err);
if (err) {
if (!err.fatal) {
connection.end();
}
done(err);
} else {
if(isNewerThan8_0_22()) {
// mysql 8.0.22 and newer report 0 parameters
assert.equal(stmt.parameters.length, 0);
} else {
// mariadb, mysql 8.0.21 and older report 1 parameter
assert.equal(stmt.parameters.length, 1);
}
done(null);
}
});
});

it('should report 1 actual parameters when 2 placeholders used in ORDER BY?', (t, done) => {
connection.prepare('select * from user where user.User like ? order by ?', (err, stmt) => {
if (err) {
if (!err.fatal) {
connection.end();
}
done(err);
} else {
if(isNewerThan8_0_22()) {
// mysql 8.0.22 and newer report 1 parameter
assert.equal(stmt.parameters.length, 1);
} else {
// mariadb, mysql 8.0.21 and older report 2 parameters
assert.equal(stmt.parameters.length, 2);
}
done(null);
}
});
*/
});
});

0 comments on commit b658be0

Please sign in to comment.