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

TypeError: "list" argument must be an Array of Buffers #528

Closed
1 of 2 tasks
raulmt opened this issue Aug 30, 2017 · 1 comment
Closed
1 of 2 tasks

TypeError: "list" argument must be an Array of Buffers #528

raulmt opened this issue Aug 30, 2017 · 1 comment

Comments

@raulmt
Copy link
Contributor

raulmt commented Aug 30, 2017

Note: for support questions, please use one of these channels: stackoverflow or slack

You want to:

  • report a bug
  • request a feature

Current behaviour

We've been having this error in production, very unfrequently, but at the same time very important because it just kills the node process and we cannot prevent that from outside, since it's a callback on Node.js internals. Here is the stack trace:

TypeError: "list" argument must be an Array of Buffers
at Function.Buffer.concat (buffer.js:314:13)
at IncomingMessage.onData (/usr/src/app/node_modules/engine.io/lib/transports/polling.js:161:23)
at emitOne (events.js:96:13)
at IncomingMessage.emit (events.js:188:7)
at IncomingMessage.Readable.read (_stream_readable.js:381:10)
at flow (_stream_readable.js:761:34)
at resume_ (_stream_readable.js:743:3)
at _combinedTickCallback (internal/process/next_tick.js:80:11)
at process._tickDomainCallback (internal/process/next_tick.js:128:9)

Steps to reproduce (if the current behaviour is a bug)

Sadly, I couldn't reproduce this locally :( I tried sending a POST body that exceeds the buffer, but onData was not called after destroying the socket. Still, this IS happening in production, so either I'm missing something, or it's really a very weird case or… I don't know… please see "Other information" section for further information.

Expected behaviour

To not generate exception

Setup

Production:

  • OS: Amazon Linux AMI 2017.03 (Fedora/Red Hat like)
  • browser: I don't think it matters much, but this has happen from Chrome and from react-native app
  • engine.io version: We are currently using 1.8.4, but as I said, this specific file is almost identical to current master (in particular the onDataRequest function is exactly the same in both, 1.8.4 and current master)
  • Node.js: 6.11.2 (latest LTS)

Dev (where I tried to reproduce):

  • OS: macOS Sierra 10.12.5
  • browser: curl
  • engine.io: 1.8.4, same as prod
  • Node.js: 6.11.2 too

Other information (e.g. stacktraces, related issues, suggestions how to fix)

We are using an older version of engine.io (and actually, through primus) but this file involved (lib/transports/pollings.js) at least it's the same as the current version in master branch, and it's the only file involved in this stacktrace besides Node.js itself.

The error is triggered because here either chunks or data is not an instance of Buffer. Reading through the code and also reading docs for all the Node.js pieces involved here (Buffer, IncomingMessage, ReadableStream, Socket) there is only one circumstance I can think of for this to happen. Here is my logic: since data argument in onData is either a string or buffer, the problems needs to happen because of chunks not being one. And chunks is assigned only in a handful of places:

  • initial assignement: in order for the bug to trigger, it needs data to not be a string, and that can only happen if req.setEncoding never happens, forcing isBinary to be true, and thus in this line chunks will necessarily be a Buffer
  • inside cleanup: same as above, plus this fn is called after finishing processing, so it shouldn't have anything to do with this
  • data concatenation inside onData: this won't happen since we are talking about a scenario where data is a Buffer, not string.
  • the line triggering the error (previous call to this…): Buffer.concat as per docs will always return a Buffer, so chunks will still be a Buffer.
  • when exceeding maxHttpBufferSize: here the code does something different to any other assignment; it just sets empty string to chunks. So, if we ever call onData again after this happens, we have the scenario where L166 can crash. But, just after setting that, req.connection.destroy(); is called… as far as I understand, connection is the socket, and destroying that means no further data can be read. So, theoretically, no other data event should be fired from the ReadableStream, I guess… but I think this is not necessarily trues, since all of socket and streams related stuff is actually async, right? So, also theoretically, I think it could happen that a new onData call is added to the callback queue just while destroying the socket or even before, and even when destroying the socket will prevent further data processing on that socket, the onData callback is already there ready for the event loop to add it to the stack. So, after this onData call is done, the one already in the callback queue is added to the call stack, and at this point we have a scenario with a data being a Buffer, but chunks being empty string.

So, I can only think of the last one as a cause for this. The solution for that would be very simple: to just use the same logic regarding chunks assignment than in the other two places (chunks = isBinary ? new Buffer(0) : '';). So I'll add that PR in case you think it can help…

@darrachequesne
Copy link
Member

Closed by #529

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

No branches or pull requests

2 participants