Message ID | 8c6c11a6-bc55-41a0-9f98-262c60f63ec8@gmx.net |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,v2] libavformat/vapoursynth: Update to API version 4, load library at runtime | expand |
Context | Check | Description |
---|---|---|
yinshiyou/make_loongarch64 | success | Make finished |
yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
Hello, I adressed the issues/concerns that were raised with the first revision of the patch. Any feedback? Did I do something wrong? Best regards Stefan Am 06.07.24 um 23:08 schrieb Stefan Oltmanns via ffmpeg-devel: > Hello, > > this is revised patch, to sum up the changes: > > The current VapourSynth implementation is rarely used, as it links the > VapourSynth library at build time, making the resulting build unable to > run when VapourSynth is not installed. Therefore barely anyone compiles > with VapourSynth activated. > > I changed it, so that it loads the library at runtime when a VapourSynth > script should be opened, just like AviSynth does. > On Windows the DLL from VapourSynth is not installed in the system > directory, but the location is stored in the Registry. Therefore I added > some code to read that information from the registry. > > As the V4 API is designed with dynamic loading in mind (only a single > import), I updated the implementation to V4 (changes are mostly > superficial, no structural changes). The V4 API is already several years > old, fully supported since R55 released in 2021. > > > Changes from first patch: > -Separated the Windows-specific function for getting the DLL location > from the platform-specific includes > -It is not enabled by default in configure > -The header files are not included anymore > > > I would like to include the header files for this reason: > While most Linux distributions ship ffmpeg, only very few contain > VapourSynth. Therefore ffmpeg won't be compiled with VapourSynth support > by these distributions, because no VapourSynth headers. Including the > headers in ffmpeg would mean they can compile with VapourSynth support > and if a user decided to install VapourSynth from somewhere else or > compile it by himself, ffmpeg support would be ready and no need for the > user to install another ffmpeg or compile one. > I'm not sure what the rules for shipping include files are, as there are > a few 3rd-party include files in ffmpeg. License is not an issue > (Vapourynth is LGPL v2.1 or later like ffmpeg). > > > > make fate runs without any issue. I tested VapourSynth input scripts > with various color formats on different platforms: > > Ubuntu 22.04 > macOS 13 (x86_64) > macOS 13 (arm64) > Windows 10 (msys2/gcc) > > It compiles on these platforms without any warning and runs without any > issues. > > Best regards > Stefan > > _______________________________________________ > 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".
Hi Stefan, On 7/6/24 23:08, Stefan Oltmanns via ffmpeg-devel wrote: > this is revised patch, to sum up the changes: > > The current VapourSynth implementation is rarely used, as it links the > VapourSynth library at build time, making the resulting build unable to > run when VapourSynth is not installed. Therefore barely anyone compiles > with VapourSynth activated. > > I changed it, so that it loads the library at runtime when a VapourSynth > script should be opened, just like AviSynth does. > On Windows the DLL from VapourSynth is not installed in the system > directory, but the location is stored in the Registry. Therefore I added > some code to read that information from the registry. > > As the V4 API is designed with dynamic loading in mind (only a single > import), I updated the implementation to V4 (changes are mostly > superficial, no structural changes). The V4 API is already several years > old, fully supported since R55 released in 2021. > > > Changes from first patch: > -Separated the Windows-specific function for getting the DLL location > from the platform-specific includes > -It is not enabled by default in configure > -The header files are not included anymore > > > I would like to include the header files for this reason: > While most Linux distributions ship ffmpeg, only very few contain > VapourSynth. Therefore ffmpeg won't be compiled with VapourSynth support > by these distributions, because no VapourSynth headers. Including the > headers in ffmpeg would mean they can compile with VapourSynth support > and if a user decided to install VapourSynth from somewhere else or > compile it by himself, ffmpeg support would be ready and no need for the > user to install another ffmpeg or compile one. > I'm not sure what the rules for shipping include files are, as there are > a few 3rd-party include files in ffmpeg. License is not an issue > (Vapourynth is LGPL v2.1 or later like ffmpeg). > > > > make fate runs without any issue. I tested VapourSynth input scripts > with various color formats on different platforms: > > Ubuntu 22.04 > macOS 13 (x86_64) > macOS 13 (arm64) > Windows 10 (msys2/gcc) > > It compiles on these platforms without any warning and runs without any > issues. > From 759b097865953ee66949ecbcdadbebfad623c29a Mon Sep 17 00:00:00 2001 > From: Stefan Oltmanns <stefan-oltmanns@gmx.net> > Date: Sat, 6 Jul 2024 22:56:53 +0200 > Subject: [PATCH] avformat/vapoursynth: Update to API version 4, load library > at runtime > > Signed-off-by: Stefan Oltmanns <stefan-oltmanns@gmx.net> > --- > configure | 3 +- > libavformat/vapoursynth.c | 171 +++++++++++++++++++++++++++++--------- > 2 files changed, 136 insertions(+), 38 deletions(-) > > diff --git a/configure b/configure > index b28221f258..e43f3827ec 100755 > --- a/configure > +++ b/configure > @@ -3575,6 +3575,7 @@ libxevd_decoder_deps="libxevd" > libxeve_encoder_deps="libxeve" > libxvid_encoder_deps="libxvid" > libzvbi_teletext_decoder_deps="libzvbi" > +vapoursynth_deps_any="libdl LoadLibrary" > vapoursynth_demuxer_deps="vapoursynth" > videotoolbox_suggest="coreservices" > videotoolbox_deps="corefoundation coremedia corevideo" > @@ -7080,7 +7081,7 @@ enabled rkmpp && { require_pkg_config rkmpp rockchip_mpp rockchip/r > { enabled libdrm || > die "ERROR: rkmpp requires --enable-libdrm"; } > } > -enabled vapoursynth && require_pkg_config vapoursynth "vapoursynth-script >= 42" VSScript.h vsscript_init > +enabled vapoursynth && require_headers "vapoursynth/VSScript4.h vapoursynth/VapourSynth4.h" > > > if enabled gcrypt; then > diff --git a/libavformat/vapoursynth.c b/libavformat/vapoursynth.c > index 8a2519e19a..9c82f8f3b8 100644 > --- a/libavformat/vapoursynth.c > +++ b/libavformat/vapoursynth.c > @@ -25,9 +25,6 @@ > > #include <limits.h> > > -#include <VapourSynth.h> > -#include <VSScript.h> > - > #include "libavutil/avassert.h" > #include "libavutil/avstring.h" > #include "libavutil/eval.h" > @@ -40,19 +37,46 @@ > #include "demux.h" > #include "internal.h" > > +/* Platform-specific directives. */ > +#ifdef _WIN32 > + #include <windows.h> > + #include "compat/w32dlfcn.h" > + #include "libavutil/wchar_filename.h" > + #undef EXTERN_C > + #define VSSCRIPT_LIB "VSScript.dll" > + #define VS_DLOPEN() ({ void *handle = NULL; \ > + char *dll_name = get_vs_script_dll_name(); \ > + handle = dlopen(dll_name, RTLD_NOW | RTLD_GLOBAL); \ > + av_free(dll_name); \ > + handle; }) > +#else > + #include <dlfcn.h> > + #define VSSCRIPT_NAME "libvapoursynth-script" > + #define VSSCRIPT_LIB VSSCRIPT_NAME SLIBSUF > + #define VS_DLOPEN() dlopen(VSSCRIPT_LIB, RTLD_NOW | RTLD_GLOBAL) > +#endif > + > +#include <vapoursynth/VSScript4.h> > + > struct VSState { > VSScript *vss; > }; > > +typedef const VSSCRIPTAPI *(*VSScriptGetAPIFunc)(int version); > + > +typedef struct VSScriptLibrary { > + void *library; > + const VSSCRIPTAPI *vssapi; > +} VSScriptLibrary; > + > typedef struct VSContext { > const AVClass *class; > > AVBufferRef *vss_state; > > const VSAPI *vsapi; > - VSCore *vscore; > > - VSNodeRef *outnode; > + VSNode *outnode; > int is_cfr; > int current_frame; > > @@ -70,19 +94,72 @@ static const AVOption options[] = { > {NULL} > }; > > +static VSScriptLibrary vs_script_library; Does vs_script_library have to be a global? > + > +static int vs_atexit_called = 0; > + > +static av_cold void vs_atexit_handler(void); > + > +#ifdef _WIN32 > +static av_cold char* get_vs_script_dll_name(void) { > + LONG r; > + WCHAR vss_path[512]; > + char *vss_path_utf8; > + DWORD buf_size = sizeof(vss_path) - 2; > + r = RegGetValueW(HKEY_CURRENT_USER, L"SOFTWARE\\VapourSynth", > + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, > + &vss_path, &buf_size); > + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) == 0) > + return vss_path_utf8; > + r = RegGetValueW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\VapourSynth", > + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, > + &vss_path, &buf_size); > + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) == 0) > + return vss_path_utf8; > + if (wchartoutf8(L"VSScript.dll", &vss_path_utf8) == 0) > + return vss_path_utf8; > + return 0; > +} > +#endif Don't fetch the path on the registry on Windows. The user should set the path outside of FFmpeg. > + > +static av_cold int vs_load_library(void) > +{ > + VSScriptGetAPIFunc get_vs_script_api; > + vs_script_library.library = VS_DLOPEN(); > + if (!vs_script_library.library) > + return -1; > + get_vs_script_api = (VSScriptGetAPIFunc)dlsym(vs_script_library.library, > + "getVSScriptAPI"); > + if (!get_vs_script_api) { > + dlclose(vs_script_library.library); > + return -2; > + } > + vs_script_library.vssapi = get_vs_script_api(VSSCRIPT_API_VERSION); > + if (!vs_script_library.vssapi) { > + dlclose(vs_script_library.library); > + return -3; > + } > + atexit(vs_atexit_handler); Can you get rid of the call to atexit()? [...] Could you split the patch into first moving to API 4 and then working on the runtime loading? The first part might be reviewed and merged faster. Ramiro
Quoting Stefan Oltmanns via ffmpeg-devel (2024-07-18 11:37:56) > Hello, > > I adressed the issues/concerns that were raised with the first revision > of the patch. > Any feedback? Did I do something wrong? I dislike the concept in general, not to mention adding unacceptable global state.
Hi Ramiro, Am 18.07.24 um 13:08 schrieb Ramiro Polla: > Hi Stefan, > >> [...] >> + >> +#include <vapoursynth/VSScript4.h> >> + >> struct VSState { >> VSScript *vss; >> }; >> >> +typedef const VSSCRIPTAPI *(*VSScriptGetAPIFunc)(int version); >> + >> +typedef struct VSScriptLibrary { >> + void *library; >> + const VSSCRIPTAPI *vssapi; >> +} VSScriptLibrary; >> + >> typedef struct VSContext { >> const AVClass *class; >> >> AVBufferRef *vss_state; >> >> const VSAPI *vsapi; >> - VSCore *vscore; >> >> - VSNodeRef *outnode; >> + VSNode *outnode; >> int is_cfr; >> int current_frame; >> >> @@ -70,19 +94,72 @@ static const AVOption options[] = { >> {NULL} >> }; >> >> +static VSScriptLibrary vs_script_library; > > Does vs_script_library have to be a global? > I think it has to: ffmpeg allows multiple input files, in case you open two VapourSynth files you want to load the Library only once. This is exactly how it's done for AviSynth. >> + >> +static int vs_atexit_called = 0; >> + >> +static av_cold void vs_atexit_handler(void); >> + >> +#ifdef _WIN32 >> +static av_cold char* get_vs_script_dll_name(void) { >> + LONG r; >> + WCHAR vss_path[512]; >> + char *vss_path_utf8; >> + DWORD buf_size = sizeof(vss_path) - 2; >> + r = RegGetValueW(HKEY_CURRENT_USER, L"SOFTWARE\\VapourSynth", >> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, >> + &vss_path, &buf_size); >> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) >> == 0) >> + return vss_path_utf8; >> + r = RegGetValueW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\VapourSynth", >> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, >> + &vss_path, &buf_size); >> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) >> == 0) >> + return vss_path_utf8; >> + if (wchartoutf8(L"VSScript.dll", &vss_path_utf8) == 0) >> + return vss_path_utf8; >> + return 0; >> +} >> +#endif > > Don't fetch the path on the registry on Windows. The user should set the > path outside of FFmpeg. How exactly should the user do that? Additional option to ffmpeg? Fetching the path from the registry is the recommended method by the VaopourSynth author. Would it be an acceptable way to move the registry stuff to the Windows specific area (where for example the LoadLibrary stuff is) and create a function to get a UTF-8 string from the registry, so that there is no Windows-style code in the VapourSynth module? > >> + >> +static av_cold int vs_load_library(void) >> +{ >> + VSScriptGetAPIFunc get_vs_script_api; >> + vs_script_library.library = VS_DLOPEN(); >> + if (!vs_script_library.library) >> + return -1; >> + get_vs_script_api = >> (VSScriptGetAPIFunc)dlsym(vs_script_library.library, >> + "getVSScriptAPI"); >> + if (!get_vs_script_api) { >> + dlclose(vs_script_library.library); >> + return -2; >> + } >> + vs_script_library.vssapi = get_vs_script_api(VSSCRIPT_API_VERSION); >> + if (!vs_script_library.vssapi) { >> + dlclose(vs_script_library.library); >> + return -3; >> + } >> + atexit(vs_atexit_handler); > > Can you get rid of the call to atexit()? Yes, that should be possible. I just read it was only included in AviSynth to work around a crash at exit caused one specific AviSynth variant. So it's probably save to remove. > > [...] > > Could you split the patch into first moving to API 4 and then working on > the runtime loading? The first part might be reviewed and merged faster. I can do that. Best regards, Stefan
On Thu, Jul 18, 2024 at 2:48 PM Stefan Oltmanns via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote: > Am 18.07.24 um 13:08 schrieb Ramiro Polla: > >> [...] > >> + > >> +#include <vapoursynth/VSScript4.h> > >> + > >> struct VSState { > >> VSScript *vss; > >> }; > >> > >> +typedef const VSSCRIPTAPI *(*VSScriptGetAPIFunc)(int version); > >> + > >> +typedef struct VSScriptLibrary { > >> + void *library; > >> + const VSSCRIPTAPI *vssapi; > >> +} VSScriptLibrary; > >> + > >> typedef struct VSContext { > >> const AVClass *class; > >> > >> AVBufferRef *vss_state; > >> > >> const VSAPI *vsapi; > >> - VSCore *vscore; > >> > >> - VSNodeRef *outnode; > >> + VSNode *outnode; > >> int is_cfr; > >> int current_frame; > >> > >> @@ -70,19 +94,72 @@ static const AVOption options[] = { > >> {NULL} > >> }; > >> > >> +static VSScriptLibrary vs_script_library; > > > > Does vs_script_library have to be a global? > > > > I think it has to: ffmpeg allows multiple input files, in case you open > two VapourSynth files you want to load the Library only once. It should be possible to dlopen()/LoadLibrary() multiple times, and the library only gets really unloaded after the last call to dlclose()/FreeLibrary(). In that case you could move that struct to the context. If it's unavoidable to keep the global, at least add some locks to access it. > This is exactly how it's done for AviSynth. Perhaps AviSynth is not the best example to follow :) > >> + > >> +static int vs_atexit_called = 0; > >> + > >> +static av_cold void vs_atexit_handler(void); > >> + > >> +#ifdef _WIN32 > >> +static av_cold char* get_vs_script_dll_name(void) { > >> + LONG r; > >> + WCHAR vss_path[512]; > >> + char *vss_path_utf8; > >> + DWORD buf_size = sizeof(vss_path) - 2; > >> + r = RegGetValueW(HKEY_CURRENT_USER, L"SOFTWARE\\VapourSynth", > >> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, > >> + &vss_path, &buf_size); > >> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) > >> == 0) > >> + return vss_path_utf8; > >> + r = RegGetValueW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\VapourSynth", > >> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, > >> + &vss_path, &buf_size); > >> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) > >> == 0) > >> + return vss_path_utf8; > >> + if (wchartoutf8(L"VSScript.dll", &vss_path_utf8) == 0) > >> + return vss_path_utf8; > >> + return 0; > >> +} > >> +#endif > > > > Don't fetch the path on the registry on Windows. The user should set the > > path outside of FFmpeg. > > How exactly should the user do that? Additional option to ffmpeg? By making sure the libraries are accessible in the PATH environment variable. For example by adding the VapourSynth path to the PATH environment variable, or overriding PATH on the call to FFmpeg on a script. Either way that's outside the scope of FFmpeg. > Fetching the path from the registry is the recommended method by the > VaopourSynth author. Sometimes the recommended method isn't necessarily the best method... > Would it be an acceptable way to move the registry stuff to the Windows > specific area (where for example the LoadLibrary stuff is) and create a > function to get a UTF-8 string from the registry, so that there is no > Windows-style code in the VapourSynth module? It's best to just remove it. [...] Regards, Ramiro
Hi Ramiro, Am 18.07.24 um 15:04 schrieb Ramiro Polla: >>>> [...] >>>> >>>> +static VSScriptLibrary vs_script_library; >>> >>> Does vs_script_library have to be a global? >>> >> >> I think it has to: ffmpeg allows multiple input files, in case you open >> two VapourSynth files you want to load the Library only once. > > It should be possible to dlopen()/LoadLibrary() multiple times, and > the library only gets really unloaded after the last call to > dlclose()/FreeLibrary(). In that case you could move that struct to > the context. If it's unavoidable to keep the global, at least add some > locks to access it. Yes, that should be possible. I did a quick search at it seems that dlopen()/LoadLibrary() is smart and will not open the same library multiple times, but return the same pointer. As dlclose won't be used anymore when removing the atexit handler, that is not an issue at all. > >> This is exactly how it's done for AviSynth. > > Perhaps AviSynth is not the best example to follow :) > >>>> + >>>> +static int vs_atexit_called = 0; >>>> + >>>> +static av_cold void vs_atexit_handler(void); >>>> + >>>> +#ifdef _WIN32 >>>> +static av_cold char* get_vs_script_dll_name(void) { >>>> + LONG r; >>>> + WCHAR vss_path[512]; >>>> + char *vss_path_utf8; >>>> + DWORD buf_size = sizeof(vss_path) - 2; >>>> + r = RegGetValueW(HKEY_CURRENT_USER, L"SOFTWARE\\VapourSynth", >>>> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, >>>> + &vss_path, &buf_size); >>>> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) >>>> == 0) >>>> + return vss_path_utf8; >>>> + r = RegGetValueW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\VapourSynth", >>>> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, >>>> + &vss_path, &buf_size); >>>> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) >>>> == 0) >>>> + return vss_path_utf8; >>>> + if (wchartoutf8(L"VSScript.dll", &vss_path_utf8) == 0) >>>> + return vss_path_utf8; >>>> + return 0; >>>> +} >>>> +#endif >>> >>> Don't fetch the path on the registry on Windows. The user should set the >>> path outside of FFmpeg. >> >> How exactly should the user do that? Additional option to ffmpeg? > > By making sure the libraries are accessible in the PATH environment > variable. For example by adding the VapourSynth path to the PATH > environment variable, or overriding PATH on the call to FFmpeg on a > script. Either way that's outside the scope of FFmpeg. Well, the DLL directory is added to PATH by the VapourSynth installer, but for safety reasons ffmpeg explictly tells the LoadLibrary function to only search the application directory and system32, quote from w32dlfcn.h: > /** > * Safe function used to open dynamic libs. This attempts to improve program security > * by removing the current directory from the dll search path. Only dll's found in the > * executable or system directory are allowed to be loaded. > * @param name The dynamic lib name. > * @return A handle to the opened lib. > */ So ffmpeg prevents that solution on purpose. Or should that behavior be changed in the w32dlfcn.h? Best regards Stefan
On Thu, Jul 18, 2024 at 3:41 PM Stefan Oltmanns via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote: > Am 18.07.24 um 15:04 schrieb Ramiro Polla: > >>>> [...] > >>>> > >>>> +static VSScriptLibrary vs_script_library; > >>> > >>> Does vs_script_library have to be a global? > >>> > >> > >> I think it has to: ffmpeg allows multiple input files, in case you open > >> two VapourSynth files you want to load the Library only once. > > > > It should be possible to dlopen()/LoadLibrary() multiple times, and > > the library only gets really unloaded after the last call to > > dlclose()/FreeLibrary(). In that case you could move that struct to > > the context. If it's unavoidable to keep the global, at least add some > > locks to access it. > > Yes, that should be possible. I did a quick search at it seems that > dlopen()/LoadLibrary() is smart and will not open the same library > multiple times, but return the same pointer. > As dlclose won't be used anymore when removing the atexit handler, that > is not an issue at all. dlclose() will have to be called at some point (i.e.: in read_close). > >> This is exactly how it's done for AviSynth. > > > > Perhaps AviSynth is not the best example to follow :) > > > >>>> + > >>>> +static int vs_atexit_called = 0; > >>>> + > >>>> +static av_cold void vs_atexit_handler(void); > >>>> + > >>>> +#ifdef _WIN32 > >>>> +static av_cold char* get_vs_script_dll_name(void) { > >>>> + LONG r; > >>>> + WCHAR vss_path[512]; > >>>> + char *vss_path_utf8; > >>>> + DWORD buf_size = sizeof(vss_path) - 2; > >>>> + r = RegGetValueW(HKEY_CURRENT_USER, L"SOFTWARE\\VapourSynth", > >>>> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, > >>>> + &vss_path, &buf_size); > >>>> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) > >>>> == 0) > >>>> + return vss_path_utf8; > >>>> + r = RegGetValueW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\VapourSynth", > >>>> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, > >>>> + &vss_path, &buf_size); > >>>> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) > >>>> == 0) > >>>> + return vss_path_utf8; > >>>> + if (wchartoutf8(L"VSScript.dll", &vss_path_utf8) == 0) > >>>> + return vss_path_utf8; > >>>> + return 0; > >>>> +} > >>>> +#endif > >>> > >>> Don't fetch the path on the registry on Windows. The user should set the > >>> path outside of FFmpeg. > >> > >> How exactly should the user do that? Additional option to ffmpeg? > > > > By making sure the libraries are accessible in the PATH environment > > variable. For example by adding the VapourSynth path to the PATH > > environment variable, or overriding PATH on the call to FFmpeg on a > > script. Either way that's outside the scope of FFmpeg. > > Well, the DLL directory is added to PATH by the VapourSynth installer, > but for safety reasons ffmpeg explictly tells the LoadLibrary function > to only search the application directory and system32, quote from > w32dlfcn.h: > > > /** > > * Safe function used to open dynamic libs. This attempts to improve program security > > * by removing the current directory from the dll search path. Only dll's found in the > > * executable or system directory are allowed to be loaded. > > * @param name The dynamic lib name. > > * @return A handle to the opened lib. > > */ > So ffmpeg prevents that solution on purpose. Or should that behavior be > changed in the w32dlfcn.h? Oh, bummer. I would expect that overriding the PATH environment variable would work kind of like how LD_LIBRARY_PATH works on Linux. I don't know why that was changed. I don't really follow what goes on in Windowsland anymore. Does anyone else care to comment on this? Martin, maybe? Ramiro
Am 18.07.24 um 16:21 schrieb Ramiro Polla: > On Thu, Jul 18, 2024 at 3:41 PM Stefan Oltmanns via ffmpeg-devel > <ffmpeg-devel@ffmpeg.org> wrote: >> Am 18.07.24 um 15:04 schrieb Ramiro Polla: >>>>>> [...] >>>>>> >>>>>> +static VSScriptLibrary vs_script_library; >>>>> >>>>> Does vs_script_library have to be a global? >>>>> >>>> >>>> I think it has to: ffmpeg allows multiple input files, in case you open >>>> two VapourSynth files you want to load the Library only once. >>> >>> It should be possible to dlopen()/LoadLibrary() multiple times, and >>> the library only gets really unloaded after the last call to >>> dlclose()/FreeLibrary(). In that case you could move that struct to >>> the context. If it's unavoidable to keep the global, at least add some >>> locks to access it. >> >> Yes, that should be possible. I did a quick search at it seems that >> dlopen()/LoadLibrary() is smart and will not open the same library >> multiple times, but return the same pointer. >> As dlclose won't be used anymore when removing the atexit handler, that >> is not an issue at all. > > dlclose() will have to be called at some point (i.e.: in read_close). > The AviSynth patch to remove it by Stephen Hutchinson does not introduce it somewhere else. It is now only called directly at the start in case a needed function cannot be loaded from the DLL. From what I read dlclose is only needed if there are any C++ deconstructors or similar stuff that need to be called before exiting the program. dlclose usually won't unload the library anyway (the spec does not require dlclose to do that) >>>> This is exactly how it's done for AviSynth. >>> >>> Perhaps AviSynth is not the best example to follow :) Is not using dlclose just another case? >>> >>>>>> + >>>>>> +static int vs_atexit_called = 0; >>>>>> + >>>>>> +static av_cold void vs_atexit_handler(void); >>>>>> + >>>>>> +#ifdef _WIN32 >>>>>> +static av_cold char* get_vs_script_dll_name(void) { >>>>>> + LONG r; >>>>>> + WCHAR vss_path[512]; >>>>>> + char *vss_path_utf8; >>>>>> + DWORD buf_size = sizeof(vss_path) - 2; >>>>>> + r = RegGetValueW(HKEY_CURRENT_USER, L"SOFTWARE\\VapourSynth", >>>>>> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, >>>>>> + &vss_path, &buf_size); >>>>>> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) >>>>>> == 0) >>>>>> + return vss_path_utf8; >>>>>> + r = RegGetValueW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\VapourSynth", >>>>>> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, >>>>>> + &vss_path, &buf_size); >>>>>> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) >>>>>> == 0) >>>>>> + return vss_path_utf8; >>>>>> + if (wchartoutf8(L"VSScript.dll", &vss_path_utf8) == 0) >>>>>> + return vss_path_utf8; >>>>>> + return 0; >>>>>> +} >>>>>> +#endif >>>>> >>>>> Don't fetch the path on the registry on Windows. The user should set the >>>>> path outside of FFmpeg. >>>> >>>> How exactly should the user do that? Additional option to ffmpeg? >>> >>> By making sure the libraries are accessible in the PATH environment >>> variable. For example by adding the VapourSynth path to the PATH >>> environment variable, or overriding PATH on the call to FFmpeg on a >>> script. Either way that's outside the scope of FFmpeg. >> >> Well, the DLL directory is added to PATH by the VapourSynth installer, >> but for safety reasons ffmpeg explictly tells the LoadLibrary function >> to only search the application directory and system32, quote from >> w32dlfcn.h: >> >>> /** >>> * Safe function used to open dynamic libs. This attempts to improve program security >>> * by removing the current directory from the dll search path. Only dll's found in the >>> * executable or system directory are allowed to be loaded. >>> * @param name The dynamic lib name. >>> * @return A handle to the opened lib. >>> */ >> So ffmpeg prevents that solution on purpose. Or should that behavior be >> changed in the w32dlfcn.h? > > Oh, bummer. I would expect that overriding the PATH environment > variable would work kind of like how LD_LIBRARY_PATH works on Linux. I > don't know why that was changed. I don't really follow what goes on in > Windowsland anymore. Does anyone else care to comment on this? Martin, > maybe? > Usually it would work on Windows that way (there is a list of all directories it looks in what order). ffmpeg changes the default behavior. Best regards Stefan
On 18 Jul 2024, at 16:21, Ramiro Polla wrote: > On Thu, Jul 18, 2024 at 3:41 PM Stefan Oltmanns via ffmpeg-devel > <ffmpeg-devel@ffmpeg.org> wrote: >> Am 18.07.24 um 15:04 schrieb Ramiro Polla: >>>>>> [...] >>>>>> >>>>>> +static VSScriptLibrary vs_script_library; >>>>> >>>>> Does vs_script_library have to be a global? >>>>> >>>> >>>> I think it has to: ffmpeg allows multiple input files, in case you open >>>> two VapourSynth files you want to load the Library only once. >>> >>> It should be possible to dlopen()/LoadLibrary() multiple times, and >>> the library only gets really unloaded after the last call to >>> dlclose()/FreeLibrary(). In that case you could move that struct to >>> the context. If it's unavoidable to keep the global, at least add some >>> locks to access it. >> >> Yes, that should be possible. I did a quick search at it seems that >> dlopen()/LoadLibrary() is smart and will not open the same library >> multiple times, but return the same pointer. >> As dlclose won't be used anymore when removing the atexit handler, that >> is not an issue at all. > > dlclose() will have to be called at some point (i.e.: in read_close). > >>>> This is exactly how it's done for AviSynth. >>> >>> Perhaps AviSynth is not the best example to follow :) >>> >>>>>> + >>>>>> +static int vs_atexit_called = 0; >>>>>> + >>>>>> +static av_cold void vs_atexit_handler(void); >>>>>> + >>>>>> +#ifdef _WIN32 >>>>>> +static av_cold char* get_vs_script_dll_name(void) { >>>>>> + LONG r; >>>>>> + WCHAR vss_path[512]; >>>>>> + char *vss_path_utf8; >>>>>> + DWORD buf_size = sizeof(vss_path) - 2; >>>>>> + r = RegGetValueW(HKEY_CURRENT_USER, L"SOFTWARE\\VapourSynth", >>>>>> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, >>>>>> + &vss_path, &buf_size); >>>>>> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) >>>>>> == 0) >>>>>> + return vss_path_utf8; >>>>>> + r = RegGetValueW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\VapourSynth", >>>>>> + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, >>>>>> + &vss_path, &buf_size); >>>>>> + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) >>>>>> == 0) >>>>>> + return vss_path_utf8; >>>>>> + if (wchartoutf8(L"VSScript.dll", &vss_path_utf8) == 0) >>>>>> + return vss_path_utf8; >>>>>> + return 0; >>>>>> +} >>>>>> +#endif >>>>> >>>>> Don't fetch the path on the registry on Windows. The user should set the >>>>> path outside of FFmpeg. >>>> >>>> How exactly should the user do that? Additional option to ffmpeg? >>> >>> By making sure the libraries are accessible in the PATH environment >>> variable. For example by adding the VapourSynth path to the PATH >>> environment variable, or overriding PATH on the call to FFmpeg on a >>> script. Either way that's outside the scope of FFmpeg. >> >> Well, the DLL directory is added to PATH by the VapourSynth installer, >> but for safety reasons ffmpeg explictly tells the LoadLibrary function >> to only search the application directory and system32, quote from >> w32dlfcn.h: >> >>> /** >>> * Safe function used to open dynamic libs. This attempts to improve program security >>> * by removing the current directory from the dll search path. Only dll's found in the >>> * executable or system directory are allowed to be loaded. >>> * @param name The dynamic lib name. >>> * @return A handle to the opened lib. >>> */ >> So ffmpeg prevents that solution on purpose. Or should that behavior be >> changed in the w32dlfcn.h? > > Oh, bummer. I would expect that overriding the PATH environment > variable would work kind of like how LD_LIBRARY_PATH works on Linux. I > don't know why that was changed. I don't really follow what goes on in > Windowsland anymore. Does anyone else care to comment on this? Martin, > maybe? IIRC this is done to prevent DLL injection attacks https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security > > 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-request@ffmpeg.org with subject "unsubscribe".
Hello Anton, can you eloborate on that? What is unacceptable with my patch that is perfectly fine in the AviSynth input module? It's the very same concept. Loading the library at runtime makes it so much more useful, because you can distribute ffmpeg binaries without forcing the user to install VapourSynth (which requires the user to install Python). VapourSynth is not just a library like x264 that you can link in statically if you like, VapourSynth is a frame server (like AviSynth) with it's own dependencies. If you worry about platforms that do not support loading libraries at runtime: VapourSynth is based on plugins that are loaded on runtime, so it won't work on those platforms anyway. Best regards Stefan Am 18.07.24 um 13:25 schrieb Anton Khirnov: > Quoting Stefan Oltmanns via ffmpeg-devel (2024-07-18 11:37:56) >> Hello, >> >> I adressed the issues/concerns that were raised with the first revision >> of the patch. >> Any feedback? Did I do something wrong? > > I dislike the concept in general, not to mention adding unacceptable > global state. >
On 7/18/24 10:53 AM, Stefan Oltmanns via ffmpeg-devel wrote: > The AviSynth patch to remove it by Stephen Hutchinson does not introduce > it somewhere else. It is now only called directly at the start in case a > needed function cannot be loaded from the DLL. > From what I read dlclose is only needed if there are any C++ > deconstructors or similar stuff that need to be called before exiting > the program. > dlclose usually won't unload the library anyway (the spec does not > require dlclose to do that) > That was just an oversight. Interestingly, the only situation that the lack of dlclose appeared to cause a problem with when testing was when the AviSynth host was the standard MSVC build for IA32, under which case the lack of dlclose caused a segfault on exit. The 64-bit build was fine and so were amd64 and i686 GCC builds. Odd, but regardless, fixed in the newer version of the patch.
Am 18.07.24 um 17:23 schrieb epirat07@gmail.com: > >>> >>> Well, the DLL directory is added to PATH by the VapourSynth installer, >>> but for safety reasons ffmpeg explictly tells the LoadLibrary function >>> to only search the application directory and system32, quote from >>> w32dlfcn.h: >>> >>>> /** >>>> * Safe function used to open dynamic libs. This attempts to improve program security >>>> * by removing the current directory from the dll search path. Only dll's found in the >>>> * executable or system directory are allowed to be loaded. >>>> * @param name The dynamic lib name. >>>> * @return A handle to the opened lib. >>>> */ >>> So ffmpeg prevents that solution on purpose. Or should that behavior be >>> changed in the w32dlfcn.h? >> >> Oh, bummer. I would expect that overriding the PATH environment >> variable would work kind of like how LD_LIBRARY_PATH works on Linux. I >> don't know why that was changed. I don't really follow what goes on in >> Windowsland anymore. Does anyone else care to comment on this? Martin, >> maybe? > > IIRC this is done to prevent DLL injection attacks > > https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security > So what's your proposal how to continue? I see different options with pros&cons: 1. Read the DLL path from registry, function for that could be located outside the VapourSynth module. Pro: Safest method to protect from DLL-injection Con: A lot of custom code/functionality for Windows 2. Change w32dlfcn.h to allow loading DLLs from PATH Pro: Minimal code-change, highest similarity between different OSes Con: Open for DLL-injection attacks the current implementations wants to prevent. 3. Allow loading DLLs from PATH with a special flag when calling dlopen. dlopen has a parameter for flags, we could define a WIN_ALLOW_LOAD_DLL_FROM_PATH for Windows that will enable load from PATH Pro: Reduced risk for DLL-injection attack, high similarity between different OSes Con: Flag needs to be defined 0 for other OSes, Posix flags need to be defined 0 for Windows (currently not needed, as the flags are thrown away by the pre-processor) Best regards Stefan
On Mon, Jul 22, 2024 at 12:08 AM Stefan Oltmanns via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote: > > Am 18.07.24 um 17:23 schrieb epirat07@gmail.com: > > > >>> > >>> Well, the DLL directory is added to PATH by the VapourSynth installer, > >>> but for safety reasons ffmpeg explictly tells the LoadLibrary function > >>> to only search the application directory and system32, quote from > >>> w32dlfcn.h: > >>> > >>>> /** > >>>> * Safe function used to open dynamic libs. This attempts to improve program security > >>>> * by removing the current directory from the dll search path. Only dll's found in the > >>>> * executable or system directory are allowed to be loaded. > >>>> * @param name The dynamic lib name. > >>>> * @return A handle to the opened lib. > >>>> */ > >>> So ffmpeg prevents that solution on purpose. Or should that behavior be > >>> changed in the w32dlfcn.h? > >> > >> Oh, bummer. I would expect that overriding the PATH environment > >> variable would work kind of like how LD_LIBRARY_PATH works on Linux. I > >> don't know why that was changed. I don't really follow what goes on in > >> Windowsland anymore. Does anyone else care to comment on this? Martin, > >> maybe? > > > > IIRC this is done to prevent DLL injection attacks > > > > https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security > > > > So what's your proposal how to continue? > > I see different options with pros&cons: > > > 1. > Read the DLL path from registry, function for that could be located > outside the VapourSynth module. > > Pro: Safest method to protect from DLL-injection > Con: A lot of custom code/functionality for Windows > Relaxing security considerations to avoid a 10 line function seems not worth it to me. So go with actually finding the correct path. - Hendrik
Am 22.07.24 um 00:15 schrieb Hendrik Leppkes: > On Mon, Jul 22, 2024 at 12:08 AM Stefan Oltmanns via ffmpeg-devel > <ffmpeg-devel@ffmpeg.org> wrote: >> >> Am 18.07.24 um 17:23 schrieb epirat07@gmail.com: >>> >>>>> >>>>> Well, the DLL directory is added to PATH by the VapourSynth installer, >>>>> but for safety reasons ffmpeg explictly tells the LoadLibrary function >>>>> to only search the application directory and system32, quote from >>>>> w32dlfcn.h: >>>>> >>>>>> /** >>>>>> * Safe function used to open dynamic libs. This attempts to improve program security >>>>>> * by removing the current directory from the dll search path. Only dll's found in the >>>>>> * executable or system directory are allowed to be loaded. >>>>>> * @param name The dynamic lib name. >>>>>> * @return A handle to the opened lib. >>>>>> */ >>>>> So ffmpeg prevents that solution on purpose. Or should that behavior be >>>>> changed in the w32dlfcn.h? >>>> >>>> Oh, bummer. I would expect that overriding the PATH environment >>>> variable would work kind of like how LD_LIBRARY_PATH works on Linux. I >>>> don't know why that was changed. I don't really follow what goes on in >>>> Windowsland anymore. Does anyone else care to comment on this? Martin, >>>> maybe? >>> >>> IIRC this is done to prevent DLL injection attacks >>> >>> https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security >>> >> >> So what's your proposal how to continue? >> >> I see different options with pros&cons: >> >> >> 1. >> Read the DLL path from registry, function for that could be located >> outside the VapourSynth module. >> >> Pro: Safest method to protect from DLL-injection >> Con: A lot of custom code/functionality for Windows >> > > Relaxing security considerations to avoid a 10 line function seems not > worth it to me. So go with actually finding the correct path. > Ok, should that function remain in the vapoursynth module, or should handling the registry be done in a header like "compat/w32registry.h"? An external file with a universal function would of course require more code. Best regards Stefan
On Mon, Jul 22, 2024 at 2:43 AM Stefan Oltmanns via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote: > > Am 22.07.24 um 00:15 schrieb Hendrik Leppkes: > > On Mon, Jul 22, 2024 at 12:08 AM Stefan Oltmanns via ffmpeg-devel > > <ffmpeg-devel@ffmpeg.org> wrote: > >> > >> Am 18.07.24 um 17:23 schrieb epirat07@gmail.com: > >>> > >>>>> > >>>>> Well, the DLL directory is added to PATH by the VapourSynth installer, > >>>>> but for safety reasons ffmpeg explictly tells the LoadLibrary function > >>>>> to only search the application directory and system32, quote from > >>>>> w32dlfcn.h: > >>>>> > >>>>>> /** > >>>>>> * Safe function used to open dynamic libs. This attempts to improve program security > >>>>>> * by removing the current directory from the dll search path. Only dll's found in the > >>>>>> * executable or system directory are allowed to be loaded. > >>>>>> * @param name The dynamic lib name. > >>>>>> * @return A handle to the opened lib. > >>>>>> */ > >>>>> So ffmpeg prevents that solution on purpose. Or should that behavior be > >>>>> changed in the w32dlfcn.h? > >>>> > >>>> Oh, bummer. I would expect that overriding the PATH environment > >>>> variable would work kind of like how LD_LIBRARY_PATH works on Linux. I > >>>> don't know why that was changed. I don't really follow what goes on in > >>>> Windowsland anymore. Does anyone else care to comment on this? Martin, > >>>> maybe? > >>> > >>> IIRC this is done to prevent DLL injection attacks > >>> > >>> https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security > >>> > >> > >> So what's your proposal how to continue? > >> > >> I see different options with pros&cons: > >> > >> > >> 1. > >> Read the DLL path from registry, function for that could be located > >> outside the VapourSynth module. > >> > >> Pro: Safest method to protect from DLL-injection > >> Con: A lot of custom code/functionality for Windows > >> > > > > Relaxing security considerations to avoid a 10 line function seems not > > worth it to me. So go with actually finding the correct path. > > > > Ok, should that function remain in the vapoursynth module, or should > handling the registry be done in a header like "compat/w32registry.h"? > > An external file with a universal function would of course require more > code. > Just have it in vapoursynth. Compat would be the wrong place for it, but in general I don't think we're going to need a lot of registry access in the future. - Hendrik
Quoting Stefan Oltmanns (2024-07-18 14:12:42) > Hello Anton, > > can you eloborate on that? What is unacceptable with my patch that is > perfectly fine in the AviSynth input module? It's the very same concept. It's not perfectly fine in avisynth, I dislike it there as well. There are also recent patches that remove the atexit handler. > Loading the library at runtime makes it so much more useful, because you > can distribute ffmpeg binaries without forcing the user to install > VapourSynth (which requires the user to install Python). Runtime loading hides dependencies from standard tools and makes program behaviour harder to analyze. Not to mention you're adding a bunch of global state, which is evil. > VapourSynth is not just a library like x264 that you can link in > statically if you like, VapourSynth is a frame server (like AviSynth) > with it's own dependencies. > If you worry about platforms that do not support loading libraries at > runtime: VapourSynth is based on plugins that are loaded on runtime, so > it won't work on those platforms anyway. I am worried about special "demuxers" than are not really demuxers and don't work like other demuxers, hence massively increasing library maintenance load.
On Mon, Jul 22, 2024 at 12:15 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Mon, Jul 22, 2024 at 12:08 AM Stefan Oltmanns via ffmpeg-devel > <ffmpeg-devel@ffmpeg.org> wrote: > > > > Am 18.07.24 um 17:23 schrieb epirat07@gmail.com: > > > > > >>> > > >>> Well, the DLL directory is added to PATH by the VapourSynth installer, > > >>> but for safety reasons ffmpeg explictly tells the LoadLibrary function > > >>> to only search the application directory and system32, quote from > > >>> w32dlfcn.h: > > >>> > > >>>> /** > > >>>> * Safe function used to open dynamic libs. This attempts to improve program security > > >>>> * by removing the current directory from the dll search path. Only dll's found in the > > >>>> * executable or system directory are allowed to be loaded. > > >>>> * @param name The dynamic lib name. > > >>>> * @return A handle to the opened lib. > > >>>> */ > > >>> So ffmpeg prevents that solution on purpose. Or should that behavior be > > >>> changed in the w32dlfcn.h? > > >> > > >> Oh, bummer. I would expect that overriding the PATH environment > > >> variable would work kind of like how LD_LIBRARY_PATH works on Linux. I > > >> don't know why that was changed. I don't really follow what goes on in > > >> Windowsland anymore. Does anyone else care to comment on this? Martin, > > >> maybe? > > > > > > IIRC this is done to prevent DLL injection attacks > > > > > > https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security > > > > > > > So what's your proposal how to continue? > > > > I see different options with pros&cons: > > > > > > 1. > > Read the DLL path from registry, function for that could be located > > outside the VapourSynth module. > > > > Pro: Safest method to protect from DLL-injection > > Con: A lot of custom code/functionality for Windows > > > > Relaxing security considerations to avoid a 10 line function seems not > worth it to me. So go with actually finding the correct path. I would prefer changing w32dlfcn.h to allow loading DLLs from PATH. Limiting to only the directory of the executable and system32 seems too excessive to me. Removing the current working directory is more understandable, but it's perfectly fine to expect PATH to be searched. Also, we don't have such restrictions on other platforms. (DY)LD_LIBRARY_PATH still work as expected. Ramiro
On Mon, Jul 22, 2024 at 2:14 PM Ramiro Polla <ramiro.polla@gmail.com> wrote: > > On Mon, Jul 22, 2024 at 12:15 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > On Mon, Jul 22, 2024 at 12:08 AM Stefan Oltmanns via ffmpeg-devel > > <ffmpeg-devel@ffmpeg.org> wrote: > > > > > > Am 18.07.24 um 17:23 schrieb epirat07@gmail.com: > > > > > > > >>> > > > >>> Well, the DLL directory is added to PATH by the VapourSynth installer, > > > >>> but for safety reasons ffmpeg explictly tells the LoadLibrary function > > > >>> to only search the application directory and system32, quote from > > > >>> w32dlfcn.h: > > > >>> > > > >>>> /** > > > >>>> * Safe function used to open dynamic libs. This attempts to improve program security > > > >>>> * by removing the current directory from the dll search path. Only dll's found in the > > > >>>> * executable or system directory are allowed to be loaded. > > > >>>> * @param name The dynamic lib name. > > > >>>> * @return A handle to the opened lib. > > > >>>> */ > > > >>> So ffmpeg prevents that solution on purpose. Or should that behavior be > > > >>> changed in the w32dlfcn.h? > > > >> > > > >> Oh, bummer. I would expect that overriding the PATH environment > > > >> variable would work kind of like how LD_LIBRARY_PATH works on Linux. I > > > >> don't know why that was changed. I don't really follow what goes on in > > > >> Windowsland anymore. Does anyone else care to comment on this? Martin, > > > >> maybe? > > > > > > > > IIRC this is done to prevent DLL injection attacks > > > > > > > > https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security > > > > > > > > > > So what's your proposal how to continue? > > > > > > I see different options with pros&cons: > > > > > > > > > 1. > > > Read the DLL path from registry, function for that could be located > > > outside the VapourSynth module. > > > > > > Pro: Safest method to protect from DLL-injection > > > Con: A lot of custom code/functionality for Windows > > > > > > > Relaxing security considerations to avoid a 10 line function seems not > > worth it to me. So go with actually finding the correct path. > > I would prefer changing w32dlfcn.h to allow loading DLLs from PATH. > Limiting to only the directory of the executable and system32 seems > too excessive to me. Removing the current working directory is more > understandable, but it's perfectly fine to expect PATH to be searched. > This is common and largely expected DLL loading behavior on Windows. Changing that to avoid 10 lines of code is rather ill advised. - Hendrik
Am 22.07.24 um 14:13 schrieb Ramiro Polla: > On Mon, Jul 22, 2024 at 12:15 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote: >> On Mon, Jul 22, 2024 at 12:08 AM Stefan Oltmanns via ffmpeg-devel >> <ffmpeg-devel@ffmpeg.org> wrote: >>> >>> Am 18.07.24 um 17:23 schrieb epirat07@gmail.com: >>>> >>>>>> >>>>>> Well, the DLL directory is added to PATH by the VapourSynth installer, >>>>>> but for safety reasons ffmpeg explictly tells the LoadLibrary function >>>>>> to only search the application directory and system32, quote from >>>>>> w32dlfcn.h: >>>>>> >>>>>>> /** >>>>>>> * Safe function used to open dynamic libs. This attempts to improve program security >>>>>>> * by removing the current directory from the dll search path. Only dll's found in the >>>>>>> * executable or system directory are allowed to be loaded. >>>>>>> * @param name The dynamic lib name. >>>>>>> * @return A handle to the opened lib. >>>>>>> */ >>>>>> So ffmpeg prevents that solution on purpose. Or should that behavior be >>>>>> changed in the w32dlfcn.h? >>>>> >>>>> Oh, bummer. I would expect that overriding the PATH environment >>>>> variable would work kind of like how LD_LIBRARY_PATH works on Linux. I >>>>> don't know why that was changed. I don't really follow what goes on in >>>>> Windowsland anymore. Does anyone else care to comment on this? Martin, >>>>> maybe? >>>> >>>> IIRC this is done to prevent DLL injection attacks >>>> >>>> https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security >>>> >>> >>> So what's your proposal how to continue? >>> >>> I see different options with pros&cons: >>> >>> >>> 1. >>> Read the DLL path from registry, function for that could be located >>> outside the VapourSynth module. >>> >>> Pro: Safest method to protect from DLL-injection >>> Con: A lot of custom code/functionality for Windows >>> >> >> Relaxing security considerations to avoid a 10 line function seems not >> worth it to me. So go with actually finding the correct path. > > I would prefer changing w32dlfcn.h to allow loading DLLs from PATH. > Limiting to only the directory of the executable and system32 seems > too excessive to me. Removing the current working directory is more > understandable, but it's perfectly fine to expect PATH to be searched. > > Also, we don't have such restrictions on other platforms. > (DY)LD_LIBRARY_PATH still work as expected. > I had a look at the documentation from LoadLibrary: That does not seem to be an option, you cannot explicitly allow PATH. If you want to allow PATH, you need the option for default search directories and that will also include the current working directory. You can use custom search paths (that's what ffmpeg does for older Windows where these flags don't exist), but those have to be explicit, so no help here. Best regards Stefan
On Mon, Jul 22, 2024 at 4:01 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Mon, Jul 22, 2024 at 2:14 PM Ramiro Polla <ramiro.polla@gmail.com> wrote: > > On Mon, Jul 22, 2024 at 12:15 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > > On Mon, Jul 22, 2024 at 12:08 AM Stefan Oltmanns via ffmpeg-devel > > > <ffmpeg-devel@ffmpeg.org> wrote: > > > > Am 18.07.24 um 17:23 schrieb epirat07@gmail.com: > > > > >>> Well, the DLL directory is added to PATH by the VapourSynth installer, > > > > >>> but for safety reasons ffmpeg explictly tells the LoadLibrary function > > > > >>> to only search the application directory and system32, quote from > > > > >>> w32dlfcn.h: > > > > >>> > > > > >>>> /** > > > > >>>> * Safe function used to open dynamic libs. This attempts to improve program security > > > > >>>> * by removing the current directory from the dll search path. Only dll's found in the > > > > >>>> * executable or system directory are allowed to be loaded. > > > > >>>> * @param name The dynamic lib name. > > > > >>>> * @return A handle to the opened lib. > > > > >>>> */ > > > > >>> So ffmpeg prevents that solution on purpose. Or should that behavior be > > > > >>> changed in the w32dlfcn.h? > > > > >> > > > > >> Oh, bummer. I would expect that overriding the PATH environment > > > > >> variable would work kind of like how LD_LIBRARY_PATH works on Linux. I > > > > >> don't know why that was changed. I don't really follow what goes on in > > > > >> Windowsland anymore. Does anyone else care to comment on this? Martin, > > > > >> maybe? > > > > > > > > > > IIRC this is done to prevent DLL injection attacks > > > > > > > > > > https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security > > > > > > > > > > > > > So what's your proposal how to continue? > > > > > > > > I see different options with pros&cons: > > > > > > > > > > > > 1. > > > > Read the DLL path from registry, function for that could be located > > > > outside the VapourSynth module. > > > > > > > > Pro: Safest method to protect from DLL-injection > > > > Con: A lot of custom code/functionality for Windows > > > > > > > > > > Relaxing security considerations to avoid a 10 line function seems not > > > worth it to me. So go with actually finding the correct path. > > > > I would prefer changing w32dlfcn.h to allow loading DLLs from PATH. > > Limiting to only the directory of the executable and system32 seems > > too excessive to me. Removing the current working directory is more > > understandable, but it's perfectly fine to expect PATH to be searched. > > This is common and largely expected DLL loading behavior on Windows. I was surprised by this statement, but it seems that the expectations on Windows have indeed changed over the last decade. I guess I've just been away for too long... Ramiro
Am 22.07.24 um 08:57 schrieb Anton Khirnov: > Quoting Stefan Oltmanns (2024-07-18 14:12:42) >> Hello Anton, >> >> can you eloborate on that? What is unacceptable with my patch that is >> perfectly fine in the AviSynth input module? It's the very same concept. > > It's not perfectly fine in avisynth, I dislike it there as well. There > are also recent patches that remove the atexit handler. The atexit will be removed in the next revision of the patch. > >> Loading the library at runtime makes it so much more useful, because you >> can distribute ffmpeg binaries without forcing the user to install >> VapourSynth (which requires the user to install Python). > > Runtime loading hides dependencies from standard tools and makes program > behaviour harder to analyze. Not to mention you're adding a bunch of > global state, which is evil. All global states will be removed in the next revision of the patch. It's the intention of my patch to reduce ("hide") the VapourSynth dependency, so unless you want to actually open a VapourSynth script there is no dependency to it. If you try to open a VapourScript script without having VapourSynth installed, you'll get an error message. VapourSynth itself has unclear dependencies, it will load plug-ins on runtime and as it uses Python you can in fact load other Python libaries, for example AI stuff like PyTorch for fancy upscaling, that will then load CUDA/ROCM. > >> VapourSynth is not just a library like x264 that you can link in >> statically if you like, VapourSynth is a frame server (like AviSynth) >> with it's own dependencies. >> If you worry about platforms that do not support loading libraries at >> runtime: VapourSynth is based on plugins that are loaded on runtime, so >> it won't work on those platforms anyway. > > I am worried about special "demuxers" than are not really demuxers and > don't work like other demuxers, hence massively increasing library > maintenance load. > I somewhat agree here, when I first saw a AviSynth demuxer in the list of supported formats it looked weird, because it's not a format that you demux. But what's the solution? Create a new library like "avframeserver" for 2 (?) different tools? How do they increase the maintinace load? There are a lot of external libraries that get used by ffmpeg, what's the difference here? As these formats do not contain any advanced video codec, but raw video, shouldn't it be rather easy to maintain, because no weird complications with some decoder? Best regards Stefan
From 759b097865953ee66949ecbcdadbebfad623c29a Mon Sep 17 00:00:00 2001 From: Stefan Oltmanns <stefan-oltmanns@gmx.net> Date: Sat, 6 Jul 2024 22:56:53 +0200 Subject: [PATCH] avformat/vapoursynth: Update to API version 4, load library at runtime Signed-off-by: Stefan Oltmanns <stefan-oltmanns@gmx.net> --- configure | 3 +- libavformat/vapoursynth.c | 171 +++++++++++++++++++++++++++++--------- 2 files changed, 136 insertions(+), 38 deletions(-) diff --git a/configure b/configure index b28221f258..e43f3827ec 100755 --- a/configure +++ b/configure @@ -3575,6 +3575,7 @@ libxevd_decoder_deps="libxevd" libxeve_encoder_deps="libxeve" libxvid_encoder_deps="libxvid" libzvbi_teletext_decoder_deps="libzvbi" +vapoursynth_deps_any="libdl LoadLibrary" vapoursynth_demuxer_deps="vapoursynth" videotoolbox_suggest="coreservices" videotoolbox_deps="corefoundation coremedia corevideo" @@ -7080,7 +7081,7 @@ enabled rkmpp && { require_pkg_config rkmpp rockchip_mpp rockchip/r { enabled libdrm || die "ERROR: rkmpp requires --enable-libdrm"; } } -enabled vapoursynth && require_pkg_config vapoursynth "vapoursynth-script >= 42" VSScript.h vsscript_init +enabled vapoursynth && require_headers "vapoursynth/VSScript4.h vapoursynth/VapourSynth4.h" if enabled gcrypt; then diff --git a/libavformat/vapoursynth.c b/libavformat/vapoursynth.c index 8a2519e19a..9c82f8f3b8 100644 --- a/libavformat/vapoursynth.c +++ b/libavformat/vapoursynth.c @@ -25,9 +25,6 @@ #include <limits.h> -#include <VapourSynth.h> -#include <VSScript.h> - #include "libavutil/avassert.h" #include "libavutil/avstring.h" #include "libavutil/eval.h" @@ -40,19 +37,46 @@ #include "demux.h" #include "internal.h" +/* Platform-specific directives. */ +#ifdef _WIN32 + #include <windows.h> + #include "compat/w32dlfcn.h" + #include "libavutil/wchar_filename.h" + #undef EXTERN_C + #define VSSCRIPT_LIB "VSScript.dll" + #define VS_DLOPEN() ({ void *handle = NULL; \ + char *dll_name = get_vs_script_dll_name(); \ + handle = dlopen(dll_name, RTLD_NOW | RTLD_GLOBAL); \ + av_free(dll_name); \ + handle; }) +#else + #include <dlfcn.h> + #define VSSCRIPT_NAME "libvapoursynth-script" + #define VSSCRIPT_LIB VSSCRIPT_NAME SLIBSUF + #define VS_DLOPEN() dlopen(VSSCRIPT_LIB, RTLD_NOW | RTLD_GLOBAL) +#endif + +#include <vapoursynth/VSScript4.h> + struct VSState { VSScript *vss; }; +typedef const VSSCRIPTAPI *(*VSScriptGetAPIFunc)(int version); + +typedef struct VSScriptLibrary { + void *library; + const VSSCRIPTAPI *vssapi; +} VSScriptLibrary; + typedef struct VSContext { const AVClass *class; AVBufferRef *vss_state; const VSAPI *vsapi; - VSCore *vscore; - VSNodeRef *outnode; + VSNode *outnode; int is_cfr; int current_frame; @@ -70,19 +94,72 @@ static const AVOption options[] = { {NULL} }; +static VSScriptLibrary vs_script_library; + +static int vs_atexit_called = 0; + +static av_cold void vs_atexit_handler(void); + +#ifdef _WIN32 +static av_cold char* get_vs_script_dll_name(void) { + LONG r; + WCHAR vss_path[512]; + char *vss_path_utf8; + DWORD buf_size = sizeof(vss_path) - 2; + r = RegGetValueW(HKEY_CURRENT_USER, L"SOFTWARE\\VapourSynth", + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, + &vss_path, &buf_size); + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) == 0) + return vss_path_utf8; + r = RegGetValueW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\VapourSynth", + L"VSScriptDLL", RRF_RT_REG_SZ, NULL, + &vss_path, &buf_size); + if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) == 0) + return vss_path_utf8; + if (wchartoutf8(L"VSScript.dll", &vss_path_utf8) == 0) + return vss_path_utf8; + return 0; +} +#endif + +static av_cold int vs_load_library(void) +{ + VSScriptGetAPIFunc get_vs_script_api; + vs_script_library.library = VS_DLOPEN(); + if (!vs_script_library.library) + return -1; + get_vs_script_api = (VSScriptGetAPIFunc)dlsym(vs_script_library.library, + "getVSScriptAPI"); + if (!get_vs_script_api) { + dlclose(vs_script_library.library); + return -2; + } + vs_script_library.vssapi = get_vs_script_api(VSSCRIPT_API_VERSION); + if (!vs_script_library.vssapi) { + dlclose(vs_script_library.library); + return -3; + } + atexit(vs_atexit_handler); + return 0; +} + static void free_vss_state(void *opaque, uint8_t *data) { struct VSState *vss = opaque; if (vss->vss) { - vsscript_freeScript(vss->vss); - vsscript_finalize(); + vs_script_library.vssapi->freeScript(vss->vss); } } static av_cold int read_close_vs(AVFormatContext *s) { - VSContext *vs = s->priv_data; + VSContext *vs; + + if (vs_atexit_called) + return 0; + + vs = s->priv_data; if (vs->outnode) vs->vsapi->freeNode(vs->outnode); @@ -90,12 +167,17 @@ static av_cold int read_close_vs(AVFormatContext *s) av_buffer_unref(&vs->vss_state); vs->vsapi = NULL; - vs->vscore = NULL; vs->outnode = NULL; return 0; } +static av_cold void vs_atexit_handler(void) +{ + dlclose(vs_script_library.library); + vs_atexit_called = 1; +} + static av_cold int is_native_endian(enum AVPixelFormat pixfmt) { enum AVPixelFormat other = av_pix_fmt_swap_endianness(pixfmt); @@ -106,7 +188,7 @@ static av_cold int is_native_endian(enum AVPixelFormat pixfmt) return pd && (!!HAVE_BIGENDIAN == !!(pd->flags & AV_PIX_FMT_FLAG_BE)); } -static av_cold enum AVPixelFormat match_pixfmt(const VSFormat *vsf, int c_order[4]) +static av_cold enum AVPixelFormat match_pixfmt(const VSVideoFormat *vsf, int c_order[4]) { static const int yuv_order[4] = {0, 1, 2, 0}; static const int rgb_order[4] = {1, 2, 0, 0}; @@ -128,13 +210,12 @@ static av_cold enum AVPixelFormat match_pixfmt(const VSFormat *vsf, int c_order[ pd->log2_chroma_h != vsf->subSamplingH) continue; - is_rgb = vsf->colorFamily == cmRGB; + is_rgb = vsf->colorFamily == cfRGB; if (is_rgb != !!(pd->flags & AV_PIX_FMT_FLAG_RGB)) continue; - is_yuv = vsf->colorFamily == cmYUV || - vsf->colorFamily == cmYCoCg || - vsf->colorFamily == cmGray; + is_yuv = vsf->colorFamily == cfYUV || + vsf->colorFamily == cfGray; if (!is_rgb && !is_yuv) continue; @@ -176,6 +257,7 @@ static av_cold int read_header_vs(AVFormatContext *s) int64_t sz = avio_size(pb); char *buf = NULL; char dummy; + char vsfmt[32]; const VSVideoInfo *info; struct VSState *vss_state; int err = 0; @@ -193,16 +275,30 @@ static av_cold int read_header_vs(AVFormatContext *s) goto done; } - if (!vsscript_init()) { - av_log(s, AV_LOG_ERROR, "Failed to initialize VSScript (possibly PYTHONPATH not set).\n"); + if (err = vs_load_library()) { + if (err == -1) av_log(s, AV_LOG_ERROR, "Could not open " VSSCRIPT_LIB + ". Check VapourSynth installation.\n"); + else if (err == -2) av_log(s, AV_LOG_ERROR, + "Could not load VapourSynth library. " + "VapourSynth installation may be outdated " + "or broken.\n"); + else if (err == -3) av_log(s, AV_LOG_ERROR, + "Failed to initialize VSScript " + "(possibly PYTHONPATH not set).\n"); err = AVERROR_EXTERNAL; goto done; } - if (vsscript_createScript(&vss_state->vss)) { + if (!(vs->vsapi = vs_script_library.vssapi->getVSAPI(VAPOURSYNTH_API_VERSION))) { + av_log(s, AV_LOG_ERROR, "Could not get VSAPI. " + "Check VapourSynth installation.\n"); + err = AVERROR_EXTERNAL; + goto done; + } + + if (!(vss_state->vss = vs_script_library.vssapi->createScript(NULL))) { av_log(s, AV_LOG_ERROR, "Failed to create script instance.\n"); err = AVERROR_EXTERNAL; - vsscript_finalize(); goto done; } @@ -235,17 +331,14 @@ static av_cold int read_header_vs(AVFormatContext *s) } buf[sz] = '\0'; - if (vsscript_evaluateScript(&vss_state->vss, buf, s->url, 0)) { - const char *msg = vsscript_getError(vss_state->vss); + if (vs_script_library.vssapi->evaluateBuffer(vss_state->vss, buf, s->url)) { + const char *msg = vs_script_library.vssapi->getError(vss_state->vss); av_log(s, AV_LOG_ERROR, "Failed to parse script: %s\n", msg ? msg : "(unknown)"); err = AVERROR_EXTERNAL; goto done; } - vs->vsapi = vsscript_getVSApi(); - vs->vscore = vsscript_getCore(vss_state->vss); - - vs->outnode = vsscript_getOutput(vss_state->vss, 0); + vs->outnode = vs_script_library.vssapi->getOutputNode(vss_state->vss, 0); if (!vs->outnode) { av_log(s, AV_LOG_ERROR, "Could not get script output node.\n"); err = AVERROR_EXTERNAL; @@ -260,7 +353,7 @@ static av_cold int read_header_vs(AVFormatContext *s) info = vs->vsapi->getVideoInfo(vs->outnode); - if (!info->format || !info->width || !info->height) { + if (!info->format.colorFamily || !info->width || !info->height) { av_log(s, AV_LOG_ERROR, "Non-constant input format not supported.\n"); err = AVERROR_PATCHWELCOME; goto done; @@ -280,18 +373,22 @@ static av_cold int read_header_vs(AVFormatContext *s) st->codecpar->codec_id = AV_CODEC_ID_WRAPPED_AVFRAME; st->codecpar->width = info->width; st->codecpar->height = info->height; - st->codecpar->format = match_pixfmt(info->format, vs->c_order); + st->codecpar->format = match_pixfmt(&info->format, vs->c_order); if (st->codecpar->format == AV_PIX_FMT_NONE) { - av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format %s\n", info->format->name); + if(vs->vsapi->getVideoFormatName(&info->format, vsfmt)) + av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format %s\n", vsfmt); + else + av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format\n"); err = AVERROR_EXTERNAL; goto done; } - av_log(s, AV_LOG_VERBOSE, "VS format %s -> pixfmt %s\n", info->format->name, - av_get_pix_fmt_name(st->codecpar->format)); - - if (info->format->colorFamily == cmYCoCg) - st->codecpar->color_space = AVCOL_SPC_YCGCO; + if (vs->vsapi->getVideoFormatName(&info->format, vsfmt)) + av_log(s, AV_LOG_VERBOSE, "VS format %s -> pixfmt %s\n", + vsfmt, av_get_pix_fmt_name(st->codecpar->format)); + else + av_log(s, AV_LOG_VERBOSE, "VS format -> pixfmt %s\n", + av_get_pix_fmt_name(st->codecpar->format)); done: av_free(buf); @@ -311,13 +408,13 @@ static int get_vs_prop_int(AVFormatContext *s, const VSMap *map, const char *nam int64_t res; int err = 1; - res = vs->vsapi->propGetInt(map, name, 0, &err); + res = vs->vsapi->mapGetInt(map, name, 0, &err); return err || res < INT_MIN || res > INT_MAX ? def : res; } struct vsframe_ref_data { const VSAPI *vsapi; - const VSFrameRef *frame; + const VSFrame *frame; AVBufferRef *vss_state; }; @@ -339,7 +436,7 @@ static int read_packet_vs(AVFormatContext *s, AVPacket *pkt) AVStream *st = s->streams[0]; AVFrame *frame = NULL; char vserr[80]; - const VSFrameRef *vsframe; + const VSFrame *vsframe; const VSVideoInfo *info = vs->vsapi->getVideoInfo(vs->outnode); const VSMap *props; const AVPixFmtDescriptor *desc; @@ -381,7 +478,7 @@ static int read_packet_vs(AVFormatContext *s, AVPacket *pkt) goto end; } - props = vs->vsapi->getFramePropsRO(vsframe); + props = vs->vsapi->getFramePropertiesRO(vsframe); frame = av_frame_alloc(); if (!frame) { @@ -410,7 +507,7 @@ static int read_packet_vs(AVFormatContext *s, AVPacket *pkt) desc = av_pix_fmt_desc_get(frame->format); - for (i = 0; i < info->format->numPlanes; i++) { + for (i = 0; i < info->format.numPlanes; i++) { int p = vs->c_order[i]; ptrdiff_t plane_h = frame->height; -- 2.34.1