| Message ID | 20240719165627.5490-1-qyot27@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2,v2] avformat/avisynth: remove atexit() handler | expand |
On 7/19/24 12:56 PM, Stephen Hutchinson wrote: > The atexit() handler in the avisynth demuxer was added because > there was a conflict in AvxSynth that arose due to their use > of C++ global objects, particularly in relation to having > added a logging function relying on log4cpp. > > This conflict was responsible for causing a segfault on exit. > It did not affect Windows with the (at the time) upstream > AviSynth 2.5 and 2.6, nor does it affect AviSynth+. > > Unfortunately, none of this was actually shielded by ifdefs > indicating the fact it was only needed for AvxSynth, so four > years ago when AviSynth+ replaced AvxSynth as the handler > for AviSynth scripts on Unix-like OSes, the fact that the > atexit handler was no longer necessary was overlooked. > > Signed-off-by: Stephen Hutchinson <qyot27@gmail.com> > --- > Changes compared to v1: > * Added missing dlclose invocation to read_close > > libavformat/avisynth.c | 46 +----------------------------------------- > 1 file changed, 1 insertion(+), 45 deletions(-) > > diff --git a/libavformat/avisynth.c b/libavformat/avisynth.c > index 625bdf7e3a..26747671c0 100644 > --- a/libavformat/avisynth.c > +++ b/libavformat/avisynth.c > @@ -115,9 +115,6 @@ typedef struct AviSynthContext { > int error; > > uint32_t flags; > - > - /* Linked list pointers. */ > - struct AviSynthContext *next; > } AviSynthContext; > > static const int avs_planes_packed[1] = { 0 }; > @@ -133,15 +130,7 @@ static const int avs_planes_rgba[4] = { AVS_PLANAR_G, AVS_PLANAR_B, > > static AVMutex avisynth_mutex = AV_MUTEX_INITIALIZER; > > -/* A conflict between C++ global objects, atexit, and dynamic loading requires > - * us to register our own atexit handler to prevent double freeing. */ > static AviSynthLibrary avs_library; > -static int avs_atexit_called = 0; > - > -/* Linked list of AviSynthContexts. An atexit handler destroys this list. */ > -static AviSynthContext *avs_ctx_list = NULL; > - > -static av_cold void avisynth_atexit_handler(void); > > static av_cold int avisynth_load_library(void) > { > @@ -185,7 +174,6 @@ static av_cold int avisynth_load_library(void) > LOAD_AVS_FUNC(avs_get_env_property, 1); > #undef LOAD_AVS_FUNC > > - atexit(avisynth_atexit_handler); > return 0; > > fail: > @@ -214,30 +202,11 @@ static av_cold int avisynth_context_create(AVFormatContext *s) > } > } > > - if (!avs_ctx_list) { > - avs_ctx_list = avs; > - } else { > - avs->next = avs_ctx_list; > - avs_ctx_list = avs; > - } > - > return 0; > } > > static av_cold void avisynth_context_destroy(AviSynthContext *avs) > { > - if (avs_atexit_called) > - return; > - > - if (avs == avs_ctx_list) { > - avs_ctx_list = avs->next; > - } else { > - AviSynthContext *prev = avs_ctx_list; > - while (prev->next != avs) > - prev = prev->next; > - prev->next = avs->next; > - } > - > if (avs->clip) { > avs_library.avs_release_clip(avs->clip); > avs->clip = NULL; > @@ -248,20 +217,6 @@ static av_cold void avisynth_context_destroy(AviSynthContext *avs) > } > } > > -static av_cold void avisynth_atexit_handler(void) > -{ > - AviSynthContext *avs = avs_ctx_list; > - > - while (avs) { > - AviSynthContext *next = avs->next; > - avisynth_context_destroy(avs); > - avs = next; > - } > - dlclose(avs_library.library); > - > - avs_atexit_called = 1; > -} > - > /* Create AVStream from audio and video data. */ > static int avisynth_create_stream_video(AVFormatContext *s, AVStream *st) > { > @@ -1134,6 +1089,7 @@ static av_cold int avisynth_read_close(AVFormatContext *s) > return AVERROR_UNKNOWN; > > avisynth_context_destroy(s->priv_data); > + dlclose(avs_library.library); > ff_mutex_unlock(&avisynth_mutex); > return 0; > } Since there's not been any more feedback, I'll push both of these refined patches in a day or two.
On Fri, Jul 19, 2024 at 7:51 PM Stephen Hutchinson <qyot27@gmail.com> wrote: > > The atexit() handler in the avisynth demuxer was added because > there was a conflict in AvxSynth that arose due to their use > of C++ global objects, particularly in relation to having > added a logging function relying on log4cpp. > > This conflict was responsible for causing a segfault on exit. > It did not affect Windows with the (at the time) upstream > AviSynth 2.5 and 2.6, nor does it affect AviSynth+. > > Unfortunately, none of this was actually shielded by ifdefs > indicating the fact it was only needed for AvxSynth, so four > years ago when AviSynth+ replaced AvxSynth as the handler > for AviSynth scripts on Unix-like OSes, the fact that the > atexit handler was no longer necessary was overlooked. > > Signed-off-by: Stephen Hutchinson <qyot27@gmail.com> > --- > Changes compared to v1: > * Added missing dlclose invocation to read_close > > libavformat/avisynth.c | 46 +----------------------------------------- > 1 file changed, 1 insertion(+), 45 deletions(-) > > diff --git a/libavformat/avisynth.c b/libavformat/avisynth.c > index 625bdf7e3a..26747671c0 100644 > --- a/libavformat/avisynth.c > +++ b/libavformat/avisynth.c > @@ -115,9 +115,6 @@ typedef struct AviSynthContext { > int error; > > uint32_t flags; > - > - /* Linked list pointers. */ > - struct AviSynthContext *next; > } AviSynthContext; > > static const int avs_planes_packed[1] = { 0 }; > @@ -133,15 +130,7 @@ static const int avs_planes_rgba[4] = { AVS_PLANAR_G, AVS_PLANAR_B, > > static AVMutex avisynth_mutex = AV_MUTEX_INITIALIZER; > > -/* A conflict between C++ global objects, atexit, and dynamic loading requires > - * us to register our own atexit handler to prevent double freeing. */ > static AviSynthLibrary avs_library; > -static int avs_atexit_called = 0; > - > -/* Linked list of AviSynthContexts. An atexit handler destroys this list. */ > -static AviSynthContext *avs_ctx_list = NULL; > - > -static av_cold void avisynth_atexit_handler(void); > > static av_cold int avisynth_load_library(void) > { > @@ -185,7 +174,6 @@ static av_cold int avisynth_load_library(void) > LOAD_AVS_FUNC(avs_get_env_property, 1); > #undef LOAD_AVS_FUNC > > - atexit(avisynth_atexit_handler); > return 0; > > fail: > @@ -214,30 +202,11 @@ static av_cold int avisynth_context_create(AVFormatContext *s) > } > } > > - if (!avs_ctx_list) { > - avs_ctx_list = avs; > - } else { > - avs->next = avs_ctx_list; > - avs_ctx_list = avs; > - } > - > return 0; > } > > static av_cold void avisynth_context_destroy(AviSynthContext *avs) > { > - if (avs_atexit_called) > - return; > - > - if (avs == avs_ctx_list) { > - avs_ctx_list = avs->next; > - } else { > - AviSynthContext *prev = avs_ctx_list; > - while (prev->next != avs) > - prev = prev->next; > - prev->next = avs->next; > - } > - > if (avs->clip) { > avs_library.avs_release_clip(avs->clip); > avs->clip = NULL; > @@ -248,20 +217,6 @@ static av_cold void avisynth_context_destroy(AviSynthContext *avs) > } > } > > -static av_cold void avisynth_atexit_handler(void) > -{ > - AviSynthContext *avs = avs_ctx_list; > - > - while (avs) { > - AviSynthContext *next = avs->next; > - avisynth_context_destroy(avs); > - avs = next; > - } > - dlclose(avs_library.library); > - > - avs_atexit_called = 1; > -} > - > /* Create AVStream from audio and video data. */ > static int avisynth_create_stream_video(AVFormatContext *s, AVStream *st) > { > @@ -1134,6 +1089,7 @@ static av_cold int avisynth_read_close(AVFormatContext *s) > return AVERROR_UNKNOWN; > > avisynth_context_destroy(s->priv_data); > + dlclose(avs_library.library); Maybe it's best to wrap this around an if (avs_library.library). [...] Besides that, the patch looks ok. Ramiro
On 7/28/24 9:38 AM, Ramiro Polla wrote: >> @@ -1134,6 +1089,7 @@ static av_cold int avisynth_read_close(AVFormatContext *s) >> return AVERROR_UNKNOWN; >> >> avisynth_context_destroy(s->priv_data); >> + dlclose(avs_library.library); > > Maybe it's best to wrap this around an if (avs_library.library). > True. I had tried toying around with C11 _Thread_local since we now use C17 as the default std, and in that case there was what I presume was a double free happening that required adding a check for whether avs_library.library still existed. As that hadn't been happening prior to that test I didn't really think much of it, but yeah, it would make sense to check it anyway. I abandoned that general idea after 1) finding out that while GCC and Clang are fine, MSVC doesn't seem to yet (or if it does, only the most recent versions of MSVC 2022 do) 2) C23 renamed it to thread_local, and most importantly, 3) I probably hadn't quite used it entirely correctly, because while the script could be parsed and played back just fine, trying to encode anything from it would segfault.
On 7/28/24 11:59 AM, Stephen Hutchinson wrote: > On 7/28/24 9:38 AM, Ramiro Polla wrote: >>> @@ -1134,6 +1089,7 @@ static av_cold int >>> avisynth_read_close(AVFormatContext *s) >>> return AVERROR_UNKNOWN; >>> >>> avisynth_context_destroy(s->priv_data); >>> + dlclose(avs_library.library); >> >> Maybe it's best to wrap this around an if (avs_library.library). >> > > True. I had tried toying around with C11 _Thread_local since we now > use C17 as the default std, and in that case there was what I presume > was a double free happening that required adding a check for whether > avs_library.library still existed. As that hadn't been happening prior > to that test I didn't really think much of it, but yeah, it would > make sense to check it anyway. > > I abandoned that general idea after 1) finding out that while GCC and > Clang are fine, MSVC doesn't seem to yet (or if it does, only the > most recent versions of MSVC 2022 do) 2) C23 renamed it to thread_local, > and most importantly, 3) I probably hadn't quite used it entirely > correctly, because while the script could be parsed and played back just > fine, trying to encode anything from it would segfault. > I had been ready to push the first two patches two weeks ago, but the addition of #s 3 and 4 delayed that so I could test a bit more. I didn't run into any issues with the two newer patches either, so I'll wait another couple days for any additional comments before pushing the whole set.
On 7/19/24 12:56 PM, Stephen Hutchinson wrote: > The atexit() handler in the avisynth demuxer was added because > there was a conflict in AvxSynth that arose due to their use > of C++ global objects, particularly in relation to having > added a logging function relying on log4cpp. > > This conflict was responsible for causing a segfault on exit. > It did not affect Windows with the (at the time) upstream > AviSynth 2.5 and 2.6, nor does it affect AviSynth+. > > Unfortunately, none of this was actually shielded by ifdefs > indicating the fact it was only needed for AvxSynth, so four > years ago when AviSynth+ replaced AvxSynth as the handler > for AviSynth scripts on Unix-like OSes, the fact that the > atexit handler was no longer necessary was overlooked. > > Signed-off-by: Stephen Hutchinson <qyot27@gmail.com> > --- > Changes compared to v1: > * Added missing dlclose invocation to read_close > > libavformat/avisynth.c | 46 +----------------------------------------- > 1 file changed, 1 insertion(+), 45 deletions(-) > > diff --git a/libavformat/avisynth.c b/libavformat/avisynth.c > index 625bdf7e3a..26747671c0 100644 > --- a/libavformat/avisynth.c > +++ b/libavformat/avisynth.c > @@ -115,9 +115,6 @@ typedef struct AviSynthContext { > int error; > > uint32_t flags; > - > - /* Linked list pointers. */ > - struct AviSynthContext *next; > } AviSynthContext; > > static const int avs_planes_packed[1] = { 0 }; > @@ -133,15 +130,7 @@ static const int avs_planes_rgba[4] = { AVS_PLANAR_G, AVS_PLANAR_B, > > static AVMutex avisynth_mutex = AV_MUTEX_INITIALIZER; > > -/* A conflict between C++ global objects, atexit, and dynamic loading requires > - * us to register our own atexit handler to prevent double freeing. */ > static AviSynthLibrary avs_library; > -static int avs_atexit_called = 0; > - > -/* Linked list of AviSynthContexts. An atexit handler destroys this list. */ > -static AviSynthContext *avs_ctx_list = NULL; > - > -static av_cold void avisynth_atexit_handler(void); > > static av_cold int avisynth_load_library(void) > { > @@ -185,7 +174,6 @@ static av_cold int avisynth_load_library(void) > LOAD_AVS_FUNC(avs_get_env_property, 1); > #undef LOAD_AVS_FUNC > > - atexit(avisynth_atexit_handler); > return 0; > > fail: > @@ -214,30 +202,11 @@ static av_cold int avisynth_context_create(AVFormatContext *s) > } > } > > - if (!avs_ctx_list) { > - avs_ctx_list = avs; > - } else { > - avs->next = avs_ctx_list; > - avs_ctx_list = avs; > - } > - > return 0; > } > > static av_cold void avisynth_context_destroy(AviSynthContext *avs) > { > - if (avs_atexit_called) > - return; > - > - if (avs == avs_ctx_list) { > - avs_ctx_list = avs->next; > - } else { > - AviSynthContext *prev = avs_ctx_list; > - while (prev->next != avs) > - prev = prev->next; > - prev->next = avs->next; > - } > - > if (avs->clip) { > avs_library.avs_release_clip(avs->clip); > avs->clip = NULL; > @@ -248,20 +217,6 @@ static av_cold void avisynth_context_destroy(AviSynthContext *avs) > } > } > > -static av_cold void avisynth_atexit_handler(void) > -{ > - AviSynthContext *avs = avs_ctx_list; > - > - while (avs) { > - AviSynthContext *next = avs->next; > - avisynth_context_destroy(avs); > - avs = next; > - } > - dlclose(avs_library.library); > - > - avs_atexit_called = 1; > -} > - > /* Create AVStream from audio and video data. */ > static int avisynth_create_stream_video(AVFormatContext *s, AVStream *st) > { > @@ -1134,6 +1089,7 @@ static av_cold int avisynth_read_close(AVFormatContext *s) > return AVERROR_UNKNOWN; > > avisynth_context_destroy(s->priv_data); > + dlclose(avs_library.library); > ff_mutex_unlock(&avisynth_mutex); > return 0; > } Pushed.
diff --git a/libavformat/avisynth.c b/libavformat/avisynth.c index 625bdf7e3a..26747671c0 100644 --- a/libavformat/avisynth.c +++ b/libavformat/avisynth.c @@ -115,9 +115,6 @@ typedef struct AviSynthContext { int error; uint32_t flags; - - /* Linked list pointers. */ - struct AviSynthContext *next; } AviSynthContext; static const int avs_planes_packed[1] = { 0 }; @@ -133,15 +130,7 @@ static const int avs_planes_rgba[4] = { AVS_PLANAR_G, AVS_PLANAR_B, static AVMutex avisynth_mutex = AV_MUTEX_INITIALIZER; -/* A conflict between C++ global objects, atexit, and dynamic loading requires - * us to register our own atexit handler to prevent double freeing. */ static AviSynthLibrary avs_library; -static int avs_atexit_called = 0; - -/* Linked list of AviSynthContexts. An atexit handler destroys this list. */ -static AviSynthContext *avs_ctx_list = NULL; - -static av_cold void avisynth_atexit_handler(void); static av_cold int avisynth_load_library(void) { @@ -185,7 +174,6 @@ static av_cold int avisynth_load_library(void) LOAD_AVS_FUNC(avs_get_env_property, 1); #undef LOAD_AVS_FUNC - atexit(avisynth_atexit_handler); return 0; fail: @@ -214,30 +202,11 @@ static av_cold int avisynth_context_create(AVFormatContext *s) } } - if (!avs_ctx_list) { - avs_ctx_list = avs; - } else { - avs->next = avs_ctx_list; - avs_ctx_list = avs; - } - return 0; } static av_cold void avisynth_context_destroy(AviSynthContext *avs) { - if (avs_atexit_called) - return; - - if (avs == avs_ctx_list) { - avs_ctx_list = avs->next; - } else { - AviSynthContext *prev = avs_ctx_list; - while (prev->next != avs) - prev = prev->next; - prev->next = avs->next; - } - if (avs->clip) { avs_library.avs_release_clip(avs->clip); avs->clip = NULL; @@ -248,20 +217,6 @@ static av_cold void avisynth_context_destroy(AviSynthContext *avs) } } -static av_cold void avisynth_atexit_handler(void) -{ - AviSynthContext *avs = avs_ctx_list; - - while (avs) { - AviSynthContext *next = avs->next; - avisynth_context_destroy(avs); - avs = next; - } - dlclose(avs_library.library); - - avs_atexit_called = 1; -} - /* Create AVStream from audio and video data. */ static int avisynth_create_stream_video(AVFormatContext *s, AVStream *st) { @@ -1134,6 +1089,7 @@ static av_cold int avisynth_read_close(AVFormatContext *s) return AVERROR_UNKNOWN; avisynth_context_destroy(s->priv_data); + dlclose(avs_library.library); ff_mutex_unlock(&avisynth_mutex); return 0; }
The atexit() handler in the avisynth demuxer was added because there was a conflict in AvxSynth that arose due to their use of C++ global objects, particularly in relation to having added a logging function relying on log4cpp. This conflict was responsible for causing a segfault on exit. It did not affect Windows with the (at the time) upstream AviSynth 2.5 and 2.6, nor does it affect AviSynth+. Unfortunately, none of this was actually shielded by ifdefs indicating the fact it was only needed for AvxSynth, so four years ago when AviSynth+ replaced AvxSynth as the handler for AviSynth scripts on Unix-like OSes, the fact that the atexit handler was no longer necessary was overlooked. Signed-off-by: Stephen Hutchinson <qyot27@gmail.com> --- Changes compared to v1: * Added missing dlclose invocation to read_close libavformat/avisynth.c | 46 +----------------------------------------- 1 file changed, 1 insertion(+), 45 deletions(-)