GNOME Bugzilla – Bug 784963
password-protected cbz file hangs evince
Last modified: 2017-07-23 08:35:05 UTC
Created attachment 355662 [details] hang.cbz When creating a password protected zip archive and renaming it to .cbz then this will cause a hang in evince. The unzip process will even continue to run (and cause high load - I don't know why...) after exiting evince. The reason is obvious: evince calls the unzip command line tool and doesn't expect any interaction from it.
Thanks for reporting this. Which Evince version is this about?
(In reply to André Klapper from comment #1) > Thanks for reporting this. Which Evince version is this about? Something old, because we now use libarchive (not that it would support password protected files any better, but it could).
(In reply to Bastien Nocera from comment #2) > (In reply to André Klapper from comment #1) > > Thanks for reporting this. Which Evince version is this about? > > Something old, because we now use libarchive (not that it would support > password protected files any better, but it could). Well, "old" as in "the current release", 3.24.0, which still uses command line tools. I am not sure if password protected cbz files are a thing, so I am not asking to support them. What I'm asking for is not to hang evince and cause CPU load if it sees such a file. I think going to libarchive is a good move, as it probably prevents all kinds of commandline interaction issues.
It throws warnings with master: (evince:32307): EvinceView-CRITICAL **: ev_page_cache_is_page_cached: assertion 'EV_IS_PAGE_CACHE (cache)' failed
+ Trace 237656
I guess that some pieces of code don't check whether opening the file succeeded. The error reporting could also use some help.
What's the password for this test case? unzip can detect that a password is needed to access the data, but libarchive doesn't. It might be thinking that because the file is 0 bytes, it doesn't need to decrypt it ;)
Sorry, I forgot the password. I'll attach two new files: One cbz with a password and real PNG images inside, one with zero-sized .png-named files inside. Password in both cases is "linux".
Created attachment 355764 [details] password-and-real-images.cbz
Created attachment 355765 [details] password-and-zero-size-files.cbz
(In reply to Hanno Boeck from comment #6) > Sorry, I forgot the password. I'll attach two new files: One cbz with a > password and real PNG images inside, one with zero-sized .png-named files > inside. > > Password in both cases is "linux". Thanks. So, yes, libarchive optimises the "zero-length" case.
Created attachment 355831 [details] [review] comics: Add test program for archive handling code To make it easier to find bugs related to archive handling specifically. This test program just lists the files in the archive provided, along with their sizes and whether they are password protected.
Created attachment 355832 [details] [review] comics: Add API to detect whether an archive entry is encrypted Only implemented and used by the libarchive backend unfortunately.
Created attachment 355833 [details] [review] comics: Add error reporting to comics_document_list() So that we can know whether a NULL return value is due to an empty file or a broken file.
Created attachment 355834 [details] [review] comics: Return an error when archive contents are encrypted
Created attachment 355835 [details] [review] comics: Use error returned by comics_document_list()
Created attachment 355836 [details] [review] comics: Throw an error if the mime-type cannot be read
Created attachment 355841 [details] [review] comics: Split off checking whether a file has a supported extension
Created attachment 355842 [details] [review] comics: Check for supported extensions in initial list loop Instead of looping over all the files once, making a list, then going over the list once again to check whether those files are supported, check for support when building the list. This fixes reporting an error when non-image files are encrypted and we could read the images just fine, or the separate errors for when the archive is empty, or there are no supported files in the archive.
That's a load of patches. Note that evince will still say that "zip files" aren't supported instead of the error that the backend sends back.
Created attachment 355852 [details] [review] comics: Return an error when archive contents are encrypted Note that this will request a password and crash when it cannot pass that information to the EvDocument later. This needs to be fixed in the evince shell itself.
Created attachment 355853 [details] [review] shell: Fix typo in comment
Created attachment 355854 [details] [review] libdocument: Remove unused has_document_security() function
Created attachment 355855 [details] [review] shell: Fix crash when comics says it's encrypted Returning an EV_DOCUMENT_ERROR_ENCRYPTED error to the shell should only trigger a password prompt if the document supports the EvDocumentSecurity to pass the password to the loader.
(In reply to Bastien Nocera from comment #18) > That's a load of patches. Note that evince will still say that "zip files" > aren't supported instead of the error that the backend sends back. That's because I was returning the wrong error code. But when returning the right error code, it would ask for a password we couldn't do anything with, and crash after that. That's what that last patch is about. And all that for an edge case :/
Comment on attachment 355854 [details] [review] libdocument: Remove unused has_document_security() function Moved that to another bug, it's not needed for this one, and was incomplete.
Review of attachment 355833 [details] [review]: ::: backend/comics/comics-document.c @@ +75,3 @@ + EV_DOCUMENT_ERROR, + EV_DOCUMENT_ERROR_INVALID, + _("File is corrupted")); Why don't we just propagate the error message from EvArchive? that's what we do in the PDf backend, for example. I guess ev_archive_open_filename() could fail for other reasons. @@ +94,3 @@ + EV_DOCUMENT_ERROR, + EV_DOCUMENT_ERROR_INVALID, + _("File is corrupted")); Ditto. @@ +218,2 @@ /* Get list of files in archive */ + cb_files = comics_document_list (comics_document, NULL); Why aren't you passing an error here?
Review of attachment 355835 [details] [review]: ::: backend/comics/comics-document.c @@ +232,3 @@ /* Get list of files in archive */ + cb_files = comics_document_list (comics_document, error); + if (!cb_files) I see, this should be squashed in previous commit.
Review of attachment 355836 [details] [review]: ::: backend/comics/comics-document.c @@ +222,2 @@ mime_type = ev_file_get_mime_type (uri, FALSE, &err); + if (mime_type == NULL) { I know we were already doing this, but I wonder if this can really happen. If we are here is because the document factory found a mime type for the uri and created a document for it. Maybe we can just add an assert here. @@ +229,3 @@ + EV_DOCUMENT_ERROR, + EV_DOCUMENT_ERROR_INVALID, + _("File is not accessible")); But if not, let just propagate the error message.
Review of attachment 355841 [details] [review]: Ok
Review of attachment 355842 [details] [review]: Great! ::: backend/comics/comics-document.c @@ -272,3 @@ - if (has_supported_extension (cb_file, supported_extensions)) { - g_ptr_array_add (comics_document->page_names, - g_strstrip (g_strdup (cb_file))); I guess we should do the strip in comics_document_list now.
Review of attachment 355852 [details] [review]: ok
Review of attachment 355853 [details] [review]: No need to submit a patch for this :-P
Review of attachment 355855 [details] [review]: Sure
Review of attachment 355833 [details] [review]: ::: backend/comics/comics-document.c @@ +75,3 @@ + EV_DOCUMENT_ERROR, + EV_DOCUMENT_ERROR_INVALID, + _("File is corrupted")); Because the error message returned by EvArchive are only useful for debug. Running evince with debug messages would allow to gather that if there's ever another reason for the archive not to be opened. @@ +218,2 @@ /* Get list of files in archive */ + cb_files = comics_document_list (comics_document, NULL); It's in a separate patch, I'll merge.
Review of attachment 355836 [details] [review]: I wouldn't assert because that's ugly, but I made it propagate the error.
Review of attachment 355842 [details] [review]: ::: backend/comics/comics-document.c @@ -272,3 @@ - if (has_supported_extension (cb_file, supported_extensions)) { - g_ptr_array_add (comics_document->page_names, - g_strstrip (g_strdup (cb_file))); I think we shouldn't be stripping since we use libarchive. We're not parsing command-line outputs anymore, and we can handle leading and trailing spaces just fine :)
Created attachment 356165 [details] [review] comics: Add test program for archive handling code To make it easier to find bugs related to archive handling specifically. This test program just lists the files in the archive provided, along with their sizes and whether they are password protected.
Created attachment 356166 [details] [review] comics: Don't strip filenames in archive This was useful when parsing command-line outputs but makes no sense since we switched to using libarchive/unarr, and would actually break support for filenames in the archive that have leading or trailing spaces.
Created attachment 356167 [details] [review] comics: Add API to detect whether an archive entry is encrypted Only implemented and used by the libarchive backend unfortunately.
Created attachment 356168 [details] [review] comics: Add error reporting to comics_document_list() So that we can know whether a NULL return value is due to an empty file or a broken file.
Created attachment 356169 [details] [review] comics: Return an error when archive contents are encrypted Note that this will request a password and crash when it cannot pass that information to the EvDocument later. This needs to be fixed in the evince shell itself.
Created attachment 356170 [details] [review] comics: Propagate an error if the mime-type cannot be read And avoid leaking an unused error.
Created attachment 356171 [details] [review] comics: Split off checking whether a file has a supported extension
Created attachment 356172 [details] [review] comics: Check for supported extensions in initial list loop Instead of looping over all the files once, making a list, then going over the list once again to check whether those files are supported, check for support when building the list. This fixes reporting an error when non-image files are encrypted and we could read the images just fine, or the separate errors for when the archive is empty, or there are no supported files in the archive.
Created attachment 356173 [details] [review] shell: Fix typo in comment
Created attachment 356174 [details] [review] shell: Fix crash when comics says it's encrypted Returning an EV_DOCUMENT_ERROR_ENCRYPTED error to the shell should only trigger a password prompt if the document supports the EvDocumentSecurity to pass the password to the loader.
Attachment 356165 [details] pushed as 5e92c9a - comics: Add test program for archive handling code Attachment 356166 [details] pushed as 9a407f1 - comics: Don't strip filenames in archive Attachment 356167 [details] pushed as 4cc667c - comics: Add API to detect whether an archive entry is encrypted Attachment 356168 [details] pushed as 2efa131 - comics: Add error reporting to comics_document_list() Attachment 356169 [details] pushed as 29747f1 - comics: Return an error when archive contents are encrypted Attachment 356170 [details] pushed as 14c54a7 - comics: Propagate an error if the mime-type cannot be read Attachment 356171 [details] pushed as 06c17eb - comics: Split off checking whether a file has a supported extension Attachment 356172 [details] pushed as 2de9104 - comics: Check for supported extensions in initial list loop Attachment 356173 [details] pushed as bfeafcc - shell: Fix typo in comment Attachment 356174 [details] pushed as 1703978 - shell: Fix crash when comics says it's encrypted
(In reply to Bastien Nocera from comment #35) > Review of attachment 355842 [details] [review] [review]: > > ::: backend/comics/comics-document.c > @@ -272,3 @@ > - if (has_supported_extension (cb_file, supported_extensions)) { > - g_ptr_array_add (comics_document->page_names, > - g_strstrip (g_strdup (cb_file))); > > I think we shouldn't be stripping since we use libarchive. We're not parsing > command-line outputs anymore, and we can handle leading and trailing spaces > just fine :) Good point!