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.

Sign up for GitHub

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

Jump to bottom

622 corrupted zim leads to crash #726

Merged
merged 2 commits into from
Apr 20, 2024
Merged

622 corrupted zim leads to crash #726

merged 2 commits into from
Apr 20, 2024

Conversation

BPerlakiH
Copy link
Collaborator

@BPerlakiH BPerlakiH commented Apr 9, 2024 •
edited by kelson42
Loading

Preparing for handling standard exception raised as part of #622 .

Fixes #622

@BPerlakiH BPerlakiH linked an issue Apr 9, 2024 that may be closed by this pull request
Corrupted ZIM leads to crash #622
Closed
@BPerlakiH BPerlakiH mentioned this pull request Apr 9, 2024
Closed
mgautierfr
mgautierfr reviewed Apr 9, 2024
View reviewed changes
SwiftUI/Model/SearchOperation/SearchOperation.mm Outdated
int count = std::max((35 - (int)[self.results count]) / (int)titleSearchArchives.size(), 5);
[self addTitleSearchResults:titleSearchArchives count:(int)count];
}
try {
Copy link
Member

Choose a reason for hiding this comment

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

You probably don't want to try/catch the whole loop adding the archive but try/catch each adding of an archive.

Else, the first corrupted zim file with stop the loop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, make sense. Please see it updated.

SwiftUI/Model/SearchOperation/SearchOperation.mm Outdated
int count = std::max((35 - (int)[self.results count]) / (int)titleSearchArchives.size(), 5);
[self addTitleSearchResults:titleSearchArchives count:(int)count];
}
} catch (std::exception) {}
Copy link
Member

Choose a reason for hiding this comment

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

Sadly, xapian exception are not inheriting standard exception.
So you should also catch Xapian::Error

Copy link
Contributor

Choose a reason for hiding this comment

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

Once you have fixed libzim, it wil!?

Copy link
Collaborator Author

@BPerlakiH BPerlakiH Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we need to use that, it needs to be exported / exposed. Currently we can only access:

namespace Xapian {
  class Enquire;
  class MSet;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be handled in openzim/libzim#870

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you think we could stick to the convention of throwing only std::exception?
If Xapian::Error could be caught by the libzim, and re-thrown as std::exception that would be the simplest / cleanest solution for client apps on Apple.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes IMHO

Copy link
Member

Choose a reason for hiding this comment

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

Do you think we could stick to the convention of throwing only std::exception?
If Xapian::Error could be caught by the libzim, and re-thrown as std::exception that would be the simplest / cleanest solution for client apps on Apple.

Yes, that's the plan with openzim/libzim#873, we will throw a zim::zim::ZimFileFormatError (which inherit std::exception) or handle the exception internally to returns default value.
When we will do one or the other solution is still open to discussion (and evolution), but it will always be one of the two ways (or a bug in libzim code).

@BPerlakiH BPerlakiH requested a review from mgautierfr April 9, 2024 20:13
@kelson42
Copy link
Contributor

@BPerlakiH Can younplease rebase and fix conflict?

@BPerlakiH
Copy link
Collaborator Author

@kelson42 rebased and conflict solved.

@BPerlakiH BPerlakiH force-pushed the 622-corrupted-zim-leads-to-crash branch from b3f47fa to 7ac2e31 Compare April 14, 2024 15:41
@rgaudin
Copy link
Member

rgaudin commented Apr 17, 2024

libkiwix 13.1.0-1 has been released

@kelson42 kelson42 force-pushed the 622-corrupted-zim-leads-to-crash branch from 7ac2e31 to 0a8e0c7 Compare April 19, 2024 09:08
@kelson42
Copy link
Contributor

@BPerlakiH Can you please check if if works now with the latest version of the libzim/libkiwix?

@BPerlakiH @kelson42
Squashed commit of the following:
8c57208 
commit b3f47fa
Merge: a90d196 b019f69
Author: Balazs Perlaki-Horvath <[email protected]>
Date:   Sun Apr 14 17:35:12 2024 +0200

    Merge branch '622-corrupted-zim-leads-to-crash' of github.com:kiwix/apple into 622-corrupted-zim-leads-to-crash

commit a90d196
Author: Balazs Perlaki-Horvath <[email protected]>
Date:   Tue Apr 9 21:31:22 2024 +0200

    Update try catch

commit b784b27
Author: Balazs Perlaki-Horvath <[email protected]>
Date:   Tue Apr 9 13:19:11 2024 +0200

    Wrap search in standard exception handling

commit b019f69
Merge: d6e8e9a 8541cde
Author: Balazs Perlaki-Horvath <[email protected]>
Date:   Tue Apr 9 23:54:53 2024 +0200

    Merge branch '622-corrupted-zim-leads-to-crash' of github.com:kiwix/apple into 622-corrupted-zim-leads-to-crash

commit d6e8e9a
Author: Balazs Perlaki-Horvath <[email protected]>
Date:   Tue Apr 9 21:31:22 2024 +0200

    Update try catch

commit c132912
Author: Balazs Perlaki-Horvath <[email protected]>
Date:   Tue Apr 9 13:19:11 2024 +0200

    Wrap search in standard exception handling

commit 8541cde
Author: Balazs Perlaki-Horvath <[email protected]>
Date:   Tue Apr 9 13:19:11 2024 +0200

    Wrap search in standard exception handling

commit 57e008e
Merge: 5024548 4459187
Author: Balazs Perlaki-Horvath <[email protected]>
Date:   Tue Apr 9 13:02:15 2024 +0200

    Merge branch '622-corrupted-zim-leads-to-crash' of github.com:kiwix/apple into 622-corrupted-zim-leads-to-crash

commit 4459187
Author: Balazs Perlaki-Horvath <[email protected]>
Date:   Mon Apr 8 08:39:00 2024 +0200

    Fix unlinked ZIM file tab state
@kelson42 kelson42 force-pushed the 622-corrupted-zim-leads-to-crash branch from 0a8e0c7 to 8c57208 Compare April 19, 2024 10:46
@BPerlakiH
Copy link
Collaborator Author

@kelson42 After updating to latest libzim, and applying one more standard exception handler, the search is working with the corrupted wikipedia ZIM file.

@kelson42 kelson42 merged commit 6675ffe into main Apr 20, 2024
4 checks passed
@kelson42 kelson42 deleted the 622-corrupted-zim-leads-to-crash branch April 20, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgaudin rgaudin Awaiting requested review from rgaudin

@kelson42 kelson42 Awaiting requested review from kelson42

@mgautierfr mgautierfr Awaiting requested review from mgautierfr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Corrupted ZIM leads to crash
4 participants
@BPerlakiH @kelson42 @rgaudin @mgautierfr