Eliminated the possibility for zim::Compressor being trapped in an infinite loop#1047
Eliminated the possibility for zim::Compressor being trapped in an infinite loop#1047
Conversation
Added a test case demonstrating how an infinite loop can be forced onto zim::Compressor.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (75.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1047 +/- ##
=======================================
Coverage 56.25% 56.26%
=======================================
Files 101 101
Lines 5016 5014 -2
Branches 2185 2186 +1
=======================================
- Hits 2822 2821 -1
+ Misses 738 737 -1
Partials 1456 1456 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a reported freeze in libzim where zim::Compressor could get stuck in an infinite loop during Zstd streaming compression when the output buffer becomes exactly full.
Changes:
- Adjusts
zim::Compressor’s encoding loop to grow the output buffer proactively whenavail_out == 0, preventing a non-progressing loop. - Makes
Compressor’s constructorexplicitand adds an invariant check forinitial_size > 0. - Adds a regression unit test intended to reproduce the “buffer exactly filled” scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/compression.cpp | Adds a Zstd-focused regression test for the “output buffer exactly full” case. |
| src/compression.h | Updates Compressor’s constructor and feed() loop to avoid infinite loops when the output buffer is exhausted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| compressor.init(&data[0]); | ||
| compressor.feed(&data[0], DATA_SIZE); | ||
| zim::zsize_t compressedDataSize2; | ||
| const auto out2 = compressor.get_data(&compressedDataSize2); |
| explicit Compressor(size_t initial_size=1024*1024) : | ||
| ret_data(new char[initial_size]), | ||
| ret_size(initial_size) | ||
| {} | ||
| { | ||
| ASSERT(initial_size, >, 0u); | ||
| } |
benoit74
left a comment
benoit74
left a comment
There was a problem hiding this comment.
I barely understand C++ code, but it seems to make sense to avoid the bug.
|
Then, I guess we are good to merge! |
|
Yup, thank you very much! |
Fixes #1046
Added a small unit-test demonstrating the bug and fixed it.
Also checked if
zim::Uncompressorsuffers from a similar bug but to the best of my understanding it is devoid of such defects.