| Message ID | 20210821144145.3077223-1-vedantparanjape160201@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Vedant, Thank you for the patch. Looks like you've found an answer to the question you asked on my review of v1 :-) On Sat, Aug 21, 2021 at 08:11:45PM +0530, Vedant Paranjape wrote: > ASan needs to be loaded first before gstreamer is loaded. This was not > possible, so verify_asan_link_order was disabled. Better way to tackle > this issue was disabling forks on the gstreamer side. > > verify_asan_link_order=0 disables the check on ASan side which checks if > ASan was loaded before any other DLLs. Since, gstreamer spawns a child s/DLLs/shared objects/ as we're on Linux, no Windows :-) > helper process while building the registry, we needed to disable this > check. But with gst_registry_fork_set_enabled() it is possible to > disable spawning this child helper process, so this ensures that ASan is > loaded before any other DLL is loaded. > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > --- > test/gstreamer/gstreamer_single_stream_test.cpp | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-) > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > index 349dcfa4..80e652eb 100644 > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > @@ -41,18 +41,13 @@ protected: The comment starts with "GStreamer by spawns a process ...". Now that we know this can be changed with gst_registry_fork_set_enabled(), I think this should be changed to "GStreamer by default spawns a process ...". > * GStreamer is most likely not, this will cause the ASan link > * order check to fail when gst-plugin-scanner dlopen()s the > * plugin as many libraries will have already been loaded by > - * then. Work around this issue by disabling the link order > - * check. This will only affect child processes, as ASan is > - * already loaded for this process by the time this code is > - * executed, and should thus hopefully be safe. > - * > - * This option is not available in gcc older than 8, the only > - * option in that case is to skip the test. > + * then. Work around this issue by disabling spawning of a > + * child helper process when rebuilding the registry. This will How about "when scanning the build directory for plugins" instead of "when rebuilding the registry", as the problems occurs when gst_registry_scan_path() is called ? > + * only affect child processes, as ASan is already loaded for > + * this process by the time this code is executed, and should > + * thus hopefully be safe. I think the last sentence can be dropped. I wrote it to document that verify_asan_link_order=0 didn't disable the check for the current process, which isn't a concern anymore as we don't disable the check now. If you're fine with those changes, I can update the patch when pushing, there's no need to send a v3. > */ > -#if defined(__SANITIZE_ADDRESS__) && !defined(__clang__) && __GNUC__ < 8 > - return TestSkip; > -#endif > - setenv("ASAN_OPTIONS", "verify_asan_link_order=0", 1); > + gst_registry_fork_set_enabled(false); > > /* Initialize GStreamer */ > g_autoptr(GError) errInit = NULL;
Hi Laurent, Thanks for the feedback. On Sun, 22 Aug, 2021, 06:37 Laurent Pinchart, < laurent.pinchart@ideasonboard.com> wrote: > Hi Vedant, > > Thank you for the patch. > > Looks like you've found an answer to the question you asked on my review > of v1 :-) > > On Sat, Aug 21, 2021 at 08:11:45PM +0530, Vedant Paranjape wrote: > > ASan needs to be loaded first before gstreamer is loaded. This was not > > possible, so verify_asan_link_order was disabled. Better way to tackle > > this issue was disabling forks on the gstreamer side. > > > > verify_asan_link_order=0 disables the check on ASan side which checks if > > ASan was loaded before any other DLLs. Since, gstreamer spawns a child > > s/DLLs/shared objects/ as we're on Linux, no Windows :-) > Oops 😅 > helper process while building the registry, we needed to disable this > > check. But with gst_registry_fork_set_enabled() it is possible to > > disable spawning this child helper process, so this ensures that ASan is > > loaded before any other DLL is loaded. > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > --- > > test/gstreamer/gstreamer_single_stream_test.cpp | 17 ++++++----------- > > 1 file changed, 6 insertions(+), 11 deletions(-) > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp > b/test/gstreamer/gstreamer_single_stream_test.cpp > > index 349dcfa4..80e652eb 100644 > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > @@ -41,18 +41,13 @@ protected: > > The comment starts with "GStreamer by spawns a process ...". Now that we > know this can be changed with gst_registry_fork_set_enabled(), I think > this should be changed to "GStreamer by default spawns a process ...". > > > * GStreamer is most likely not, this will cause the ASan > link > > * order check to fail when gst-plugin-scanner dlopen()s > the > > * plugin as many libraries will have already been loaded > by > > - * then. Work around this issue by disabling the link order > > - * check. This will only affect child processes, as ASan is > > - * already loaded for this process by the time this code is > > - * executed, and should thus hopefully be safe. > > - * > > - * This option is not available in gcc older than 8, the > only > > - * option in that case is to skip the test. > > + * then. Work around this issue by disabling spawning of a > > + * child helper process when rebuilding the registry. This > will > > How about "when scanning the build directory for plugins" instead of > "when rebuilding the registry", as the problems occurs when > gst_registry_scan_path() is called ? > > > + * only affect child processes, as ASan is already loaded > for > > + * this process by the time this code is executed, and > should > > + * thus hopefully be safe. > > I think the last sentence can be dropped. I wrote it to document that > verify_asan_link_order=0 didn't disable the check for the current > process, which isn't a concern anymore as we don't disable the check > now. > > If you're fine with those changes, I can update the patch when pushing, > there's no need to send a v3. > Yes I am mostly fine with the changes. > */ > > -#if defined(__SANITIZE_ADDRESS__) && !defined(__clang__) && __GNUC__ < 8 > > - return TestSkip; > > -#endif > > - setenv("ASAN_OPTIONS", "verify_asan_link_order=0", 1); > > + gst_registry_fork_set_enabled(false); > > > > /* Initialize GStreamer */ > > g_autoptr(GError) errInit = NULL; > > -- > Regards, > > Laurent Pinchart > Regards Vedant Paranjape
diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp index 349dcfa4..80e652eb 100644 --- a/test/gstreamer/gstreamer_single_stream_test.cpp +++ b/test/gstreamer/gstreamer_single_stream_test.cpp @@ -41,18 +41,13 @@ protected: * GStreamer is most likely not, this will cause the ASan link * order check to fail when gst-plugin-scanner dlopen()s the * plugin as many libraries will have already been loaded by - * then. Work around this issue by disabling the link order - * check. This will only affect child processes, as ASan is - * already loaded for this process by the time this code is - * executed, and should thus hopefully be safe. - * - * This option is not available in gcc older than 8, the only - * option in that case is to skip the test. + * then. Work around this issue by disabling spawning of a + * child helper process when rebuilding the registry. This will + * only affect child processes, as ASan is already loaded for + * this process by the time this code is executed, and should + * thus hopefully be safe. */ -#if defined(__SANITIZE_ADDRESS__) && !defined(__clang__) && __GNUC__ < 8 - return TestSkip; -#endif - setenv("ASAN_OPTIONS", "verify_asan_link_order=0", 1); + gst_registry_fork_set_enabled(false); /* Initialize GStreamer */ g_autoptr(GError) errInit = NULL;
ASan needs to be loaded first before gstreamer is loaded. This was not possible, so verify_asan_link_order was disabled. Better way to tackle this issue was disabling forks on the gstreamer side. verify_asan_link_order=0 disables the check on ASan side which checks if ASan was loaded before any other DLLs. Since, gstreamer spawns a child helper process while building the registry, we needed to disable this check. But with gst_registry_fork_set_enabled() it is possible to disable spawning this child helper process, so this ensures that ASan is loaded before any other DLL is loaded. Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> --- test/gstreamer/gstreamer_single_stream_test.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)