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

Correctly test invalid offsets in clusters. #895

Merged
merged 3 commits into from
Jun 22, 2024
Merged

Conversation

mgautierfr
Copy link
Collaborator

@mgautierfr mgautierfr commented Jun 10, 2024

This offset test in the cluster is to check that value parsed are consistent.
This is not to catch a potential bug in our code.

So we must throw a ZimFileFormatError in this case.

Extend the validate function to test this kind of corrupted offset.

Need openzim/zim-testing-suite#9
Fix #893

test/archive.cpp Show resolved Hide resolved
src/fileimpl.cpp Outdated Show resolved Hide resolved
@kelson42
Copy link
Contributor

@mgautierfr Both macOS and Windows CI don't pass.

@mgautierfr
Copy link
Collaborator Author

I know.
It is because we need a new version of zim-testing-suite (which is blocked by openzim/zim-testing-suite#10) and kiwix/kiwix-build#706

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

LGTM. Can be merged once rebased & fixed-up.

@kelson42
Copy link
Contributor

@mgautierfr What is the status here? Testing auite has been released, but the CI still fails... and acutally I wanted to release 9.2.2 this WE.

This test is to check that value parsed are consistent.
This is not to catch a potential bug in our code.

So we must throw a ZimFileFormatError in this case.
Not that the test is correctly catching invalid offset only for nons variant.
Current `validate` method is not checking the offsets in the cluster.
Nons variant detect the invalid offset as `titlePtrList` is stored in the cluster
and so we try to open it (using `X/listing/titleOrdered/vX`)

Correct test will be added in next commit.

Need PR openzim/zim-testing-suite#9
@kelson42
Copy link
Contributor

I have:

  • Made a PR to used version 0.6.0 of the ZIM testing suite
  • Rebase the git rev log.

I will merge

@kelson42 kelson42 merged commit 0bd084a into main Jun 22, 2024
34 checks passed
@kelson42 kelson42 deleted the fix_crash_invalid_zim branch June 22, 2024 11:53
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.

New crash scenario of the libzim with corrupted ZIM file
3 participants