Re: [PR] Add Zstandard compression support and update tests [trafficserver]

[via GitHub]

bryancall commented on PR #12201:
URL: https://github.com/apache/trafficserver/pull/12201#issuecomment-3474740205

   ## Code Review - ZSTD Compression Implementation
   
   Thank you for this excellent work on adding ZSTD compression support! The 
testing shows the feature works well and achieves great compression ratios 
(98.6%). I've completed a thorough code review and found a few issues that 
should be addressed before merging.
   
   ---
   
   ## 🔴 Critical Issue
   
   ### Resource Management in `zstd_transform_init()`
   
   **Location**: `plugins/compress/compress.cc:558-580`
   
   **Problem**: If `ZSTD_CCtx_setParameter` fails during initialization, the 
function returns early but leaves `data->zstrm_zstd.cctx` in a valid (non-NULL) 
state. The caller at line 395-398 only checks `if (!data->zstrm_zstd.cctx)`, 
which won't catch these initialization failures. This could lead to compression 
proceeding with a misconfigured context.
   
   **Current Code**:
   ```cpp
   static void
   zstd_transform_init(Data *data)
   {
     if (!data->zstrm_zstd.cctx) {
       error("Failed to initialize Zstd compression context");
       return;
     }
   
     size_t result = ZSTD_CCtx_setParameter(data->zstrm_zstd.cctx, 
ZSTD_c_compressionLevel, ...);
     if (ZSTD_isError(result)) {
       error("Failed to set Zstd compression level: %s", 
ZSTD_getErrorName(result));
       return;  // ⚠️ Context is valid but misconfigured
     }
     
     result = ZSTD_CCtx_setParameter(data->zstrm_zstd.cctx, 
ZSTD_c_checksumFlag, 1);
     if (ZSTD_isError(result)) {
       error("Failed to enable Zstd checksum: %s", ZSTD_getErrorName(result));
       return;  // ⚠️ Context exists but checksum not enabled
     }
   }
   ```
   
   **Recommended Fix**:
   ```cpp
   static void
   zstd_transform_init(Data *data)
   {
     if (!data->zstrm_zstd.cctx) {
       error("Failed to initialize Zstd compression context");
       return;
     }
   
     size_t result = ZSTD_CCtx_setParameter(data->zstrm_zstd.cctx, 
ZSTD_c_compressionLevel, data->hc->zstd_compression_level());
     if (ZSTD_isError(result)) {
       error("Failed to set Zstd compression level: %s", 
ZSTD_getErrorName(result));
       ZSTD_freeCCtx(data->zstrm_zstd.cctx);  // ✅ Clean up
       data->zstrm_zstd.cctx = nullptr;        // ✅ Mark as invalid
       return;
     }
     
     result = ZSTD_CCtx_setParameter(data->zstrm_zstd.cctx, 
ZSTD_c_checksumFlag, 1);
     if (ZSTD_isError(result)) {
       error("Failed to enable Zstd checksum: %s", ZSTD_getErrorName(result));
       ZSTD_freeCCtx(data->zstrm_zstd.cctx);  // ✅ Clean up
       data->zstrm_zstd.cctx = nullptr;        // ✅ Mark as invalid
       return;
     }
   
     debug("zstd compression context initialized with level %d", 
data->hc->zstd_compression_level());
   }
   ```
   
   ---
   
   ## ⚠️ Medium Issues
   
   ### 1. Integer Overflow in `zstd_compress_operation()`
   
   **Location**: `plugins/compress/compress.cc:514`
   
   **Problem**: Casting `int64_t upstream_length` to `size_t` without bounds 
checking. If `upstream_length` is negative (malicious input or bug), the cast 
wraps to a huge positive value.
   
   **Current Code**:
   ```cpp
   ZSTD_inBuffer input = {upstream_buffer, 
static_cast<size_t>(upstream_length), 0};
   ```
   
   **Recommended Fix**:
   ```cpp
   if (upstream_length < 0) {
     error("Invalid upstream_length: %" PRId64 " (negative value)", 
upstream_length);
     return false;
   }
   
   if (upstream_buffer == nullptr && upstream_length > 0) {
     error("upstream_buffer is NULL with non-zero length");
     return false;
   }
   
   ZSTD_inBuffer input = {upstream_buffer, 
static_cast<size_t>(upstream_length), 0};
   ```
   
   ### 2. Similar Issue with `downstream_length`
   
   **Location**: `plugins/compress/compress.cc:520`
   
   **Problem**: Same cast issue, though less critical since it comes from ATS 
API.
   
   **Recommended**: Add validation:
   ```cpp
   if (downstream_length < 0) {
     error("Invalid downstream_length from TSIOBufferBlockWriteStart");
     return false;
   }
   ```
   
   ---
   
   ## ℹ️ Minor Issues
   
   ### 1. Typo in Comment
   
   **Location**: `plugins/compress/compress.cc:236`
   
   **Issue**: Comment says "brotlidestory" instead of "brotli destroy"
   
   **Fix**: Change to `// Brotli destroy`
   
   ### 2. Inconsistent Error Logging
   
   **Location**: `plugins/compress/compress.cc:395-398`
   
   **Issue**: Mix of `TSError()` and `error()` macro.
   
   **Current**:
   ```cpp
   if (!data->zstrm_zstd.cctx) {
     TSError("Failed to create Zstandard compression context");  // Uses TSError
     return;
   }
   ```
   
   **Elsewhere**:
   ```cpp
   error("Failed to initialize Zstd compression context");  // Uses error()
   ```
   
   **Recommendation**: Use consistent error logging throughout the ZSTD code - 
prefer `error()` macro for consistency with the rest of the file.
   
   ---
   
   ## ✅ Excellent Practices Observed
   
   1. **Proper error checking**: All ZSTD errors are checked with 
`ZSTD_isError()` and error names are logged
   2. **Resource cleanup**: `ZSTD_freeCCtx()` is properly called in 
`data_destroy()`
   3. **Data integrity**: Checksum is enabled with `ZSTD_c_checksumFlag`
   4. **Infinite loop prevention**: Code checks for no progress in compression 
loop (line 542-544)
   5. **Proper compression modes**: Uses `ZSTD_e_continue` and `ZSTD_e_flush` 
appropriately
   6. **Backward compatibility**: Gracefully handles missing ZSTD libraries
   
   ---
   
   ## Summary
   
   - **Critical Issues**: 1 (must fix before merge)
   - **Medium Issues**: 2 (should fix for robustness)
   - **Minor Issues**: 2 (nice to fix)
   
   **Overall Assessment**: The ZSTD implementation is well-structured and 
follows the ZSTD API correctly. The critical initialization issue should be 
addressed, and the integer overflow checks would significantly improve 
robustness. Great work on achieving excellent compression ratios and 
maintaining backward compatibility!
   
   **Testing**: All three test phases passed successfully ✅
   - Test 1 (Master baseline): GZIP and Brotli working
   - Test 2 (PR without ZSTD): No regressions, graceful fallback
   - Test 3 (PR with ZSTD): ZSTD achieves 98.6% compression (4701 → 68 bytes)
   


-- 
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: github-unsubscr...@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org