Warn on non-executable ffmpeg #2159
Conversation
In the preferences widget warn the user when they enter a non-executable path to ffmpeg. Thus they don't have to start thumbnailing just to find out that the path is wrong. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>
In the error messages shown when failing to start ffmpeg, instruct the user to set the correct executable in the preferences. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>
| // Try to execute ffmpeg. But wait at most 2 sec for startup and execution | ||
| // so that the UI doesn't hang unnecessarily. | ||
| QProcess ffmpeg; | ||
| ffmpeg.start(s); |
There was a problem hiding this comment.
not sure if someone will bother with exploits, but i'm hoping that we didn't introduce an attack vector by executing a third party binary without the consent of the user. the functionality should be off by default and if the user picks a binary we should only then enable it.
then again, someone might play with adding the value in settings/registry.
ideally, there should be some sort of validation that this is truly ffmpeg, but not exactly easy to do as the user can pick any version.
There was a problem hiding this comment.
Why not just fstat() and check the executable bit? Is there a Windows version of that? I would guess, actually running it is overkill. We might even try to see if it is an actual ffmpeg by asking it to encode something and checksum the result ;-)
There was a problem hiding this comment.
not sure if someone will bother with exploits, but i'm hoping that we didn't introduce an attack vector by executing a third party binary without the consent of the user.
This sounds a bit academic. The executable is only run when the user explicitly edited the field. We could check for changed() to make sure that the user didn't just tab through. This means that the user plans to upload videos, so the ffmpeg executable will be run anyway. And if an attacker managed to install a backdoored ffmpeg binary, they could also have installed a backdoored subsurface binary in the first place.
Why not just fstat() and check the executable bit?
As far as I know, Windows is POSIX, so it should work. Feel free to submit a patch. I'm not convinced that this solves anything, but I also don't mind.
There was a problem hiding this comment.
Why not just fstat() and check the executable bit?
On second thought - perhaps not such a good idea: you would have to scan the PATH in a platform independent way. And do it in such a way that it finds the same executable as QProcess. I'm not sure we want to open that can of worms.
There was a problem hiding this comment.
The executable is only run when the user explicitly edited the field.
if that's the case then this is safer.
As far as I know, Windows is POSIX, so it should work. Feel free to submit a patch. I'm not convinced that this solves anything, but I also don't mind.
Windows has it's own "Windows" standard and is certainly not POSIX.
i don't think we need to do anything else other than only calling the "ffmpeg" binary after the user has enabled (picked it) it in settings.
There was a problem hiding this comment.
I meant just checking the executable bit instead of trying to run it.
There was a problem hiding this comment.
I think that is a valid concern, but also, as @bstoeger points out, an attack vector that doesn't pass the smell test. I wouldn't worry about it.
There was a problem hiding this comment.
Windows has it's own "Windows" standard and is certainly not POSIX.
Ok - good to know.
I meant just checking the executable bit instead of trying to run it.
Yes, I understand. But first you have to find the same binary that QProcess would use, since that is what is used by the thumbnail-generator. I haven't looked at the code of QProcess, but I can imagine that it's not completely trivial.
|
I would say an untranslated string is still better than silently failing. |
|
Given the sum of comments, merging. |
Describe the pull request:
Pull request long description:
This warns when the user sets a non-existing ffmpeg. Moreover, the error-message when ffmpeg fails to execute is extended to instruct the user to set the proper executable in the preferences. This is in response to #1922, though probably not exactly what the reporter wanted.