Copilot commented on code in PR #12924:
URL: https://github.com/apache/trafficserver/pull/12924#discussion_r2875083877
##########
plugins/compress/gzip_compress.cc:
##########
@@ -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);
- 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;
Review Comment:
`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.
##########
plugins/compress/compress.cc:
##########
@@ -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;
+ }
Review Comment:
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`.
##########
plugins/compress/gzip_compress.cc:
##########
@@ -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);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
