fix files straddling multipart boundary#231
Conversation
When processing a split (multi-volume) compressed file, if a read request completes exactly at a volume boundary (consuming the last byte, volume_bytes_left == 0), the next call to unshield_reader_read() calculates bytes_to_read = MIN(requested, 0) = 0 and immediately errors out. unshield_file_save_old() handles this properly, but the "new" path does not.
|
This happened with a 20 year old multipart installer, works just fine with this fix, thanks for this great tool! |
|
Nice catch! Could you share a link to it so I can test myself? |
There was a problem hiding this comment.
Pull request overview
Fixes extraction of split (multi-volume) cabinet files when a prior read ends exactly on a volume boundary, ensuring the next unshield_reader_read() call can advance to the next volume instead of failing with bytes_to_read == 0.
Changes:
- In
unshield_reader_read(), detect theFILE_SPLIT && volume_bytes_left == 0condition whenbytes_to_readcomputes to 0. - Attempt to open the next volume and continue the read loop rather than erroring immediately.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!unshield_reader_open_volume(reader, reader->volume + 1)) | ||
| { | ||
| unshield_error("bytes_to_read is zero and failed to open next volume %i", | ||
| reader->volume + 1); | ||
| goto exit; | ||
| } |
There was a problem hiding this comment.
After successfully calling unshield_reader_open_volume(), this loop assumes reader->volume_bytes_left will become > 0. However, unshield_reader_open_volume() can return success without setting reader->volume_bytes_left (and without updating reader->volume) when the split file index is neither the first nor last file in that volume (lib/file.c:469). In that case, this new continue path will repeatedly try to open the same next volume and can spin forever. Consider guarding against this by verifying that opening the next volume actually advanced reader state (e.g., reader->volume_bytes_left > 0 / reader->volume changed) and failing fast otherwise, or by adjusting unshield_reader_open_volume() so a successful return always updates reader->volume and reader->volume_bytes_left.
| if (!unshield_reader_open_volume(reader, reader->volume + 1)) | |
| { | |
| unshield_error("bytes_to_read is zero and failed to open next volume %i", | |
| reader->volume + 1); | |
| goto exit; | |
| } | |
| int previous_volume = reader->volume; | |
| uint32_t previous_volume_bytes_left = reader->volume_bytes_left; | |
| int next_volume = reader->volume + 1; | |
| if (!unshield_reader_open_volume(reader, next_volume)) | |
| { | |
| unshield_error("bytes_to_read is zero and failed to open next volume %i", | |
| next_volume); | |
| goto exit; | |
| } | |
| if (reader->volume == previous_volume && | |
| reader->volume_bytes_left == previous_volume_bytes_left) | |
| { | |
| unshield_error("bytes_to_read is zero and opening next volume %i did not advance reader state", | |
| next_volume); | |
| goto exit; | |
| } |
| if ((reader->file_descriptor->flags & FILE_SPLIT) && reader->volume_bytes_left == 0) | ||
| { | ||
| if (!unshield_reader_open_volume(reader, reader->volume + 1)) | ||
| { | ||
| unshield_error("bytes_to_read is zero and failed to open next volume %i", | ||
| reader->volume + 1); | ||
| goto exit; | ||
| } | ||
| continue; |
There was a problem hiding this comment.
This change fixes a boundary condition for split/multi-volume reads, but there doesn’t appear to be automated test coverage for the scenario where a read ends exactly at a volume boundary and the next unshield_reader_read() call must advance to the next volume. Adding a regression test (e.g., a small multi-volume fixture that straddles a boundary) would help prevent this from reoccurring.
|
Sorry for the lack of updates. @Hoernchen are you able to share the installation files and are you able to address the code review? I suspect that the failed test is caused by the loop that Copilot discovered. |
|
No, unfortunately I can't share them. |
When processing a split (multi-volume) compressed file, if a read request completes exactly at a volume boundary (consuming the last byte, volume_bytes_left == 0), the next call to unshield_reader_read() calculates bytes_to_read = MIN(requested, 0) = 0 and immediately errors out.
unshield_file_save_old() handles this properly, but the "new" path does not.