Fix zlib reset misuse on maxPayload error to prevent callback race and incorrect error code#2285
Merged
lpinca merged 2 commits intowebsockets:masterfrom May 2, 2025
Merged
Conversation
Member
|
Good catch. There is no race condition, it is just undocumented behavior of 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();
}
/**
|
lpinca
reviewed
May 1, 2025
Contributor
Author
|
The workflow fails because of the timeout. on 12 ubuntu |
Amirali-Amirifar
force-pushed
the
fix/perMessageDeflate
branch
from
1f0cc38 to
a96521a
Compare
Contributor
Author
|
Seems like a breaking change, on node 10 and 12. |
Member
|
Yes, it seems that the 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);
}
|
…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 <[email protected]>
Amirali-Amirifar
force-pushed
the
fix/perMessageDeflate
branch
from
a96521a to
ce6b539
Compare
Contributor
Author
|
@lpinca I have added the updated logic, also squashed the commits. The tests now pass. |
lpinca
reviewed
May 2, 2025
…d error. - Refer to previous commit - Ensure Both paths, flush and error are tested
lpinca
approved these changes
May 2, 2025
Member
|
Thank you. |
lpinca
pushed a commit
that referenced
this pull request
May 2, 2025
Ensure that the correct error is surfaced when the max message size is exceeded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
maxPayloadsize, the internalzlib.InflateRawstream 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()withthis.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 exceededand would show: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.