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

[Ramiro Polla]

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.

Ramiro