fix(compress): resolve clean code warnings and fix memory pool bugs by Fencyee · Pull Request #1019 · ModelEngine-Group/unified-cache-management

Fencyee · 2026-06-11T11:44:22Z

Purpose

Fix static analysis warnings in compress_lib and remove unused FSE/HUF/Bitstream source files that are not invoked by the current Compress_Dump/Compress_Load path. Also fix memory pool allocation bugs.

Modifications

Replace deprecated C++ headers (<stdint.h> → <cstdint>, etc.)
Add missing header guards (memory_pool.h, hist.h); extend guards in huf.h/fse.h
Wrap multi-statement macros in do-while(0) (RET_ERROR_IF, CHECK_F, etc.)
Replace unsafe memcpy/memset/memmove with *_s variants via securec.h
Zero-initialize ts_header_t; pass array lengths as function parameters
Extract FixedRatio/DataType enums into compress_types.h
Remove 16 unused FSE/HUF/Bitstream files from compress_lib/
Fix: throw std::bad_alloc on posix_memalign failure and pool exhaustion
Fix: skip submitting corrupted compressed data on TunstallCompressBF16 failure
Fix: remove dead OOM check (std::make_unique throws on failure, never returns null)

ygwpz · 2026-06-13T06:33:17Z

    void* allocate()
    {
        std::lock_guard<std::mutex> lock(mutex_);
+        if (freeBlocks.empty()) { throw std::bad_alloc(); }


🔴 Critical: Potential memory leak in constructor

If freeBlocks.reserve(poolSize) throws std::bad_alloc after posix_memalign succeeds, the allocated memory (pool) will leak because the destructor is not called when a constructor throws an exception.

Recommendation: Use RAII pattern - allocate memory last or wrap pool in a smart pointer/deleter before any operation that can throw.

// Better approach: if (posix_memalign(&pool, 4096, totalSize) != 0) { throw std::bad_alloc(); } std::unique_ptr<void, decltype(&free)> pool_guard(pool, &free); freeBlocks.reserve(poolSize); // if this throws, pool_guard cleans up pool_guard.release(); // ownership transferred to class member

Fixed in memory_pool.h.
Wrapped the posix_memalign result in a std::unique_ptr<void, decltype(&free)> guard before calling freeBlocks.reserve(). If reserve throws, the guard automatically frees the allocation. Ownership is only transferred to the member via .release() after all potentially-throwing operations succeed.

ygwpz · 2026-06-13T06:33:47Z

-#include <limits.h>  // from limits.h import UINT32_MAX
-#include <math.h>    // from math.h import log
-#include <stdlib.h>  // from stdlib.h import qsort
-#include <string.h>  // from string.h import memset, memcpy


🔴 Critical: Secure function return value silently discarded

memcpy_s returns EOK on success and an error code on failure (e.g., EINVAL for invalid parameters, ERANGE for buffer overflow). Casting to (void) discards these errors, potentially leading to data corruption.

Recommendation: Check the return value:

errno_t err = memcpy_s(p_dst, src_len, p_src, src_len); if (err != EOK) { return R_ERR_COPY_FAILED; }

Fixed — defined a SECUREC_CHECK macro and replaced all 6 (void)memcpy_s / (void)memset_s calls with proper error checking that returns R_ERR_COPY_FAILED on failure.

ygwpz · 2026-06-13T06:33:50Z

-#include <string.h>
+#include <cstddef>
+#include <cstdint>
+#include <cstring>


🔴 Critical: Secure function return value silently discarded

memcpy_s returns an error code on failure. Discarding this with (void) means buffer overflow or invalid parameter errors go unnoticed, potentially causing data corruption or memory safety issues.

Recommendation: Check return value:

errno_t err = memcpy_s(p_data + ((i - 1) << 1), sizeof(bf16), &bf16, sizeof(bf16)); if (err != EOK) { return R_ERR_COPY_FAILED; }

Fixed — same SECUREC_CHECK macro applied to all 13 call sites. Also changed decompress_fp8_to_bf16_inplace from void to int so errors can propagate to the caller.

ygwpz · 2026-06-13T06:34:13Z

+#include <cstring>  // from cstring import memset, memcpy
+#include "securec.h"
 #if TS_DEBUG_PRINT
 #include <stdio.h>


Warning: Unused bounds-checking parameters

New parameters idx2symb_len and symb2idx_len are added but immediately suppressed with (void) casts. These should be used for actual bounds validation to prevent buffer overflows:

if (idx2symb_len < p_hist->n_symb) { return R_ERR_DST_OVERFLOW; } if (symb2idx_len < TS_N_SYMB) { return R_ERR_DST_OVERFLOW; }

Fixed — replaced (void)idx2symb_len / (void)symb2idx_len with actual bounds checks:

RET_ERROR_IF(R_ERR_DST_OVERFLOW, idx2symb_len < (size_t)p_hist->n_symb); RET_ERROR_IF(R_ERR_DST_OVERFLOW, symb2idx_len < TS_N_SYMB);

Changed return type from void to int and added RET_WHEN_ERROR at the call site.

ygwpz · 2026-06-13T06:34:16Z

-            Status::NoSpace();
-        }

        for (const UC::Detail::Shard& s : desc) {


Warning: Missing handling for other compression ratios

The ok flag is only set to false when ratio == R200 and compression fails. If ratio is something else (e.g., R133, R139, R145, R152), there is no compression logic at all, but ok remains true and compBytes = n_bf16 is still set.

This means:

For other ratios, compBuf is allocated but never used properly

The data is submitted to backend with potentially incorrect size

Recommendation: Either add compression logic for other ratios, or return early with an error for unsupported ratios.

Fixed — added else branches in both Compress_Dump and Compress_Load that log an error and set ok = false (or continue) for unsupported ratios, preventing silent data corruption if a new ratio is added in the future.

ygwpz · 2026-06-13T06:34:19Z

-#include <stdint.h>
-#include <string.h>
+#include <cstddef>
+#include <cstdint>


Suggestion: Good fix for header initialization

The = {} zero-initialization ensures hdr is fully initialized before memcpy_s overwrites hdr.dynamic. Without this, the union members other than dynamic would contain uninitialized garbage. This is a correct fix for the previous potential undefined behavior.

ygwpz · 2026-06-13T06:34:44Z

 #define RET_ERROR_IF(err_code, condition) \
-    {                                     \
+    do {                                  \
        int c = (condition);              \


Warning: Unused bounds parameter for LUT validation

The lut_size parameter is added to ts_check_lut but suppressed with (void). The function still uses the hardcoded TS_LUT_SIZE constant instead of the parameter. This defeats the purpose of adding the parameter for bounds checking.

Recommendation: Use lut_size instead of TS_LUT_SIZE:

for (; mark < lut_size; mark++) { // ... validate within bounds }

Fixed — removed (void)lut_size and replaced the hardcoded TS_LUT_SIZE with the lut_size parameter in both loops, so the function correctly validates within the caller-specified bounds.

ygwpz · 2026-06-13T06:34:47Z

@@ -1,6 +1,10 @@
+#ifndef MEMORY_POOL_H


Architecture: Good addition of header guard

Adding #ifndef MEMORY_POOL_H / #define MEMORY_POOL_H / #endif prevents multiple inclusion issues and potential ODR violations. This was a missing safeguard in the original code.

ygwpz · 2026-06-13T06:34:49Z

@@ -1,8 +1,9 @@

-#include <limits.h>  // from limits.h import UINT32_MAX
-#include <math.h>    // from math.h import log


Architecture: Correct fix for multi-statement macro

Wrapping RET_ERROR_IF and RET_WHEN_ERROR macros in do { } while(0) ensures they behave like single statements. This prevents issues like:

if (condition) RET_ERROR_IF(code, cond); // Without do-while, only first statement executes else do_something(); // This becomes part of the else!

Good fix for a common C++ macro pitfall.

ygwpz · 2026-06-13T06:35:07Z

            std::make_unique<MemoryPool>(compBufSize, ct.task->desc.size());
-        if (!dump_memoryPool_) {
-            UC_ERROR("COMPRESS DUMP OOM | task_id: {}, required: {} B", ct.task->id,
-                     shardSize_ * desc.size());


Logic: Correct removal of dead OOM check

std::make_unique throws std::bad_alloc on failure - it never returns nullptr. The old code checking if (!dump_memoryPool_) was dead code that would never execute.

Good cleanup. The new MemoryPool constructor now properly throws std::bad_alloc, which will propagate up and be handled appropriately.

ygwpz · 2026-06-13T06:35:09Z

@@ -0,0 +1,621 @@
+/*


Architecture: Adding securec.h for memory safety functions

Note: securec.h provides memcpy_s, memset_s, memmove_s which offer buffer overflow protection by checking destination sizes. However, the current code discards all return values with (void) casts.

Recommendation: Define error handling macros for secure functions:

#define SECUREC_CHECK(err) if (err != EOK) { return R_ERR_SECURE; }

Then use: SECUREC_CHECK(memcpy_s(...));

Fixed with the following approach:

Added R_ERR_COPY_FAILED (value 13) to tunstall.h

Created a new internal header securec_check.h containing the shared SECUREC_CHECK macro (avoids defining it twice in two .cc files, and avoids polluting the public tunstall.h with securec.h dependencies)

Applied SECUREC_CHECK to all memcpy_s / memset_s / memmove_s call sites

- Replace deprecated C++ headers and unsafe memcpy/memset/memmove - Add missing header guards; wrap multi-statement macros in do-while(0) - Zero-initialize ts_header_t; pass array lengths to satisfy bounds checks - Extract FixedRatio/DataType enums to compress_types.h - Remove unused FSE/HUF/Bitstream source files (not invoked by current path) - Fix: throw bad_alloc on posix_memalign failure and pool exhaustion - Fix: skip submitting corrupted data on compression failure

Fencyee requested review from mag1c-h and ygwpz as code owners June 11, 2026 11:44

ygwpz reviewed Jun 13, 2026

View reviewed changes

Fencyee force-pushed the develop branch 2 times, most recently from d4ff481 to 3e3f8b9 Compare June 18, 2026 06:56

Fencyee force-pushed the develop branch from 3e3f8b9 to 81d85a2 Compare June 18, 2026 07:09

		@@ -1,8 +1,9 @@

		#include <limits.h> // from limits.h import UINT32_MAX
		#include <math.h> // from math.h import log

Conversation

Fencyee commented Jun 11, 2026

Purpose

Modifications

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants