ELL double free during debug exit cleanup

ELL double free during debug exit cleanup

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

Hi Guys,

I am copying this over to the ELL reflector, hoping for some insight. The new bluez-5.52 release has a unit
test segfaulting in a test I added.  This only occurs on Link Time Optimization (LTO) builds, but that appears
to be the favored build flavor of openSUSE.

The segfault occurs in the ELL code during exit, specifically due to a double call of:
> l_queue_destroy(debug_sections, l_free);

Here is a bug report, that disects the problem, and suggests an ELL patch (scroll down in the bug report to
"Comment 6":

https://bugzilla.opensuse.org/show_bug.cgi?id=1155889#c6

I would like your opinion before creating a patch to submit.

Regards,
Brian Gix

Re: ELL double free during debug exit cleanup

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

For what it's worth:

I created an x86_64 openSUSE tumbleweed VM, and GIT repos of the master tip of both ELL and BLUEZ.

This appears to build and run correctly using that configuration.
The *reported* failure is using the BLUEZ tarball of 5.52, and sure enough if I do the *exact instructions* in
the bug report:
	> tar xf bluez-5.52.tar.xz
	> cd bluez-5.52
	> CFLAGS="-flto=auto" ./configure --enable-mesh
	> make check

I can reproduce the problem on openSUSE only (it fails much earlier, in unrelated areas if I try this on
Fedora-30).

A working patch to solve this problem looks like this (using l_* for new global symbols):

diff --git a/ell/log.c b/ell/log.c
index f3f4b0c..a73aac1 100644
--- a/ell/log.c
+++ b/ell/log.c
@@ -441,7 +441,7 @@ LIB_EXPORT void l_debug_disable(void)
 	debug_pattern = NULL;
 }
 
-__attribute__((constructor)) static void register_debug_section()
+__attribute__((constructor)) void l_register_debug_section()
 {
 	extern struct l_debug_desc __start___ell_debug[];
 	extern struct l_debug_desc __stop___ell_debug[];
@@ -449,7 +449,7 @@ __attribute__((constructor)) static void register_debug_section()
 	l_debug_add_section(__start___ell_debug, __stop___ell_debug);
 }
 
-__attribute__((destructor(65535))) static void free_debug_sections()
+__attribute__((destructor(65535))) void l_free_debug_sections()
 {
 	l_queue_destroy(debug_sections, l_free);
 }
diff --git a/ell/log.h b/ell/log.h
index 19bf10b..04b913a 100644
--- a/ell/log.h
+++ b/ell/log.h
@@ -72,6 +72,8 @@ struct l_debug_desc {
 					__func__ , ##__VA_ARGS__); \
 } while (0)
 
+void l_register_debug_section(void);
+void l_free_debug_sections(void);
 void l_debug_enable_full(const char *pattern,
 				struct l_debug_desc *start,
 				struct l_debug_desc *stop);


On Tue, 2019-11-05 at 16:22 +0000, Gix, Brian wrote:
> Hi Guys,
> 
> I am copying this over to the ELL reflector, hoping for some insight. The new bluez-5.52 release has a unit
> test segfaulting in a test I added.  This only occurs on Link Time Optimization (LTO) builds, but that
> appears
> to be the favored build flavor of openSUSE.
> 
> The segfault occurs in the ELL code during exit, specifically due to a double call of:
> > l_queue_destroy(debug_sections, l_free);
> 
> Here is a bug report, that disects the problem, and suggests an ELL patch (scroll down in the bug report to
> "Comment 6":
> 
> https://bugzilla.opensuse.org/show_bug.cgi?id=1155889#c6
> 
> I would like your opinion before creating a patch to submit.
> 
> Regards,
> Brian Gix
> _______________________________________________
> ell mailing list -- ell(a)lists.01.org
> To unsubscribe send an email to ell-leave(a)lists.01.org

Re: ELL double free during debug exit cleanup

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 3475 bytes --]

Self-comment...

On Tue, 2019-11-05 at 20:23 +0000, Gix, Brian wrote:
> For what it's worth:
> 
> I created an x86_64 openSUSE tumbleweed VM, and GIT repos of the master tip of both ELL and BLUEZ.
> 
> This appears to build and run correctly using that configuration.
> The *reported* failure is using the BLUEZ tarball of 5.52, and sure enough if I do the *exact instructions*
> in
> the bug report:
> 	> tar xf bluez-5.52.tar.xz
> 	> cd bluez-5.52
> 	> CFLAGS="-flto=auto" ./configure --enable-mesh
> 	> make check
> 
> I can reproduce the problem on openSUSE only (it fails much earlier, in unrelated areas if I try this on
> Fedora-30).
> 
> A working patch to solve this problem looks like this (using l_* for new global symbols):
> 
> diff --git a/ell/log.c b/ell/log.c
> index f3f4b0c..a73aac1 100644
> --- a/ell/log.c
> +++ b/ell/log.c
> @@ -441,7 +441,7 @@ LIB_EXPORT void l_debug_disable(void)
>  	debug_pattern = NULL;
>  }
>  
> -__attribute__((constructor)) static void register_debug_section()
> +__attribute__((constructor)) void l_register_debug_section()
>  {
>  	extern struct l_debug_desc __start___ell_debug[];
>  	extern struct l_debug_desc __stop___ell_debug[];
> @@ -449,7 +449,7 @@ __attribute__((constructor)) static void register_debug_section()
>  	l_debug_add_section(__start___ell_debug, __stop___ell_debug);
>  }
>  
> -__attribute__((destructor(65535))) static void free_debug_sections()
> +__attribute__((destructor(65535))) void l_free_debug_sections()
>  {
>  	l_queue_destroy(debug_sections, l_free);
>  }
> diff --git a/ell/log.h b/ell/log.h
> index 19bf10b..04b913a 100644
> --- a/ell/log.h
> +++ b/ell/log.h
> @@ -72,6 +72,8 @@ struct l_debug_desc {
>  					__func__ , ##__VA_ARGS__); \
>  } while (0)
>  
> +void l_register_debug_section(void);
> +void l_free_debug_sections(void);

Since this probably *shouldn't* be an externally visible function pair, the forward prototype definitions
probably will belong in log.c, and not here.

But I still await Denis' & Marcel's judgement on the overall strategy... or whether this was something that I
absolutely just messed up in the mesh crypto unit test, over in BLUEZ.

>  void l_debug_enable_full(const char *pattern,
>  				struct l_debug_desc *start,
>  				struct l_debug_desc *stop);
> 
> 
> On Tue, 2019-11-05 at 16:22 +0000, Gix, Brian wrote:
> > Hi Guys,
> > 
> > I am copying this over to the ELL reflector, hoping for some insight. The new bluez-5.52 release has a unit
> > test segfaulting in a test I added.  This only occurs on Link Time Optimization (LTO) builds, but that
> > appears
> > to be the favored build flavor of openSUSE.
> > 
> > The segfault occurs in the ELL code during exit, specifically due to a double call of:
> > > l_queue_destroy(debug_sections, l_free);
> > 
> > Here is a bug report, that disects the problem, and suggests an ELL patch (scroll down in the bug report to
> > "Comment 6":
> > 
> > https://bugzilla.opensuse.org/show_bug.cgi?id=1155889#c6
> > 
> > I would like your opinion before creating a patch to submit.
> > 
> > Regards,
> > Brian Gix
> > _______________________________________________
> > ell mailing list -- ell(a)lists.01.org
> > To unsubscribe send an email to ell-leave(a)lists.01.org
> _______________________________________________
> ell mailing list -- ell(a)lists.01.org
> To unsubscribe send an email to ell-leave(a)lists.01.org

Re: ELL double free during debug exit cleanup

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

Hi Brian,

>> -__attribute__((constructor)) static void register_debug_section()
>> +__attribute__((constructor)) void l_register_debug_section()

So really, this would be about the last solution I'd pick, no matter 
where the forward declaration lives.

Have you looked into why the library/or this file is being statically 
linked twice?

Regards,
-Denis

Re: ELL double free during debug exit cleanup

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 962 bytes --]

Hi Denis,

On Tue, 2019-11-05 at 14:48 -0600, Denis Kenzior wrote:
> Hi Brian,
> 
> > > -__attribute__((constructor)) static void register_debug_section()
> > > +__attribute__((constructor)) void l_register_debug_section()
> 
> So really, this would be about the last solution I'd pick, no matter 
> where the forward declaration lives.
> 
> Have you looked into why the library/or this file is being statically 
> linked twice?

I have but haven't found anything I can put my finger on yet.  I have been trying to figure out what make Link
Time Optimization different, and of course I do have my suspicions that *I* must have done something wrong in
the BLUEZ Unit test, since I can't reproduce the issue in any of the ELL unit tests.

I am continuing my research focussing on how BLUEZ is building it's (one) ELL based unit test. This last email
was mostly about confirming the observations of the bug reporter.  
> 
> Regards,
> -Denis

Re: ELL double free during debug exit cleanup

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

Hi Denis and others,

On Tue, 2019-11-05 at 21:31 +0000, Gix, Brian wrote:
> Hi Denis,
> 
> On Tue, 2019-11-05 at 14:48 -0600, Denis Kenzior wrote:
> > Hi Brian,
> > 
> > > > -__attribute__((constructor)) static void register_debug_section()
> > > > +__attribute__((constructor)) void l_register_debug_section()
> > 
> > So really, this would be about the last solution I'd pick, no matter 
> > where the forward declaration lives.
> > 
> > Have you looked into why the library/or this file is being statically 
> > linked twice?

After disecting what the BLUEZ Makefile was doing, I did find the inclusion of both log.c and ell/libell-
internal.la in the unit test that was failing.  Thankfully, I was also able to confirm that the issue was
limited to the one test, and now have a template to include other ELL based unit tests.

I apologize for the alarm, but will now at least be including openSUSE and LTO in my validation passes. 

Thank you so much for your time, Denis.

> Regards,
> > -Denis
> _______________________________________________
> ell mailing list -- ell(a)lists.01.org
> To unsubscribe send an email to ell-leave(a)lists.01.org