On Fri, May 16, 2025 at 12:43 AM softworkz .
<softworkz-at-hotmail....@ffmpeg.org> wrote:
>
>
>
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of Mark
> > Thompson
> > Sent: Freitag, 16. Mai 2025 00:35
> > To: ffmpeg-devel@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-boun...@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....@ffmpeg.org> wrote:
> > >>>> On Thu, May 15, 2025 at 11:11 PM softworkz <g...@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?
What about the user parsing the output from the cli, looking for a
specific string (such as "graph file saved to [...]"), and opening
that?
Another point I'd like to add is that for platform-specific code like
this, in the long term, we get a bunch of patches to fix changes for
Microsoft's or Apple's new way of doing things, or someone wants to
add BeOS or TempleOS support, and they're rarely properly maintained.
It's better not to set that precedent.
Ramiro
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
