GNOME Bugzilla – Bug 744049
Opening password protected PDFs can crash evince
Last modified: 2015-10-16 11:29:33 UTC
Created attachment 296202 [details] [review] Check whether load job succeeded When I try to open a password protected PDF file when list of recent files is clean evince crashes. Steps: 1. rm ~/.local/share/recently-used.xbel 2. evince 3. ctrl+o 4. select the file 5. crash I used the file attached in the original downstream report: https://bugzilla.redhat.com/show_bug.cgi?id=1189222. Attached patch fixes the crash for me. It seems that evince tries to access document info of the document even if its load job failed, so I've added a simple check to the load job callback in addition to the other checks.
Review of attachment 296202 [details] [review]: Thanks! ::: shell/ev-recent-view.c @@ +369,3 @@ + if (g_cancellable_is_cancelled (data->cancellable) || + !document || + ev_job_is_failed (EV_JOB (job_load))) { !document was supposed to handle that case, but we forgot we might have a document in case of EV_DOCUMENT_ERROR_ENCRYPTED. I think we can do something better here. We could leave the !document, that covers the cases of load failed expect the password one. Then we check if job failed and error was EV_DOCUMENT_ERROR_ENCRYPTED and in that case we can check if we have the password in the keyring. If we don't have a password, or it fails again with the password, or it's not a EV_DOCUMENT_ERROR_ENCRYPTED but job_is_failed is true, then we just call get_document_info_async_data_free (data); and return early. The code should be similar to what ev-window.c does in ev_window_load_job_cb, but if you don't have time to work on this, feel free to land your patch as it is to fix the crash (you could probably remove the !document in that case, because checking job_is_failed should be enough)
Comment on attachment 296202 [details] [review] Check whether load job succeeded (In reply to Carlos Garcia Campos from comment #1) > Review of attachment 296202 [details] [review] [review]: > > Thanks! > > ::: shell/ev-recent-view.c > @@ +369,3 @@ > + if (g_cancellable_is_cancelled (data->cancellable) || > + !document || > + ev_job_is_failed (EV_JOB (job_load))) { > > !document was supposed to handle that case, but we forgot we might have a > document in case of EV_DOCUMENT_ERROR_ENCRYPTED. I think we can do something > better here. We could leave the !document, that covers the cases of load > failed expect the password one. Then we check if job failed and error was > EV_DOCUMENT_ERROR_ENCRYPTED and in that case we can check if we have the > password in the keyring. If we don't have a password, or it fails again with > the password, or it's not a EV_DOCUMENT_ERROR_ENCRYPTED but job_is_failed is > true, then we just call get_document_info_async_data_free (data); and return > early. The code should be similar to what ev-window.c does in > ev_window_load_job_cb, but if you don't have time to work on this, feel free > to land your patch as it is to fix the crash (you could probably remove the > !document in that case, because checking job_is_failed should be enough) I've removed the check for !document and pushed it to master. Do you want me to push it also to 3.14 ? I have to move to something else now so I can't work on the better solution now.
Created attachment 296582 [details] [review] Check for failed rendering jobs I've got some other reports similar to this one. See https://bugzilla.redhat.com/show_bug.cgi?id=1190174, https://bugzilla.redhat.com/show_bug.cgi?id=1187182 and https://bugzilla.redhat.com/show_bug.cgi?id=1175915. These crashes happens because of a wrong PostScript file. It loads but it doesn't render correctly. Evince then crashes because unhandled checks for failed rendering jobs of sidebar thumbnails, recent view thumbnails and the document itself. There is a good reproducer described here: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=767234. Attached patch fixes these crashes for me. Although evince still shows that the document is still loading (this happens because of draw_one_page() calls ev_view_set_loading (view, TRUE) if "!page_surface" which satisfies also the case when the rendering failed not just when the document is still loading).
(In reply to Marek Kašík from comment #2) > Comment on attachment 296202 [details] [review] [review] > Check whether load job succeeded > > (In reply to Carlos Garcia Campos from comment #1) > > Review of attachment 296202 [details] [review] [review] [review]: > > > > Thanks! > > > > ::: shell/ev-recent-view.c > > @@ +369,3 @@ > > + if (g_cancellable_is_cancelled (data->cancellable) || > > + !document || > > + ev_job_is_failed (EV_JOB (job_load))) { > > > > !document was supposed to handle that case, but we forgot we might have a > > document in case of EV_DOCUMENT_ERROR_ENCRYPTED. I think we can do something > > better here. We could leave the !document, that covers the cases of load > > failed expect the password one. Then we check if job failed and error was > > EV_DOCUMENT_ERROR_ENCRYPTED and in that case we can check if we have the > > password in the keyring. If we don't have a password, or it fails again with > > the password, or it's not a EV_DOCUMENT_ERROR_ENCRYPTED but job_is_failed is > > true, then we just call get_document_info_async_data_free (data); and return > > early. The code should be similar to what ev-window.c does in > > ev_window_load_job_cb, but if you don't have time to work on this, feel free > > to land your patch as it is to fix the crash (you could probably remove the > > !document in that case, because checking job_is_failed should be enough) > > I've removed the check for !document and pushed it to master. Do you want me > to push it also to 3.14 ? Yes, please. > I have to move to something else now so I can't work on the better solution > now. No problem, thanks!
(In reply to Marek Kašík from comment #3) > Created attachment 296582 [details] [review] [review] > Check for failed rendering jobs > > I've got some other reports similar to this one. See > https://bugzilla.redhat.com/show_bug.cgi?id=1190174, > https://bugzilla.redhat.com/show_bug.cgi?id=1187182 and > https://bugzilla.redhat.com/show_bug.cgi?id=1175915. > > These crashes happens because of a wrong PostScript file. It loads but it > doesn't render correctly. Evince then crashes because unhandled checks for > failed rendering jobs of sidebar thumbnails, recent view thumbnails and the > document itself. > > There is a good reproducer described here: > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=767234. > > Attached patch fixes these crashes for me. Although evince still shows that > the document is still loading (this happens because of draw_one_page() calls > ev_view_set_loading (view, TRUE) if "!page_surface" which satisfies also the > case when the rendering failed not just when the document is still loading). This is a different issue, could you please use a new bug report?
(In reply to Carlos Garcia Campos from comment #4) > (In reply to Marek Kašík from comment #2) > > I've removed the check for !document and pushed it to master. Do you want me > > to push it also to 3.14 ? > > Yes, please. Done (In reply to Carlos Garcia Campos from comment #5) > This is a different issue, could you please use a new bug report? I've filed it here: https://bugzilla.gnome.org/show_bug.cgi?id=744586
Review of attachment 296582 [details] [review]: ::: libview/ev-jobs.c @@ +645,3 @@ + ev_job_failed (job, + EV_DOCUMENT_ERROR, + EV_DOCUMENT_ERROR_INVALID, I don't think EV_DOCUMENT_ERROR_INVALID is the right one for rendering errors, but we are already using that error in other cases that don't fit either, and we are just checking if the jobs failed or not.