Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "RangeError: Maximum call stack size exceeded" in hasBinary #109

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

ayan4m1
Copy link

@ayan4m1 ayan4m1 commented Aug 21, 2021

Background

serialport 9.2.0
socket.io 4.3.0
import { createServer } from 'socket.io';
import SerialPort from 'serialport';

const serialPort = new SerialPort('/dev/ttyUSB0');
const io = createServer(8080);

serialPort.on('open', () => {
  io.on('connection', (socket) => {
    // this works
    serialPort.on('data', (data) => {
      socket.emit('data', data);
    });
    // but this does not
    serialPort.pipe(socket);
  });
});

When piping a SerialPort instance to a socket.io Socket, the following occurs:

RangeError: Maximum call stack size exceeded
    at hasBinary (/code/project/node_modules/socket.io-parser/dist/is-binary.js:28:19)
    at hasBinary (/code/project/node_modules/socket.io-parser/dist/is-binary.js:49:63)
    at hasBinary (/code/project/node_modules/socket.io-parser/dist/is-binary.js:49:63)

Investigating further, I found this issue which was resolved by patching socket.io-parser like this. I cleaned up and modernized that patch for this repo. All credit to the original authors for their fix.

Changes

  • Use flatted to parse and stringify JSON, allowing circular objects to be handled
  • Prevent infinite recursion in _deconstructPacket
  • Prevent infinite recursion in hasBinary
  • Detect Buffer instances as binary
  • Add type hints to reconstructPacket
  • Update existing test case now that exceptions are not thrown for circular objects
  • Add test case to ensure that circular data is flattened appropriately

I tried to fix the RangeError without adding flatted, but it seems to be necessary as part of this solution. Hopefully the small additional dependency is worth the benefits in terms of preventing infinite recursion.

@darrachequesne
Copy link
Member

Hi! Thanks for your work on this.

I'm afraid of the increase of the package size in the browser, but we could release this under another package name (and provide it as a custom parser). What do you think?

@MarcGodard
Copy link

I seem to be having the issue this is fixing.

@ayan4m1
Copy link
Author

ayan4m1 commented Sep 6, 2021

Hi! Thanks for your work on this.

I'm afraid of the increase of the package size in the browser, but we could release this under another package name (and provide it as a custom parser). What do you think?

flatted is 0.5kb gzipped - it wouldn't be a concern for me, but up to you as to how it's handled.

It could potentially be broken out as two separate changes - handling circular JSON structures (which would be a custom parser) and "fixing the RangeError itself" (which would go into socket.io-parser) but I wasn't able to successfully separate the concerns on my first try.

I'd rather try again to split them up into separate branches than make a custom parser which overrides bits and pieces of the existing one (I can see that being fairly messy) in order to both fix the RangeError and provide circular JSON support.

@darrachequesne Any thoughts?

darrachequesne and others added 9 commits April 18, 2022 00:20
A specially crafted packet could be incorrectly decoded.

Example:

```js
const decoder = new Decoder();

decoder.on("decoded", (packet) => {
  console.log(packet.data); // prints [ 'hello', [Function: splice] ]
})

decoder.add('51-["hello",{"_placeholder":true,"num":"splice"}]');
decoder.add(Buffer.from("world"));
```

As usual, please remember not to trust user input.
zuul is now archived [1] and does not support the new W3C WebDriver
protocol, since it relies on the wd package [2] under the hood, which
uses the (now deprecated) JSON Wire Protocol.

We will now use the webdriver.io test framework, which allows to run
our tests in local and on Sauce Labs (cross-browser and mobile tests).
This allows us to run our tests on latest versions of Android and iOS,
since Sauce Labs only supports the W3C WebDriver protocol for these
platforms ([3]).

[1]: https://github.com/defunctzombie/zuul
[2]: https://github.com/admc/wd
[3]: https://docs.saucelabs.com/dev/w3c-webdriver-capabilities/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants