TCP message parsing and ordering about coin-hive-stratum

[procloud]

can also increase default buffer size, but not recommended since attacks can be made to fill out the buffers

[procloud]

for the record, i dont think the buffer size limit is causing any JSON parsing errors. the default buffer is pretty large. from what ive seen, these json's are on the very small size, unable to overload one chunk of data.

however,, ive noticed there is handling as @elmigranto described which can be beneficial for certain conditions. but a default buffer limit should suffice here. anything larger than pool traffic will be non pool traffic.

[cazala]

I don't think the proxy should care about the order of the messages, the client and the server are the ones that should keep track of message ids. When using non-coinhive miners, there's no need for a proxy, and messages go directly from miner to pool and this issue of unsynced messages can still happen, so it's on them to link the messages and replies by their ids.

[elmigranto]

If code were just proxying and simply passing data around, sure, but…

Code does JSON.parse which makes me think it wants to read messages and do some processing, so we probably should care about IDs or, at the very least, receiving full JSON string befor trying to parse it and act on it. Otherwise, why parse at all, just send bytes up and down the stream.

[cazala]

Yes, that's true, but as @procloud said the stratum messages should never go beyond the size of the current buffer. Although something I should add is a way to catch those JSON.parse in case someone sends non-pool data (ie an attacker) or something weird happens like #21 so the proxy doesn't die.

[elmigranto]

Not sure about which buffer was meant, but TCP will autoadjust packet size based on connection stats in real time, so… It also may be the case when multiple JSON lines are in the same packet.

I'm not suggesting to drop try/catch around JSON parsing, my point is to provide it with correct argument, so we never try to parse incomplete or multiple messages. If you are worried that attacker might send you a lot of garbage without new line character, I think it would be better to introduce hard limit on buffer:

socket.on('data', chunk => {
 this.buffer += chunk;

 if (Buffer.byteLength(this.buffer) > MAX_BUF_SIZE) {
   // close connection
   // cleanup socket
 }

 while (this.buffer.includes('\n')) {
   const [jsonLine, rest] = this.buffer.split('\n', 2);
   this.buffer = rest;
   this.emit('message', jsonLine);
 }
});

[elmigranto]

Just to clarify, this.buffer is just an app-level string specific to that socket (const buffer = '' + socket.setEncoding('utf8')). It seems, you can only adjust write socket buffer size on net.Socket in node (or disable it completely), so anything read is either coming from OS or pool settings. Please clarify which buffer is "big enough"?

Also, for client (client <-> proxy) websocket is used, so we get all of this already. So disregard my last post, unless you are worried about malicisous server (proxy <-> pool server).

[cazala]

Yea, actually that could explain #21, those two JSONs pasted together.. I should add something like that to split several messages that arrive in the same packet

[cazala]

I added something similar to this to handle incomplete messages or more than one message in one chunk (b18151d) it is released under version 1.2.0