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

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Freitag, 16. Mai 2025 00:35
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [FFmpeg-cvslog] fftools/graphprint: Now, make it a
> Killer-Feature!
> 
> 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.

How about some middle ground like where the user needs to confirm with 
another keypress?

Or maybe something where the user needs to setup a script or an environment
variable to confirm that the automatic opening is performed?

Or when a user uses the option for the first time, show a prompt whether 
they are sure they want this to be auto-opened?

Thanks
sw