8000 Fix zlib reset misuse on maxPayload error to prevent callback race and incorrect error code by Amirali-Amirifar · Pull Request #2285 · websockets/ws · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix zlib reset misuse on maxPayload error to prevent callback race and incorrect error code #2285

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 2, 2025
Merged

Fix zlib reset misuse on maxPayload error to prevent callback race and incorrect error code #2285

merged 2 commits into from
May 2, 2025

Conversation

Amirali-Amirifar
Copy link
Contributor

Description

This merge request fixes a long-standing issue in the handling of maxPayload violations during permessage-deflate decompression in the ws package.

Previously, when the payload exceeded the configured maxPayload size, the internal zlib.InflateRaw stream was reset via .reset() after removing the 'data' listener. However, this approach left unprocessed data in zlib's internal buffer queue. On large payloads (e.g., zip bombs that inflate to hundreds of megabytes), this residual data could be processed after the reset, causing zlib to throw a fatal error (Z_DATA_ERROR) due to missing dictionary state. note that the payload must be large enough so that zlib should fill its buffer and chunks in the buffer should have back-refrence to chunks before .reset().

Changes

Replaced this.reset() with this.close() when the maxPayload is exceeded.

This ensures the inflater is safely and fully cleaned up, avoiding race conditions and crashes due to leftover queued chunks.

Fixes the misleading error code: previously, zlib would throw a generic decompression error with status 1007. We now correctly emit a RangeError with status 1009 as per RFC 6455, Section 7.4.1 for "Message too big".

Security

There is no security impact if the consumer handles the 'error' event properly on the WebSocket. However, without this fix, status code on large payloads would not be Max payload size exceeded and would show:

Error: invalid code lengths set
    at genericNodeError (node:internal/errors:984:15)
    at wrappedFn (node:internal/errors:538:14)
    at Zlib.zlibOnError [as onerror] (node:zlib:190:17) {
  errno: -3,
  code: 'Z_DATA_ERROR',
  [Symbol(status-code)]: 1007
}

Testing

Added a test to ensure proper error message is shown when maxPayload is exceeded on large payloads (1GB).
Since this is a race condition (between zlib buffer and reset), I generated a large test case ensure this is always checked. also better resource cleanup on high traffic environments.

Why this matters
This bug has existed undetected for over 8 years due to being masked by zlib's internal buffering behavior and only triggers under high-inflation payloads. Fixing this ensures:

Proper lifecycle management of the inflater.

Compliance with WebSocket error handling standards.

Stability and robustness under malicious or extreme input conditions.

Let me know if you'd like to add credits or reference an issue number.

@lpinca
Copy link
Member
lpinca commented May 1, 2025

Good catch. There is no race condition, it is just undocumented behavior of inflate.reset(). I think it should just discard the data without raising an error. Anyway, inflate.close() is already called when the inflate.flush() callback is called so I think it is ok to call it sooner. Can you please update as follows?

diff --git a/lib/permessage-deflate.js b/lib/permessage-deflate.js
index 77d918b..62bded4 100644
--- a/lib/permessage-deflate.js
+++ b/lib/permessage-deflate.js
@@ -366,7 +366,9 @@ class PerMessageDeflate {
       const err = this._inflate[kError];
 
       if (err) {
-        this._inflate.close();
+        //
+        // The stream was already closed in `inflateOnData()`.
+        //
         this._inflate = null;
         callback(err);
         return;
@@ -494,7 +496,7 @@ function inflateOnData(chunk) {
   this[kError].code = 'WS_ERR_UNSUPPORTED_MESSAGE_LENGTH';
   this[kError][kStatusCode] = 1009;
   this.removeListener('data', inflateOnData);
-  this.reset();
+  this.close();
 }
 
 /**

@Amirali-Amirifar
Copy link
Contributor Author
Amirali-Amirifar commented May 1, 2025 •
edited
Loading

The workflow fails because of the timeout. on 12 ubuntu

  1) PerMessageDeflate
       #compress and #decompress
         doesn't call the callback twice when `maxPayload` is exceeded:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/ws/ws/test/permessage-deflate.test.js)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

  2) Receiver
       emits an error if the message length exceeds `maxPayload`:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/ws/ws/test/receiver.test.js)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

@Amirali-Amirifar Amirali-Amirifar force-pushed the fix/perMessageDeflate branch from 1f0cc38 to a96521a Compare May 1, 2025 10:00
@Amirali-Amirifar
Copy link
Contributor Author
Amirali-Amirifar commented May 1, 2025 •
edited
Loading

Seems like a breaking change, on node 10 and 12.

@lpinca
Copy link
Member
lpinca commented May 1, 2025 •
edited
Loading

Yes, it seems that the inflate.flush() callback is not called. The following patch should work.

diff --git a/lib/permessage-deflate.js b/lib/permessage-deflate.js
index 77d918b..41ff70e 100644
--- a/lib/permessage-deflate.js
+++ b/lib/permessage-deflate.js
@@ -494,6 +494,14 @@ function inflateOnData(chunk) {
   this[kError].code = 'WS_ERR_UNSUPPORTED_MESSAGE_LENGTH';
   this[kError][kStatusCode] = 1009;
   this.removeListener('data', inflateOnData);
+
+  //
+  // The choice to employ `zlib.reset()` over `zlib.close()` is dictated by the
+  // fact that in Node.js versions prior to 13.10.0, the callback for
+  // `zlib.flush()` is not called if `zlib.close()` is used. Utilizing
+  // `zlib.reset()` ensures that either the callback is invoked or an error is
+  // emitted.
+  //
   this.reset();
 }
 
@@ -509,6 +517,12 @@ function inflateOnError(err) {
   // closed when an error is emitted.
   //
   this[kPerMessageDeflate]._inflate = null;
+
+  if (this[kError]) {
+    this[kCallback](this[kError]);
+    return;
+  }
+
   err[kStatusCode] = 1007;
   this[kCallback](err);
 }

@Amirali-Amirifar @lpinca
Fix(perMessageDeflate): Resolve error handling for large payloads exc…
ce6b539 
…eeding maxPayload.

This resolves an RFC non-compliance issue where certain large, compressed
payloads could return Z_DATA_ERROR(1007) instead of WS_ERR_UNSUPPORTED_MESSAGE_LENGTH(1009) during permessage-deflate decompression.
The updated logic ensures compatibility across Node.js versions < 13.10.0 while
preserving correct error handling for inflated data.

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
@Amirali-Amirifar Amirali-Amirifar force-pushed the fix/perMessageDeflate branch from a96521a to ce6b539 Compare May 1, 2025 21:14
@Amirali-Amirifar
Copy link
Contributor Author

@lpinca I have added the updated logic, also squashed the commits. The tests now pass.

@Amirali-Amirifar
Test(perMessageDeflate): Add more test cases for maxPayload exceede…
7283e33 
…d error.

- Refer to previous commit
- Ensure Both paths, flush and error are tested
@lpinca lpinca merged commit 4f20aed into websockets:master May 2, 2025
62 checks passed
@lpinca
Copy link
Member
lpinca commented May 2, 2025

Thank you.

lpinca pushed a commit that referenced this pull request May 2, 2025
@Amirali-Amirifar @lpinca
[fix] Handle oversized messages with designated error (#2285)
2137243 
Ensure that the correct error is surfaced when the max message size is
exceeded.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Amirali-Amirifar @lpinca
0