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

["softworkz ." <softworkz-at-hotmail.com@ffmpeg.org>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of softworkz .
> Sent: Freitag, 16. Mai 2025 00:19
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [FFmpeg-cvslog] fftools/graphprint: Now, make it a
> Killer-Feature!
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Ramiro
> Polla
> > Sent: Freitag, 16. Mai 2025 00:13
> > To: FFmpeg development discussions and patches <ffmpeg-devel@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@ffmpeg.org> wrote:
> > > > On Thu, May 15, 2025 at 11:11 PM softworkz <git@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
> 
> _______________________________________________

So, in case you have just seen those few lines and not looked at the whole
patchset: 

This is creating filtergraph visualizations like this:
https://softworkz.github.io/ffmpeg_output_apis/2_hwa_qsv.html

The html and css are trusted because they are included included as
compressed resources in the binary and the rest is built dynamically
in code from the filtergraph objects at runtime, so this can all be
considered as trusted. And only this very specific shown launched
for viewing in a browser.

I totally agree that this should never be done for arbitrary html
content that is not under our control.

Best
softworkz






_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".