Thumbnail djvu: Avoid a crash when djvu is not installed
Needs ReviewPublic

Authored by meven on Sun, May 17, 6:12 AM.

Details

Reviewers
broulik
ngraham
pino
Group Reviewers
Frameworks
Summary

Use _exit instead of exit not to run atexit handlers in case execvp() fails.

BUG: 420074
FIXED-IN: 20.04

Test Plan

Previewed a djvu file without ddjvu installed and having no crash.

Diff Detail

Repository
R320 KIO Extras
Branch
arcpatch-D29805
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27088
Build 27106: arc lint + arc unit
meven created this revision.Sun, May 17, 6:12 AM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptSun, May 17, 6:12 AM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Sun, May 17, 6:12 AM
pino requested changes to this revision.Sun, May 17, 7:05 AM
pino added a subscriber: pino.

This change is definitely not correct.

A child process exited with 1 as result but it wasn't an error case.

it is an error case: the execv*() family [1] of functions replaces the execution with the requested process, and never return in a successful execution. "successful execution" means that the process was started correctly, no matter its resulting exit code. Anything after execv*() will be processed only if it fails, and one of the most common issues is that the requested executable does not exists.

In addition to this, your case merely changes the exit() code, still calling exit() (which appears in the backtraces of the two bugs, and most probably one can be marked as duplicate of the other). The issue here is that fork() clones the calling process (in this case kdeinit), including the atexit handlers: apparently a mesa atexit handler is run... The real fix in this case is to use _exit() [2], which is a more direct way to exit the current process without running the atexit handlers.

Furthermore, this code looks like a C snippet to invoke a process and reading its stdout retrofitted as ThumbCreator plugin... sadly C is not an easy language, and the process APIs are complex and very prone to mistakes. The ideal solution would be to invoke ddjvu using QProcess, and read its output as it comes; it would also have the (small) advantage of making this thumbnailer plugin available on any platform, Windows included (in case anyone cases, that is).

Let me sum up what I think is the approach to make this ThumbCreator work properly:

  • to fix the issue in the stable branch:
    • switch exit() wth _exit(), using the same exit code (otherwise ok won't be set as false after the waitpid() call in the parent)
    • before the pipe creation, check for the ddjvu executable existance using QStandardPath::findExecutable, returning false quickly
  • to fix the issue in the long term:
    • rewrite this to call ddjvu using QProcess

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
[2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/_Exit.html

This revision now requires changes to proceed.Sun, May 17, 7:05 AM
meven planned changes to this revision.Sun, May 17, 7:41 AM

Thanks @pino

meven updated this revision to Diff 83014.Sun, May 17, 7:44 AM

Review feedback

meven edited the summary of this revision. (Show Details)Sun, May 17, 7:45 AM
meven edited the test plan for this revision. (Show Details)Sun, May 17, 7:48 AM
pino requested changes to this revision.Sun, May 17, 7:51 AM

Please update the text of the revision and the commit message, as they don't make much sense. Please mention that it fixes a crash in case ddjvu is not installed, by avoiding executing it, and using _exit() to not run atexit handlers in case execvp() fails.

Also, please respect the indentation of the code: 2 spaces, and no brackets for a single instruction in a block.

This revision now requires changes to proceed.Sun, May 17, 7:51 AM
meven retitled this revision from Thumbnail djvu: avoid to exit(1) when it should not to Thumbnail djvu: Avoid a crash when djvu is not installed.Sun, May 17, 3:55 PM
meven edited the summary of this revision. (Show Details)
meven updated this revision to Diff 83018.Sun, May 17, 3:56 PM

fix indentation

pino added a comment.Sat, May 30, 8:16 AM

please target release/20.04 for this fix, thanks

thumbnail/djvucreator.cpp
52–54

extra brackets

meven marked an inline comment as done.Sat, May 30, 10:19 AM
meven added inline comments.
thumbnail/djvucreator.cpp
52–54

I'd like to use instead the Framework coding style to improve homogenizing coding styles.
https://community.kde.org/Policies/Frameworks_Coding_Style#Braces

pino added inline comments.Sat, May 30, 10:27 AM
thumbnail/djvucreator.cpp
52–54

this code does not follow that style, so please keep new changes coherent with the existing style

meven updated this revision to Diff 83177.Sat, May 30, 11:16 AM
meven marked an inline comment as done.

Remove brackets

meven marked 2 inline comments as done.Sat, May 30, 11:16 AM
pino added a comment.Sat, May 30, 1:13 PM

FIXED-IN: 20.08

still for 20.08...

meven added a comment.Sat, May 30, 1:33 PM
In D29805#674204, @pino wrote:

FIXED-IN: 20.08

still for 20.08...

Yes kio-extra is released with KDE Applications

pino added a comment.Sat, May 30, 1:39 PM
In D29805#674204, @pino wrote:

FIXED-IN: 20.08

still for 20.08...

Yes kio-extra is released with KDE Applications

Yes, I know. Asked to land this fix instead to release/20.04, and thus change the commit message accordingly.

meven added a comment.Sat, May 30, 3:53 PM
In D29805#674206, @pino wrote:
In D29805#674204, @pino wrote:

FIXED-IN: 20.08

still for 20.08...

Yes kio-extra is released with KDE Applications

Yes, I know. Asked to land this fix instead to release/20.04, and thus change the commit message accordingly.

Please be explicit when you comment, no one could deduce what you meant.

If this is ready approve and add a comment "land to 20.04".

pino added a comment.Sat, May 30, 3:58 PM
In D29805#674206, @pino wrote:
In D29805#674204, @pino wrote:

FIXED-IN: 20.08

still for 20.08...

Yes kio-extra is released with KDE Applications

Yes, I know. Asked to land this fix instead to release/20.04, and thus change the commit message accordingly.

Please be explicit when you comment, no one could deduce what you meant.

I wrote it two times to land this on the stable branch: the first time when I explained why the initial patch was not correct and what the problem actually was (with hints on short term and long term fixes), and earlier today when I wrote:

In D29805#674185, @pino wrote:

please target release/20.04 for this fix, thanks

There is no need to "deduce" anything, just read what I wrote, thanks.

If this is ready approve and add a comment "land to 20.04".

The commit message still says "20.08", so not yet.

meven added a comment.Sat, May 30, 4:29 PM
In D29805#674225, @pino wrote:
In D29805#674206, @pino wrote:
In D29805#674204, @pino wrote:

FIXED-IN: 20.08

still for 20.08...

Yes kio-extra is released with KDE Applications

Yes, I know. Asked to land this fix instead to release/20.04, and thus change the commit message accordingly.

Please be explicit when you comment, no one could deduce what you meant.

I wrote it two times to land this on the stable branch: the first time when I explained why the initial patch was not correct and what the problem actually was (with hints on short term and long term fixes), and earlier today when I wrote:

In D29805#674185, @pino wrote:

please target release/20.04 for this fix, thanks

There is no need to "deduce" anything, just read what I wrote, thanks.

If this is ready approve and add a comment "land to 20.04".

The commit message still says "20.08", so not yet.

That's a detail once it is accepted it is trivial to edit...
Usually this is not required.

meven edited the summary of this revision. (Show Details)Sat, May 30, 4:29 PM