node:zlib fails to decompress brotli data larger than 4096 bytes

[honnip]

Steps to reproduce

import { brotliCompressSync, brotliDecompressSync } from "node:zlib";

const a = "a".repeat(4097);
const compressed = brotliCompressSync(a);
const decompressed = brotliDecompressSync(compressed); // error: Uncaught TypeError: Failed to decompress

This works with "a".repeat(4096)

version

$ deno -V
deno 1.35.0

[lachrimae]

I have determined why this happens. The Rust code FFIs out to this C function. On line 2283, you can see that all non-success results are coerced to the same generic error code BROTLI_DECODER_RESULT_ERROR. On the Rust side, we only attempt to double the buffer size when we get a BROTLI_DECODER_RESULT_NEEDS_MORE_OUTPUT result from the C code. But as mentioned, that result is always coerced away to the generic case.

Possible solutions are to upstream a change to google/brotli that stops coercing errors to the same generic case. Or to use CBrotliDecoderDecompressStream, which does not perform this error-coercing step.

[lachrimae]

Upstreaming seems like a bad idea, this has been the API for something like 8 years and it's documented, so not a mistake.

When I rewrite brotli_decompress as

fn brotli_decompress(buffer: &[u8]) -> Result<ToJsBuffer, AnyError> {
  let mut output = Vec::with_capacity(4096);
  let mut decompressor = Decompressor::new(buffer, buffer.len());
  decompressor.read_to_end(&mut output)?;
  Ok(output.into())
}

it passes the test in this issue. @littledivy is there a reason you wrote new unsafe FFI code here instead of using the Rust API in the brotli crate?

[lachrimae]

This is the PR where this code was introduced by the way: #19223

[lachrimae]

Okay I have looked a little bit more closely into the brotli crate being used, and I realized it's actually Dropbox's rust-brotli library. The rust-brotli crate provides a C FFI interface, allowing rust-brotli to replace google/brotli as a drop-in replacement. What we are doing here in Deno is calling into the rust-brotli C FFI layer, which is immediately calling back into Rust. Everything I said about error types being coerced is true, but it's happening here in rust-brotli.

We should definitely switch this over to the Rust API because the unsafe C API is just a layer of indirection on top of Rust.