Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Reader proceeds to next volume when current one runs out of bytes #173

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jaens
Copy link

@jaens jaens commented Sep 13, 2023

Currently, extracting IS5 CABs that split files over multiple volumes seems to fail with bytes_to_read can't be zero, because the current volume runs out of bytes, meaning MIN(bytes_left, reader->volume_bytes_left) becomes 0 which always leads to an error in unshield_reader_read.

Detect this condition and jump to opening the next volume. This is possibly related to #124 and #129, although it might not fix the latter.

@jaens jaens marked this pull request as ready for review September 13, 2023 17:00
@twogood
Copy link
Owner

twogood commented Sep 15, 2023

goto is not something you see everyday! :)

Do you have some .cab files that can be used as test cases?

@twogood twogood self-assigned this Sep 15, 2023
@twogood
Copy link
Owner

twogood commented Sep 15, 2023

I wonder why the tests fail :(

@jaens
Copy link
Author

jaens commented Sep 15, 2023

goto is not something you see everyday! :)

Can be avoided by restructuring all the surrounding code into nested ifs, I suppose...

Do you have some .cab files that can be used as test cases?

uhh... not sure if abandonware can be distributed as a test case?

I wonder why the tests fail :(

I'll check them locally, might need even more extra checks to prevent infinite loops, in case something else runs out I guess.

@twogood
Copy link
Owner

twogood commented Sep 16, 2023

@jaens The existing test cases are pretty much based on abandonware... I need to check that they have not started to fail on main branch as well.

@twogood
Copy link
Owner

twogood commented Sep 17, 2023

All tests succeeded in #174 so I can't take in this PR in its current state.

Copy link
Owner

@twogood twogood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Existing tests must still work
  • Add new test case for this issue (just cut & paste an existing test)

(The goto can be refactored later.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants