Use _exit instead of exit not to run atexit handlers in case execvp() fails.
BUG: 420074
FIXED-IN: 20.04
broulik | |
ngraham | |
pino |
Frameworks |
Use _exit instead of exit not to run atexit handlers in case execvp() fails.
BUG: 420074
FIXED-IN: 20.04
Previewed a djvu file without ddjvu installed and having no crash.
No Linters Available |
No Unit Test Coverage |
Buildable 27005 | |
Build 27023: arc lint + arc unit |
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:
[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
[2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/_Exit.html
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.
please target release/20.04 for this fix, thanks
thumbnail/djvucreator.cpp | ||
---|---|---|
52–54 | extra brackets |
thumbnail/djvucreator.cpp | ||
---|---|---|
52–54 | I'd like to use instead the Framework coding style to improve homogenizing coding styles. |
thumbnail/djvucreator.cpp | ||
---|---|---|
52–54 | this code does not follow that style, so please keep new changes coherent with the existing style |
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".
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:
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.