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

perMessageDeflate causes garbage data #983

Closed
phoboslab opened this issue Jan 31, 2017 · 4 comments
Closed

perMessageDeflate causes garbage data #983

phoboslab opened this issue Jan 31, 2017 · 4 comments

Comments

@phoboslab
Copy link

phoboslab commented Jan 31, 2017

Data of alternating(?) various sizes(?) sent out with perMessageDeflate:true will arrive as garbage in Browsers. This test case reliably fails in some browsers:
http://phoboslab.org/files/bugs/node-ws/

Firefox 51 Windows & OSX: fail
Chrome 56 Windows & OSX: fail
Safari 10 OSX: success
Edge Windows: success

Not really sure if this is a bug in ws or browsers. It's super weird. Tested against ws HEAD (though the test case on phoboslab.org runs on ws 2.0.0).

Given that there's another big issue relating to perMessageDeflate (#804), I'd weigh in towards disabling it by default.

I'd also argue that deflate should remain disabled, as it's surprising behaviour. I'm using ws to send out MPEG1 frames; further compressing these is futile and only costs extra CPU and RAM. I didn't know about this behavior until I stumbled over this bug.

Edit: Well, I guess it's a browser bug. It seems to appear when an uncompressed message is sandwiched between two compressed messages. Setting threshold: 40000 or threshold: 0 (causing all or none of the messages to be compressed) will not cause the bug.

Submitted Chrome/Firefox bug reports:
https://bugs.chromium.org/p/chromium/issues/detail?id=687378
https://bugzilla.mozilla.org/show_bug.cgi?id=1335599

@lpinca
Copy link
Member

lpinca commented Feb 1, 2017

@phoboslab interesting.
It's probably a bug we introduced with version 2.0.0.

If I use ws@1.1.1 with this code:

'use strict';

const WebSocket = require('ws');

const d1 = new Uint8Array(32);
const d2 = new Uint8Array(16);
const server = new WebSocket.Server({
  perMessageDeflate: { threshold: 0 },
  port: 3000
});

server.on('connection', function (ws) {
  ws.send(d1, { compress: true });
  ws.send(d2, { compress: false });
  ws.send(d1, { compress: true });
  ws.close();
});

everything works as expected.

@lpinca
Copy link
Member

lpinca commented Feb 1, 2017

This is definitely a bug on our end. The client receives the following frames:

  1. c2 06 62 60 c0 0f 00 00
  2. 82 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  3. c2 04 22 04 00 00 88 02 03 e8

with ws@1.1.1 and

  1. c2 06 62 60 c0 0f 00 00
  2. 82 04 42 07 00 00
  3. c2 04 22 04 00 00 88 02 03 e8

with ws@2.0.0. The second frame is completely wrong in this case.

@lpinca lpinca closed this as completed in d856dcb Feb 1, 2017
@phoboslab
Copy link
Author

Thanks!

Pardon my ignorance, but shouldn't the 2nd frame be uncompressed, or is it not allowed to mix compressed & uncompressed frames? Do you have any idea why it would work in Safari/Edge?

@lpinca
Copy link
Member

lpinca commented Feb 1, 2017

Yes, the second frame was incorrectly compressed. It shouldn't have been compressed.
It worked on Safari because permessage-deflate is not used. Safari uses a non standard header to negotiate the extension (x-webkit-deflate-frame) and ws does not support that, see #940.

Not sure about Edge, maybe it worked for the same reason.

I also agree that permessage-deflate should be disabled by default but it has been enabled by default since version 0.6 and big users like Socket.IO expect it to be enabled so we chose to keep it enabled by default in version 2.0.0.

Thanks for the report and the test case!

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