Skip to content

compress plugin - use configured compression levels for gzip and brotli #12924

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

Open
JakeChampion wants to merge 1 commit into
base: master
Choose a base branch
from
+83 −20
Open

compress plugin - use configured compression levels for gzip and brotli #12924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 36 additions & 11 deletions plugins/compress/brotli_compress.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@

namespace Brotli
{
const int BROTLI_COMPRESSION_LEVEL = 6;
const int BROTLI_LGW = 16;

static bool
compress_operation(Data *data, const char *upstream_buffer, int64_t upstream_length, BrotliEncoderOperation op)
{
Expand Down Expand Up @@ -77,13 +74,7 @@ compress_operation(Data *data, const char *upstream_buffer, int64_t upstream_len
void
data_alloc(Data *data)
{
debug("brotli compression. Create Brotli Encoder Instance.");
data->bstrm.br = BrotliEncoderCreateInstance(nullptr, nullptr, nullptr);
if (!data->bstrm.br) {
fatal("Brotli Encoder Instance Failed");
}
BrotliEncoderSetParameter(data->bstrm.br, BROTLI_PARAM_QUALITY, BROTLI_COMPRESSION_LEVEL);
BrotliEncoderSetParameter(data->bstrm.br, BROTLI_PARAM_LGWIN, BROTLI_LGW);
data->bstrm.br = nullptr;
data->bstrm.next_in = nullptr;
data->bstrm.avail_in = 0;
data->bstrm.total_in = 0;
Expand All @@ -92,10 +83,44 @@ data_alloc(Data *data)
data->bstrm.total_out = 0;
}

bool
transform_init(Data *data)
{
debug("brotli compression: creating Brotli Encoder Instance");
data->bstrm.br = BrotliEncoderCreateInstance(nullptr, nullptr, nullptr);
if (!data->bstrm.br) {
error("brotli-transform: failed to create Brotli Encoder Instance");
return false;
}

int compression_level = data->hc->brotli_compression_level();
int lgwin = data->hc->brotli_lgw_size();

if (!BrotliEncoderSetParameter(data->bstrm.br, BROTLI_PARAM_QUALITY, compression_level)) {
error("brotli-transform: failed to set compression level %d", compression_level);
BrotliEncoderDestroyInstance(data->bstrm.br);
data->bstrm.br = nullptr;
return false;
}

if (!BrotliEncoderSetParameter(data->bstrm.br, BROTLI_PARAM_LGWIN, lgwin)) {
error("brotli-transform: failed to set window size %d", lgwin);
BrotliEncoderDestroyInstance(data->bstrm.br);
data->bstrm.br = nullptr;
return false;
}

debug("brotli compression context initialized with level %d, lgwin %d", compression_level, lgwin);
return true;
}

void
data_destroy(Data *data)
{
BrotliEncoderDestroyInstance(data->bstrm.br);
if (data->bstrm.br) {
BrotliEncoderDestroyInstance(data->bstrm.br);
data->bstrm.br = nullptr;
}
}

void
Expand Down
3 changes: 3 additions & 0 deletions plugins/compress/brotli_compress.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ void data_alloc(Data *data);
// Destroy brotli compression context
void data_destroy(Data *data);

// Configure the context with compression level. Returns true when ready.
bool transform_init(Data *data);

// Compress one chunk of data
void transform_one(Data *data, const char *upstream_buffer, int64_t upstream_length);

Expand Down
21 changes: 21 additions & 0 deletions plugins/compress/compress.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,10 +337,31 @@ compress_transform_init(TSCont contp, Data *data)
data->downstream_vio = TSVConnWrite(downstream_conn, contp, data->downstream_reader, INT64_MAX);
}

// Initialize algorithm-specific compression encoders with configured levels
if ((data->compression_type & (COMPRESSION_TYPE_GZIP | COMPRESSION_TYPE_DEFLATE)) &&
(data->compression_algorithms & (ALGORITHM_GZIP | ALGORITHM_DEFLATE))) {
if (!Gzip::transform_init(data)) {
error("Failed to configure gzip/deflate compression context");
TSHandleMLocRelease(bufp, TS_NULL_MLOC, hdr_loc);
return;
}
Comment on lines +341 to +347
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

When Gzip::transform_init (or Brotli::transform_init) fails and compress_transform_init returns early, data->state has already been set to transform_state_output (line 325). This means compress_transform_do won't call compress_transform_init again, and will proceed to call compress_transform_one and compress_transform_finish, which in turn call Gzip::transform_one / Gzip::transform_finish (or brotli equivalents) on an encoder that was never successfully initialized. This is undefined behavior that can lead to crashes.

The fix should either: (a) change the state to a new "error" state that prevents further processing, or (b) use compress_transform_init's return value to skip subsequent processing in compress_transform_do.

Copilot uses AI. Check for mistakes.
}

#if HAVE_BROTLI_ENCODE_H
if (data->compression_type & COMPRESSION_TYPE_BROTLI && (data->compression_algorithms & ALGORITHM_BROTLI)) {
if (!Brotli::transform_init(data)) {
error("Failed to configure brotli compression context");
TSHandleMLocRelease(bufp, TS_NULL_MLOC, hdr_loc);
return;
}
}
#endif

#if HAVE_ZSTD_H
if (data->compression_type & COMPRESSION_TYPE_ZSTD && (data->compression_algorithms & ALGORITHM_ZSTD)) {
if (!Zstd::transform_init(data)) {
error("Failed to configure Zstandard compression context");
TSHandleMLocRelease(bufp, TS_NULL_MLOC, hdr_loc);
return;
}
}
Expand Down
29 changes: 20 additions & 9 deletions plugins/compress/gzip_compress.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ extern const char *dictionary;

namespace Gzip
{
const int ZLIB_COMPRESSION_LEVEL = 6;
voidpf
gzip_alloc(voidpf /* opaque ATS_UNUSED */, uInt items, uInt size)
{
Expand All @@ -51,11 +50,6 @@ gzip_free(voidpf /* opaque ATS_UNUSED */, voidpf address)
void
data_alloc(Data *data)
{
int window_bits = WINDOW_BITS_GZIP;
if (data->compression_type & COMPRESSION_TYPE_DEFLATE) {
window_bits = WINDOW_BITS_DEFLATE;
}

data->zstrm.next_in = Z_NULL;
data->zstrm.avail_in = 0;
data->zstrm.total_in = 0;
Expand All @@ -66,19 +60,36 @@ data_alloc(Data *data)
data->zstrm.zfree = Gzip::gzip_free;
data->zstrm.opaque = (voidpf) nullptr;
data->zstrm.data_type = Z_ASCII;
}

bool
transform_init(Data *data)
{
int window_bits = WINDOW_BITS_GZIP;
if (data->compression_type & COMPRESSION_TYPE_DEFLATE) {
window_bits = WINDOW_BITS_DEFLATE;
}

int compression_level = data->hc->zlib_compression_level();
debug("gzip compression context initialized with level %d", compression_level);
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

The debug message "gzip compression context initialized with level %d" is printed at line 74, which is before deflateInit2 is actually called at line 76. If deflateInit2 fails, this message will still have been logged, suggesting the context was initialized when it was not. The debug message should be moved to after the successful deflateInit2 call (and after the optional deflateSetDictionary call), just before return true, to accurately reflect initialization success.

Copilot uses AI. Check for mistakes.

int err = deflateInit2(&data->zstrm, ZLIB_COMPRESSION_LEVEL, Z_DEFLATED, window_bits, ZLIB_MEMLEVEL, Z_DEFAULT_STRATEGY);
int err = deflateInit2(&data->zstrm, compression_level, Z_DEFLATED, window_bits, ZLIB_MEMLEVEL, Z_DEFAULT_STRATEGY);

if (err != Z_OK) {
fatal("gzip-transform: ERROR: deflateInit (%d)!", err);
error("gzip-transform: deflateInit2 failed (%d)", err);
return false;
Comment on lines 78 to +80
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

Gzip::data_destroy calls deflateEnd unconditionally, but after this PR's changes, data_alloc no longer calls deflateInit2 — only transform_init does. If transform_init is never called or fails before completing deflateInit2 successfully (the deflateInit2 failure path at line 78-80 returns false without a paired deflateEnd), deflateEnd will eventually be called on an uninitialized z_stream. According to zlib documentation, calling deflateEnd on a stream not initialized by deflateInit/deflateInit2 is undefined behavior. A guard is needed: either track whether deflateInit2 succeeded (e.g., check data->zstrm.state != nullptr, which zlib sets during initialization), or add a boolean flag zstrm_initialized to Data or the gzip module.

Copilot uses AI. Check for mistakes.
}

if (Compress::dictionary) {
err = deflateSetDictionary(&data->zstrm, reinterpret_cast<const Bytef *>(Compress::dictionary), strlen(Compress::dictionary));
if (err != Z_OK) {
fatal("gzip-transform: ERROR: deflateSetDictionary (%d)!", err);
error("gzip-transform: deflateSetDictionary failed (%d)", err);
deflateEnd(&data->zstrm);
return false;
}
}

return true;
}

void
Expand Down
3 changes: 3 additions & 0 deletions plugins/compress/gzip_compress.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ void data_alloc(Data *data);
// Destroy gzip/deflate compression context
void data_destroy(Data *data);

// Configure the context with compression level. Returns true when ready.
bool transform_init(Data *data);

// Compress one chunk of data
void transform_one(Data *data, const char *upstream_buffer, int64_t upstream_length);

Expand Down