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(node/zlib): accept dataview and buffer in zlib bindings #21756

Merged
merged 7 commits into from
Jan 3, 2024

Conversation

JoviDeCroock
Copy link
Contributor

@JoviDeCroock JoviDeCroock commented Jan 2, 2024

fixes #20516
Follow up to #21747 and #21746

This tackles the last point of #20516 where certain inputs weren't accepted in the other zlib methods

This adds the toU8 conversion of _brotli to _zlib.mjs, when we create the ZLibBuffer, we'll sanitize the input. I noticed that the async had no handler for string input so I added that as well.

Not sure what is going wrong here for the parallel test 😅

@JoviDeCroock JoviDeCroock marked this pull request as draft January 2, 2024 06:36
@JoviDeCroock JoviDeCroock force-pushed the accept-view-buffer branch 3 times, most recently from 32e9e68 to a10ccfd Compare January 2, 2024 07:11
@JoviDeCroock JoviDeCroock marked this pull request as ready for review January 2, 2024 07:26
@JoviDeCroock JoviDeCroock force-pushed the accept-view-buffer branch 3 times, most recently from 2666caf to 65e5bce Compare January 2, 2024 07:39
@bartlomieju
Copy link
Member

@littledivy please review

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@littledivy
Copy link
Member

@JoviDeCroock The following Node compat test is failing:

Node.js compatibility ... Node.js compatibility "parallel/test-zlib-truncated.js" => ./cli/tests/node_compat/test.ts:58:11
error: AssertionError: Failed assertion: "parallel/test-zlib-truncated.js" failed:

error: Uncaught AssertionError: Missing expected exception.
    at new AssertionError (ext:deno_node/assertion_error.ts:414:11)
    at Function.throws (node:assert:133:11)
    at file:///home/runner/work/deno/deno/cli/tests/node_compat/test/parallel/test-zlib-truncated.js:49:12
    at Gzip.onEnd (ext:deno_node/_zlib.mjs:212:5)
    at Gzip.emit (ext:deno_node/_stream.mjs:1859:11)
    at endReadableNT (ext:deno_node/_stream.mjs:3648:16)
    at Array.processTicksAndRejections (ext:deno_node/_next_tick.ts:33:15)
    at eventLoopTick (ext:core/01_core.js:180:29)

@JoviDeCroock
Copy link
Contributor Author

@littledivy Yes I can see that 😅 as mentioned in the description I can't quite tell why or what is failing as the whole failure is very vaguely worded

Comment on lines 166 to 168
if (input.buffer) {
input = new Uint8Array(input.buffer);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (input.buffer) {
input = new Uint8Array(input.buffer);
}
if (input.buffer) {
input = new Uint8Array(input.buffer);
}

I'm not sure if this is the reason for test failure but the input view should respect offsets.

input = new Uint8Array(input.buffer, input.byteOffset, input.byteLength)

In the test, the compressed buffer is truncated and passed to decompress(). It expects decompress() to fail.

// Sync truncated input test
assert.throws(function() {
zlib[methods.decompSync](truncated);
}, errMessage);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that did the trick ye

@littledivy littledivy merged commit f5ad15b into denoland:main Jan 3, 2024
14 checks passed
@JoviDeCroock JoviDeCroock deleted the accept-view-buffer branch January 3, 2024 13:34
bartlomieju pushed a commit that referenced this pull request Jan 4, 2024
Fixes #20516 
Follow up to #21747 and #21746

This tackles the last point of #20516 where certain inputs weren't
accepted in the other zlib methods

This adds the `toU8` conversion of `_brotli` to `_zlib.mjs`, when we
create the ZLibBuffer, we'll sanitize the input. I noticed that the
async had no handler for `string` input so I added that as well.
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.

node:zlib module has numerous bugs, inconsistencies, and errors
3 participants