[FFmpeg-devel] [FFmpeg-cvslog] fftools/graphprint: Now, make it a Killer-Feature!

[Mark Thompson]

On 15/05/2025 23:19, softworkz . wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Ramiro Polla
>> Sent: Freitag, 16. Mai 2025 00:13
>> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [FFmpeg-cvslog] fftools/graphprint: Now, make it a
>> Killer-Feature!
>>
>> On Fri, May 16, 2025 at 12:00 AM softworkz .
>> <softworkz-at-hotmail.com at ffmpeg.org> wrote:
>>>> On Thu, May 15, 2025 at 11:11 PM softworkz <git at videolan.org> wrote:
>>>> [...]
>>>>> diff --git a/fftools/graph/filelauncher.c b/fftools/graph/filelauncher.c
>>>>> new file mode 100644
>>>>> index 0000000000..45514ca599
>>>>> --- /dev/null
>>>>> +++ b/fftools/graph/filelauncher.c
>>>> [...]
>>>>> +int ff_open_html_in_browser(const char *html_path)
>>>>> +{
>>>>> +    if (!html_path || !*html_path)
>>>>> +        return -1;
>>>>> +
>>>>> +#if defined(_WIN32)
>>>>> +
>>>>> +    // --- Windows ---------------------------------
>>>>> +    {
>>>>> +        HINSTANCE rc = ShellExecuteA(NULL, "open", html_path, NULL,
>> NULL,
>>>> SW_SHOWNORMAL);
>>>>> +        if ((UINT_PTR)rc <= 32) {
>>>>> +            // Fallback: system("start ...")
>>>>> +            char cmd[1024];
>>>>> +            _snprintf_s(cmd, sizeof(cmd), _TRUNCATE, "start \"\"
>> \"%s\"",
>>>> html_path);
>>>>> +            if (system(cmd) != 0)
>>>>> +                return -1;
>>>>> +        }
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>> +#elif defined(__APPLE__)
>>>>> +
>>>>> +    // --- macOS -----------------------------------
>>>>> +    {
>>>>> +        // "open" is the macOS command to open a file/URL with the
>> default
>>>> application
>>>>> +        char cmd[1024];
>>>>> +        snprintf(cmd, sizeof(cmd), "open '%s' 1>/dev/null 2>&1 &",
>>>> html_path);
>>>>> +        if (system(cmd) != 0)
>>>>> +            return -1;
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>> +#else
>>>>> +
>>>>> +    // --- Linux / Unix-like -----------------------
>>>>> +    // We'll try xdg-open, then gnome-open, then kfmclient
>>>>> +    {
>>>>> +        // Helper macro to try one browser command
>>>>> +        // Returns 0 on success, -1 on failure
>>>>> +        #define TRY_CMD(prog) do {                                   \
>>>>> +            char buf[1024];                                          \
>>>>> +            snprintf(buf, sizeof(buf), "%s '%s' 1>/dev/null 2>&1 &", \
>>>>> +                     (prog), html_path);                              \
>>>>> +            int ret = system(buf);                                    \
>>>>> +            /* On Unix: system() returns -1 if the shell can't run. */\
>>>>> +            /* Otherwise, check exit code in lower 8 bits.
>> */\
>>>>> +            if (ret != -1 && WIFEXITED(ret) && WEXITSTATUS(ret) == 0) \
>>>>> +                return 0;                                             \
>>>>> +        } while (0)
>>>>> +
>>>>> +        TRY_CMD("xdg-open");
>>>>> +        TRY_CMD("gnome-open");
>>>>> +        TRY_CMD("kfmclient exec");
>>>>> +
>>>>> +        fprintf(stderr, "Could not open '%s' in a browser.\n",
>> html_path);
>>>>> +        return -1;
>>>>> +    }
>>>>> +
>>>>> +#endif
>>>>> +}
>>>> [...]
>>>>
>>>> Sorry I didn't have a closer look at the patchset while it was under
>>>> review, but system(cmd) is a big no-no. We could create a file with an
>>>> explicit path passed by the user, but then it's up to the user to open
>>>> it.
>>>
>>> What's bad about opening a file in the browser when that's the documented
>>> behavior of the cli parameter?
>>
>> Straight out of ChatGPT:
>> I understand the motivation — making the feature more user-friendly by
>> launching the result directly is a nice touch. The concern isn't with
>> the feature itself, but rather with the way it's implemented.
>> Using system() to launch a browser introduces potential security
>> risks, especially if the file path is ever constructed from untrusted
>> input (e.g. future scripting, API wrapping, or unexpected shell
>> expansion). It's generally discouraged in projects like FFmpeg, where
>> robustness and security are critical.
> 
> Hi,
> 
> of course I understand that. 
> But it isn't constructed from untrusted input.
> 
> Best regards
> sw

$ export TMPDIR="'; rm -rf / ;'\\\\"
$ ./ffmpeg_g -sg -i /dev/null -f null -

Calls to system are just not a good idea in general.  Suggest printing the file name and let the user open the file however they choose to.