sidorares / node-mysql2

:zap: fast mysqljs/mysql compatible mysql driver for node.js
https://sidorares.github.io/node-mysql2/
MIT License
4.07k stars 618 forks source link

[question RE: proxy example] unable to select database #1029

Closed warren-bank closed 5 years ago

warren-bank commented 5 years ago

Hi. Great library!

The following output is produced by a proxy server which uses your wonderful library as both a server and a client (to a remote MariaDB database).

This test fully confirms that the observed behavior isn't the result of anything that I added, since the same occurs in the proxy server example as taken directly from your repo.

Example from the mysql command-line client connected to the proxy:

Welcome to the MariaDB monitor.  Commands end with ; or \g.
Your MySQL connection id is 1
Server version: mysql-proxy mariadb.org binary distribution

Copyright (c) 2000, 2018, Oracle, MariaDB Corporation Ab and others.

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

MySQL [(none)]> show databases;
+--------------------+
| Database           |
+--------------------+
| information_schema |
| mysql              |
| mysql_proxy_test   |
| performance_schema |
| test               |
+--------------------+
5 rows in set (0.005 sec)

MySQL [(none)]> use mysql_proxy_test;
Database changed
MySQL [mysql_proxy_test]> select database();
+------------+
| database() |
+------------+
| NULL       |
+------------+
1 row in set (0.014 sec)

MySQL [mysql_proxy_test]> show tables;
ERROR 1064 (_____): No database selected
MySQL [mysql_proxy_test]>

The fundamental issue is how the mysql client executes "USE db_name;" statements. Apparently, this command isn't sent to the proxy and, consequently, is not forwarded to the remote db connection. Instead, this connection sees: "SELECT DATABASE()".

I can fully confirm that when the "USE db_name;" query is directly passed to the remote db connection.. it works. The problem is that the mysql client doesn't communicate this query.

Do you think this is a bug in the mysql client?

sidorares commented 5 years ago

It might be that when you do "USE db_name" instead of usual COM_QUERY command client sends COM_INIT_DB command. You can handle it using 'init_db' event , see https://github.com/sidorares/node-mysql2/blob/c460c0fe9c9b47282f573801f6e883913752916f/lib/commands/server_handshake.js#L88

warren-bank commented 5 years ago

Great guess.. that appears to be exactly the place to start.

when I add (as a simple test):

  conn.on('init_db', (schemaName) => {
    conn.emit('query', `USE ${schemaName};`)
  })

unfortunately, in the code that you referenced:

  case CommandCode.INIT_DB:
    if (connection.listeners('init_db').length) {
      const schemaName = packet.readString(encoding);
      connection.emit('init_db', schemaName);
    } else {
      connection.writeOk();
    }
    break;

..the mere act of adding a listener causes the truthy branch of your if statement to execute. On my machine, this alone throws an error:

C:\node-mysql-proxy\node_modules\iconv-lite\lib\index.js:106
  throw new Error("Encoding not recognized: '" + encoding + "' (searched as: '"+enc+"')");
                ^

Error: Encoding not recognized: 'undefined' (searched as: 'undefined')
    at Object.getCodec (C:\node-mysql-proxy\node_modules\iconv-lite\lib\index.js:106:23)
    at Object.getDecoder (C:\node-mysql-proxy\node_modules\iconv-lite\lib\index.js:127:23)
    at Object.exports.decode (C:\node-mysql-proxy\node_modules\mysql2\lib\parsers\string.js:10:25)
    at Packet.readString (C:\node-mysql-proxy\node_modules\mysql2\lib\packets\packet.js:407:25)
    at ServerHandshake.dispatchCommands (C:\node-mysql-proxy\node_modules\mysql2\lib\commands\server_handshake.js:87:37)
    at ServerHandshake.execute (C:\node-mysql-proxy\node_modules\mysql2\lib\commands\command.js:39:22)
    at Connection.handlePacket (C:\node-mysql-proxy\node_modules\mysql2\lib\connection.js:408:32)
    at PacketParser.Connection.packetParser.p [as onPacket] (C:\node-mysql-proxy\node_modules\mysql2\lib\connection.js:70:12)
    at PacketParser.executeStart (C:\node-mysql-proxy\node_modules\mysql2\lib\packet_parser.js:75:16)
    at Socket.Connection.stream.on.data (C:\node-mysql-proxy\node_modules\mysql2\lib\connection.js:77:25)

I'll need to trace that encoding variable, and see if I can assign it a valid value (through configuration in my code).

sidorares commented 5 years ago

encoding is set few lines earlier from connection.clientHelloReply.encoding, I wonder why it's undefined ( we might need to set some default value if it's actually can be the case of it not set, or find the reason why it's not there if client sets it )

sidorares commented 5 years ago

Also try to hardcode encoding to be 'utf8' just to see that we are on the right track with init_db and you receive "db_name" as schemaName argument

warren-bank commented 5 years ago

ok.. I'll try that next.

here is a quick report on what I just tried.. and its very interesting result:

  conn.serverHandshake({
    protocolVersion: 10,
    serverVersion:   'mysql-proxy',
    connectionId:    id++,
    statusFlags:     2,
    characterSet:    8,
    capabilityFlags: 0xffffff,
    authCallback:    (data, cb) => {
                         console.log('==================>', data)
                         console.log('==================>', conn.clientHelloReply)
                         cb(null, null)
                     }
  })

..and in my console log, the encoding is set correctly:

MySQL server is listening on port "33306"
==================> { user: 'Admin',
  database: undefined,
  address: '127.0.0.1',
  authPluginData1: <Buffer >,
  authPluginData2: <Buffer >,
  authToken: <Buffer > }
==================> { clientFlags: 10461829,
  maxPacketSize: 16777216,
  charsetNumber: 4,
  encoding: 'cp850',
  user: 'Admin',
  authToken: <Buffer >,
  authPluginName: 'mysql_native_password',
  connectAttributes:
   { _os: 'Windows',
     _client_name: 'libmariadb',
     _pid: '5376',
     _server_host: '127.0.0.1',
     _platform: 'AMD64',
     _client_version: '3.1.4',
     program_name: 'mysql',
     _thread: '5596' } }
warren-bank commented 5 years ago

notes:

now:

sidorares commented 5 years ago

this is server encoding, client also sets client encoding and when server receives client hello it needs to read it correctly. We need to check that it's 1) sent 2) decoded

warren-bank commented 5 years ago

memory refresher:

warren-bank commented 5 years ago

interesting..

  file: C:\node-mysql-proxy\node_modules\mysql2\lib\commands\server_handshake.js
  line: 75
  code: const encoding = 'cp850'
  code: const encoding = 'utf8'

..same error: encoding undefined

warren-bank commented 5 years ago

to confirm that things are sane:

  const encoding = 'utf8'
  console.log('==================>', connection.clientHelloReply)

..output looks pretty much identical to how it did in the authCallback hook function, so the correct encoding string is absolutely being passed to packet.readString(encoding)

warren-bank commented 5 years ago

tracing how encoding passes through code:

https://github.com/sidorares/node-mysql2/blob/769c0023ed3d662ffc054c576bf2d4899ca48b1c/lib/packets/packet.js#L402

  readString(len, encoding) {
    if (typeof len === 'undefined') {
      len = this.end - this.offset;
    }
    this.offset += len;
    return StringParser.decode(
      this.buffer.slice(this.offset - len, this.offset),
      encoding
    );
  }

https://github.com/sidorares/node-mysql2/blob/769c0023ed3d662ffc054c576bf2d4899ca48b1c/lib/parsers/string.js#L5

exports.decode = function(buffer, encoding, options) {
  if (Buffer.isEncoding(encoding)) {
    return buffer.toString(encoding);
  }

  const decoder = Iconv.getDecoder(encoding, options || {});

  const res = decoder.write(buffer);
  const trail = decoder.end();

  return trail ? res + trail : res;
};

https://github.com/ashtuchkin/iconv-lite/blob/8faf11c470794be4ecd2cb81bcd6792f85426cb9/lib/index.js#L126

iconv.getDecoder = function getDecoder(encoding, options) {
    var codec = iconv.getCodec(encoding),
        decoder = new codec.decoder(options, codec);

    if (codec.bomAware && !(options && options.stripBOM === false))
        decoder = new bomHandling.StripBOM(decoder, options);

    return decoder;
}

https://github.com/ashtuchkin/iconv-lite/blob/8faf11c470794be4ecd2cb81bcd6792f85426cb9/lib/index.js#L63

iconv.getCodec = function getCodec(encoding) {
  // ...
  // throw new Error("Encoding not recognized: '" + encoding + "' (searched as: '"+enc+"')");
}
warren-bank commented 5 years ago

https://github.com/ashtuchkin/iconv-lite/blob/8faf11c470794be4ecd2cb81bcd6792f85426cb9/lib/index.js#L77

  var codecDef = iconv.encodings[enc];
  console.log('==================>', {encoding, enc, codecDef})
==================> { encoding: undefined, enc: 'undefined', codecDef: undefined }
warren-bank commented 5 years ago

wrong.. wrong.. wrong.. sorry..

I was wrong: iconv-lite is off the hook.. no "encoding" string if ever passed to their entry point.. wtf??

https://github.com/ashtuchkin/iconv-lite/blob/8faf11c470794be4ecd2cb81bcd6792f85426cb9/lib/index.js#L126

  iconv.getDecoder = function getDecoder(encoding, options) {
    console.log('==================>', [...arguments])
  }
==================> [ undefined, {} ]
warren-bank commented 5 years ago

the calling signature: packet.readString(encoding)

..doesn't match the method signature: readString(len, encoding) {...}

..encoding is passed in place of len

..fix: change calling signature: packet.readString(undefined, encoding)

warren-bank commented 5 years ago

summary:

warren-bank commented 5 years ago

awesome!

after applying that fix.. I retested:

  conn.on('init_db', (schemaName) => {
    conn.emit('query', `USE ${schemaName};`)
  })

..and now "USE db_name;" statements work in the mysql client

sidorares commented 5 years ago

awesome. Can you make a PR? Also maybe try to search where else .readString() could be used incorrectly ( Binlog related packets maybe )

warren-bank commented 5 years ago

happy to do so.

..thanks again for such a great library! :)

sidorares commented 5 years ago

You're welcome. Thanks for your help!

sidorares commented 5 years ago

@warren-bank because there is a big change in master currently don't expect this to be in npm very quickly, I plan to do one or two alpha releases published first and if no major issues publish as 2.0

warren-bank commented 5 years ago

not a problem.. my project is just a proof-of-concept for right now.. nothing mission critical. I'll try to write a monkey patch for until it hits npm.

warren-bank commented 5 years ago

for anyone else interested, this monkey patch works well..

const Packet = require('mysql2/lib/packets/packet')

const rs = Packet.prototype.readString

Packet.prototype.readString = function(len, encoding) {
  if ((typeof len === 'string') && (typeof encoding === 'undefined')) {
    encoding = len
    len = undefined
  }
  return rs.call(this, len, encoding)
}