fix: handle prepare response with actual number of parameter definition 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

Author: sidorares

.github/workflows/ci-linux.yml

@@ -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

README.md

@@ -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]

lib/commands/prepare.js

@@ -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) {
@@ -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) {

lib/connection.js

@@ -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();
  }
  }
 

test/builtin-runner/regressions/2052.test.mjs

@@ -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);
  }
  });
- */
  });
 });