Fix trivial undefined behavior - #63
Conversation
- Guard against division by zero in PrintStats() when log_stats_samples config is set to 0. - Change 'static std::mutex log_mutex' to 'inline std::mutex log_mutex' in logging.h so all TUs share one mutex instance, preventing a data race on the shared log stream. - Replace type-punned uint32_t pointer casts with memcpy in utils.cpp (DetectGzipExt) and iaa.cpp (CompressIAA) to avoid misaligned access and strict-aliasing violations. - In CompressIAA, perform the available_out bounds check before advancing next_out_ptr, avoiding formation of an out-of-bounds pointer. Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>
The division-by-zero guard in PrintStats() gives log_stats_samples=0 a meaning it did not have before: statistics are no longer appended to the log. Document that, along with what a sample is, since a value of 0 is otherwise indistinguishable from a typo in the documented 0-INT_MAX range. Setting log_stats_samples=0 is the only way to stop statistics output without rebuilding: there is no use_statistics option, PrintStats() is called unconditionally from deflate() and inflate() when built with ENABLE_STATISTICS=ON, and LogStats() does not consult log_level. Also add log_stats_samples to config/default_config, which listed every other option in config_names[] but this one. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
LogStats() writes to the stream returned by GetLogStream() without holding any lock, so it can interleave with concurrent Log() calls writing to that same stream. Making log_mutex shared across translation units fixed the Log()-vs-Log() race but left this one. Move log_mutex out of the DEBUG_LOG block into the combined DEBUG_LOG || ENABLE_STATISTICS block so both Log() and LogStats() can lock it. Locking it from LogStats() where it stood would not compile in a DEBUG_LOG=OFF, ENABLE_STATISTICS=ON build, which CI builds. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
input_length is already a uint32_t*, so the temporary is only needed for the computed length that follows. No functional change. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
asonje
left a comment
asonje
left a comment
There was a problem hiding this comment.
All four fixes look correct to me; I've pushed three small follow-ups to the branch
f82fb9e: document log_stats_samples = 0. Your guard gives 0 a new meaning, so the README should say so. Worth noting 0 is the only way to turn statistics off without rebuilding. I also added the option to config/default_config, which
had every other entry in config_names[] but this one.
be69718: lock LogStats() too. The static → inline change fixes Log() vs Log(), but LogStats() writes to the same GetLogStream() with no lock, so the race just moves. I had to move log_mutex up into the DEBUG_LOG || ENABLE_STATISTICS block; locking it from LogStats() where it was wouldn't compile with DEBUG_LOG=OFF, ENABLE_STATISTICS=ON`, which CI builds.
88ad868 — tiny iaa.cpp cleanup. input_length is already a uint32_t*, so the temporary is only needed for the computed length after it.
On the div-by-zero specifically: It exposed a preexisting bug that i will fix in a separate PR.
ENABLE_STATISTICS is declared in the top-level CMakeLists.txt, but tests/CMakeLists.txt only includes common.cmake, so -DENABLE_STATISTICS=ON is silently dropped for the test build.
2 participants
Guard against division by zero in PrintStats() when log_stats_samples config is set to 0.
Change 'static std::mutex log_mutex' to 'inline std::mutex log_mutex' in logging.h so all TUs share one mutex instance, preventing a data race on the shared log stream.
Replace type-punned uint32_t pointer casts with memcpy in utils.cpp (DetectGzipExt) and iaa.cpp (CompressIAA) to avoid misaligned access and strict-aliasing violations.
In CompressIAA, perform the available_out bounds check before advancing next_out_ptr, avoiding formation of an out-of-bounds pointer.