Badly handle ArrayBuffer type

[ghostoy]

This issue is originated from nwjs/nw.js#5034. When using Socket.io in latest NW.js, binary data cannot be handles properly.

The root cause is that engine.io-parser doesn't handle ArrayBuffer properly. See lib/index.js:L148-L157.

  if (binaryType === 'arraybuffer') { // <-- [1]
    var type = data[0]; // <-- [2]
    var intArray = new Uint8Array(data.length - 1);
    for (var i = 1; i < data.length; i++) {
      intArray[i - 1] = data[i];
    }
    return { type: packetslist[type], data: intArray.buffer };
  }
  var type = data[0]; // <-- [3] 
  return { type: packetslist[type], data: data.slice(1) };

1. In Node.js or NW.js, binaryType is always null. I assume it's a bug as well.
2. ArrayBuffer cannot be directly read using array styled syntax data[0]. One should construct a TypedArray form the buffer, such as Uint8Array, and read the data inside.
3. I guess here it assumes data could only be a Node Buffer. However, in NW.js data can be of type ArrayBuffer from browser builtin WebSocket.

[ghostoy]

@nuclearace Could you please check this issue?

[nstepien]

This is an issue for me as well when I use socket.io with wsEngine: 'uws'.
This seems to do the trick in the decodePacket function:

  // Binary data
  if (data instanceof ArrayBuffer) {
    var type = Buffer.from(data, 0, 1)[0];
    return {
      type: packetslist[type],
      data: Buffer.from(data, 1, data.byteLength - 1)
    };
  }

I'm not sure if this is the most appropriate fix though.
@alexhultman or @darrachequesne might know better.

I think you are barking up the wrong tree here. This issue is already 2 months old and has no response. Engine.io has completely stopped being further developed.

[nstepien]

Socket.io still depends on engine.io which depends on this parser. ¯_(ツ)_/¯

socket.io hasn't been actively developed in a long time. I have some ancient PR's still not properly merged. The dev doesn't respond so yeah - this issue will probably be fixed by 2057.

[darrachequesne]

@MayhemYDG hi! As usual, providing a failing test case and a fixing PR is the quickest* way to fix something 👼

*: time is relative, isn't it?

[Rainforce15]

since I am writing a test for this now and read a bit of code beyond where the problem crops up:
There already is a test with "should encode/decode an ArrayBuffer as binary", and it tests encode(data:ArrayBuffer, type:"message") with a Buffer as output, and tests decode(data:Buffer, type:"arraybuffer") and an ArrayBuffer as output. Is this intended behaviour?
Because the problem is that some parts are running decode() not with data of type Buffer, but ArrayBuffer (and binaryType==null).

should decode() even handle a 'data' argument of type ArrayBuffer in Node.js? Or does the usage of decode() need fixing?