Fix use of singleton in optinfo framework

Message ID 1586216686-16474-2-git-send-email-gromero@linux.ibm.com
State Superseded
Headers show
Series
  • Fix use of singleton in optinfo framework
Related show

Commit Message

Patrick Palka via Gcc-patches April 6, 2020, 11:44 p.m.
Currently an use of get() method of dump_context singleton in optinfo
framework causes a new class to be instantiated, which calls the singleton
dtor when the class is destroyed, freeing memory that is referenced after
free() is called, generating an ICE error.

This commit fixes the issue by using singleton's static member get()
directly to get the singleton's active instance, which doesn't instantiate
a new class, so no dtor is called.

gcc/Changelog:
2020-04-06  Gustavo Romero  <gromero@linux.ibm.com>

	* dumpfile.c:
	(selftest::temp_dump_context::temp_dump_context): Fix ctor.
---
 gcc/dumpfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.7.4

Comments

Patrick Palka via Gcc-patches April 7, 2020, 1:42 a.m. | #1
On Mon, 2020-04-06 at 19:44 -0400, Gustavo Romero wrote:

Thanks for this patch.

The patch looks correct, but I'm not sure that the description of the
problem is exact in every detail.  I think you've run into a bug in
code I wrote; sorry.

> Currently an use of get() method of dump_context singleton in optinfo

> framework causes a new class to be instantiated, 


I agree, and that's a bug.

> which calls the singleton

> dtor when the class is destroyed, freeing memory that is referenced

> after

> free() is called, generating an ICE error.


I introduced dump_context when adding the opt_info machinery in order
to make it more amenable to unit-testing via the selftest framework.

If I'm reading the code correctly, dump_context has no ctor, but
instead relies on the fields being zero-initialized in the BSS segment.
It has a dtor, which deletes the m_pending field.

It looks like the initializer of m_context in temp_dump_context's ctor
uses the implicitly-defined default constructor for dump_context, and
this leaves the fields uninitialized; in particular m_pending.

I *think* what's going on is that the temporary dump_context that gets
created in the "m_saved" initializer is uninitialized, and when its
dtor runs it deletes the m_pending, thus calling delete on
uninitialized memory - whatever happens to be in the stack.  I don't
see a complaint about this under valgrind though.

Sorry about this.  IIRC I was trying to avoid having a ctor run before
main.  If I'm reading things right there's still a dormant bug here
even with your patch.

> 

> This commit fixes the issue by using singleton's static member get()

> directly to get the singleton's active instance, which doesn't

> instantiate

> a new class, so no dtor is called.


I agree.

> gcc/Changelog:

> 2020-04-06  Gustavo Romero  <gromero@linux.ibm.com>

> 

> 	* dumpfile.c:

> 	(selftest::temp_dump_context::temp_dump_context): Fix ctor.

> ---

>  gcc/dumpfile.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c

> index 468ffab..d0d8aa7 100644

> --- a/gcc/dumpfile.c

> +++ b/gcc/dumpfile.c

> @@ -2076,7 +2076,7 @@ temp_dump_context::temp_dump_context (bool

> forcibly_enable_optinfo,

>  				      bool forcibly_enable_dumping,

>  				      dump_flags_t test_pp_flags)

>  : m_context (),

> -  m_saved (&dump_context ().get ())

> +  m_saved(&dump_context::get())


Whitespace nit: keep the space between m_saved and the open-paren.

>  {

>    dump_context::s_current = &m_context;

>    if (forcibly_enable_optinfo)
Patrick Palka via Gcc-patches April 7, 2020, 1:46 a.m. | #2
On Mon, 2020-04-06 at 21:42 -0400, David Malcolm via Gcc-patches wrote:
> On Mon, 2020-04-06 at 19:44 -0400, Gustavo Romero wrote:

> 

> Thanks for this patch.

> 

> The patch looks correct, but I'm not sure that the description of the

> problem is exact in every detail.  I think you've run into a bug in

> code I wrote; sorry.

> 

> > Currently an use of get() method of dump_context singleton in

> > optinfo

> > framework causes a new class to be instantiated, 

> 

> I agree, and that's a bug.

> 

> > which calls the singleton

> > dtor when the class is destroyed, freeing memory that is referenced

> > after

> > free() is called, generating an ICE error.

> 

> I introduced dump_context when adding the opt_info machinery in order

> to make it more amenable to unit-testing via the selftest framework.

> 

> If I'm reading the code correctly, dump_context has no ctor, but

> instead relies on the fields being zero-initialized in the BSS

> segment.

> It has a dtor, which deletes the m_pending field.

> 

> It looks like the initializer of m_context in temp_dump_context's

> ctor

> uses the implicitly-defined default constructor for dump_context, and

> this leaves the fields uninitialized; in particular m_pending.

> 

> I *think* what's going on is that the temporary dump_context that

> gets

> created in the "m_saved" initializer is uninitialized, and when its

> dtor runs it deletes the m_pending, thus calling delete on

> uninitialized memory - whatever happens to be in the stack.


or rather: "using delete with an uninitialized optinfo *"

>   I don't

> see a complaint about this under valgrind though.

> 

> Sorry about this.  IIRC I was trying to avoid having a ctor run

> before

> main.  If I'm reading things right there's still a dormant bug here

> even with your patch.

> 

> > This commit fixes the issue by using singleton's static member

> > get()

> > directly to get the singleton's active instance, which doesn't

> > instantiate

> > a new class, so no dtor is called.

> 

> I agree.

> 

> > gcc/Changelog:

> > 2020-04-06  Gustavo Romero  <gromero@linux.ibm.com>

> > 

> > 	* dumpfile.c:

> > 	(selftest::temp_dump_context::temp_dump_context): Fix ctor.

> > ---

> >  gcc/dumpfile.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c

> > index 468ffab..d0d8aa7 100644

> > --- a/gcc/dumpfile.c

> > +++ b/gcc/dumpfile.c

> > @@ -2076,7 +2076,7 @@ temp_dump_context::temp_dump_context (bool

> > forcibly_enable_optinfo,

> >  				      bool forcibly_enable_dumping,

> >  				      dump_flags_t test_pp_flags)

> >  : m_context (),

> > -  m_saved (&dump_context ().get ())

> > +  m_saved(&dump_context::get())

> 

> Whitespace nit: keep the space between m_saved and the open-paren.

> 

> >  {

> >    dump_context::s_current = &m_context;

> >    if (forcibly_enable_optinfo)
Patrick Palka via Gcc-patches April 7, 2020, 6:24 a.m. | #3
On Mon, Apr 06, 2020 at 09:42:17PM -0400, David Malcolm via Gcc-patches wrote:
> > 2020-04-06  Gustavo Romero  <gromero@linux.ibm.com>

> > 

> > 	* dumpfile.c:

> > 	(selftest::temp_dump_context::temp_dump_context): Fix ctor.

> > ---

> >  gcc/dumpfile.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c

> > index 468ffab..d0d8aa7 100644

> > --- a/gcc/dumpfile.c

> > +++ b/gcc/dumpfile.c

> > @@ -2076,7 +2076,7 @@ temp_dump_context::temp_dump_context (bool

> > forcibly_enable_optinfo,

> >  				      bool forcibly_enable_dumping,

> >  				      dump_flags_t test_pp_flags)

> >  : m_context (),

> > -  m_saved (&dump_context ().get ())

> > +  m_saved(&dump_context::get())

> 

> Whitespace nit: keep the space between m_saved and the open-paren.


And between get and open paren too.

	Jakub
Patrick Palka via Gcc-patches April 8, 2020, 3 a.m. | #4
Hi David,

On 04/06/2020 10:42 PM, David Malcolm wrote:
> On Mon, 2020-04-06 at 19:44 -0400, Gustavo Romero wrote:

> 

> Thanks for this patch.

> 

> The patch looks correct, but I'm not sure that the description of the

> problem is exact in every detail.  I think you've run into a bug in

> code I wrote; sorry.


Thanks a lot for your quick reply and review.


>> which calls the singleton

>> dtor when the class is destroyed, freeing memory that is referenced

>> after

>> free() is called, generating an ICE error.

> 

> I introduced dump_context when adding the opt_info machinery in order

> to make it more amenable to unit-testing via the selftest framework.

> 

> If I'm reading the code correctly, dump_context has no ctor, but

> instead relies on the fields being zero-initialized in the BSS segment.

> It has a dtor, which deletes the m_pending field.

> 

> It looks like the initializer of m_context in temp_dump_context's ctor

> uses the implicitly-defined default constructor for dump_context, and

> this leaves the fields uninitialized; in particular m_pending.

> 

> I *think* what's going on is that the temporary dump_context that gets

> created in the "m_saved" initializer is uninitialized, and when its

> dtor runs it deletes the m_pending, thus calling delete on

> uninitialized memory - whatever happens to be in the stack.  I don't

> see a complaint about this under valgrind though.


Yeah I think that's correct. On my wording in the commit message it says
as if the dereference of corrupted data happens after the dtor finishes,
which is wrong. Ultimately iirc even optinfo dtor can be the culprit but
as you said it might depend upon what's in the uninitialized data.
I'll fix it in v2. I also said base gcc used was 4.7, but it's actually
4.2.1, so I'll fix it for the records too. Finally, it seems that
this patch also helps 64-bit builds, accordingly to Piotr (CC:ed).


> Sorry about this.  IIRC I was trying to avoid having a ctor run before

> main.  If I'm reading things right there's still a dormant bug here

> even with your patch.


Got it. OK, I still need to spend some time on FreeBSD not building
GCC9 on 64-bit (due to other reasons), so I'll keep an eye on it and
report back if I find something worth.


Kind regards,
Gustavo
Patrick Palka via Gcc-patches April 8, 2020, 3:03 a.m. | #5
On 04/07/2020 03:24 AM, Jakub Jelinek wrote:
> On Mon, Apr 06, 2020 at 09:42:17PM -0400, David Malcolm via Gcc-patches wrote:

>>> 2020-04-06  Gustavo Romero  <gromero@linux.ibm.com>

>>>

>>> 	* dumpfile.c:

>>> 	(selftest::temp_dump_context::temp_dump_context): Fix ctor.

>>> ---

>>>   gcc/dumpfile.c | 2 +-

>>>   1 file changed, 1 insertion(+), 1 deletion(-)

>>>

>>> diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c

>>> index 468ffab..d0d8aa7 100644

>>> --- a/gcc/dumpfile.c

>>> +++ b/gcc/dumpfile.c

>>> @@ -2076,7 +2076,7 @@ temp_dump_context::temp_dump_context (bool

>>> forcibly_enable_optinfo,

>>>   				      bool forcibly_enable_dumping,

>>>   				      dump_flags_t test_pp_flags)

>>>   : m_context (),

>>> -  m_saved (&dump_context ().get ())

>>> +  m_saved(&dump_context::get())

>>

>> Whitespace nit: keep the space between m_saved and the open-paren.

> 

> And between get and open paren too.


Thanks, Jakub.
Patrick Palka via Gcc-patches April 8, 2020, 8:52 a.m. | #6
Hi,

since there has been some misunderstanding, I will explain.

There are 4 possible options here:
1. FreeBSD 12.1-RELEASE, powerpc, GCC 4.2
2. FreeBSD 13.0-CURRENT (head branch), powerpc, LLVM 10.0.0
3. FreeBSD 12.1-RELEASE, powerpc64, GCC 4.2
4. FreeBSD 13.0-CURRENT (head branch), powerpc64, LLVM 10.0.0

The patch that Gustavo provided fixes completely build issue for 1.

For 2. and 4., there's another issue with namespace collision between
LLVM and GCC, we use our own non-upstreamable patch. The ICE reported
here doesn't happen.

For 3., we currently build GCC9 using GCC8 as a bootstrap compiler and
it works fine (ICE doesn't happen). However, I'd like to get rid of the
bootstrap compiler and just build with base GCC 4.2. That's when the ICE
happens. There are also other issues, all related to GCC persistently
setting -O2, with which GCC 4.2 and stage 1 binaries compiled with -O2
just segfault. They work fine with -O0, however.

So this patch helps with building on both 32 and 64 bits using old GCC
4.2.

On 20-04-08 00:00:37, Gustavo Romero wrote:
>Hi David,

>

>On 04/06/2020 10:42 PM, David Malcolm wrote:

>> On Mon, 2020-04-06 at 19:44 -0400, Gustavo Romero wrote:

>>

>> Thanks for this patch.

>>

>> The patch looks correct, but I'm not sure that the description of the

>> problem is exact in every detail.  I think you've run into a bug in

>> code I wrote; sorry.

>

>Thanks a lot for your quick reply and review.

>

>

>>> which calls the singleton

>>> dtor when the class is destroyed, freeing memory that is referenced

>>> after

>>> free() is called, generating an ICE error.

>>

>> I introduced dump_context when adding the opt_info machinery in order

>> to make it more amenable to unit-testing via the selftest framework.

>>

>> If I'm reading the code correctly, dump_context has no ctor, but

>> instead relies on the fields being zero-initialized in the BSS segment.

>> It has a dtor, which deletes the m_pending field.

>>

>> It looks like the initializer of m_context in temp_dump_context's ctor

>> uses the implicitly-defined default constructor for dump_context, and

>> this leaves the fields uninitialized; in particular m_pending.

>>

>> I *think* what's going on is that the temporary dump_context that gets

>> created in the "m_saved" initializer is uninitialized, and when its

>> dtor runs it deletes the m_pending, thus calling delete on

>> uninitialized memory - whatever happens to be in the stack.  I don't

>> see a complaint about this under valgrind though.

>

>Yeah I think that's correct. On my wording in the commit message it says

>as if the dereference of corrupted data happens after the dtor finishes,

>which is wrong. Ultimately iirc even optinfo dtor can be the culprit but

>as you said it might depend upon what's in the uninitialized data.

>I'll fix it in v2. I also said base gcc used was 4.7, but it's actually

>4.2.1, so I'll fix it for the records too. Finally, it seems that

>this patch also helps 64-bit builds, accordingly to Piotr (CC:ed).

>

>

>> Sorry about this.  IIRC I was trying to avoid having a ctor run before

>> main.  If I'm reading things right there's still a dormant bug here

>> even with your patch.

>

>Got it. OK, I still need to spend some time on FreeBSD not building

>GCC9 on 64-bit (due to other reasons), so I'll keep an eye on it and

>report back if I find something worth.

>

>

>Kind regards,

>Gustavo
Hans-Peter Nilsson April 14, 2020, 3:33 a.m. | #7
On Mon, 6 Apr 2020, Gustavo Romero via Gcc-patches wrote:

> Currently an use of get() method of dump_context singleton in optinfo

> framework causes a new class to be instantiated, which calls the singleton

> dtor when the class is destroyed, freeing memory that is referenced after

> free() is called, generating an ICE error.

>

> This commit fixes the issue by using singleton's static member get()

> directly to get the singleton's active instance, which doesn't instantiate

> a new class, so no dtor is called.

>

> gcc/Changelog:

> 2020-04-06  Gustavo Romero  <gromero@linux.ibm.com>

>

> 	* dumpfile.c:

> 	(selftest::temp_dump_context::temp_dump_context): Fix ctor.


Better formatted ChangeLogentry; removing ":" (and breaking at
column 70 seems better than after with leaving a single word on
the next line):

 	* dumpfile.c (selftest::temp_dump_context::temp_dump_context):
	Fix ctor.

Sadly this patch doesn't fix PR bootstrap/87252; I just checked
against f8e72b8d9f2:f81653ba8c1:2dd4ceacd8b (truncated from
LAST_UPDATED; I don't remember which field is the actual
commit).

brgds, H-P
Patrick Palka via Gcc-patches April 21, 2020, 9:04 p.m. | #8
Hi Hans,

On 4/14/20 12:33 AM, Hans-Peter Nilsson wrote:
> Sadly this patch doesn't fix PR bootstrap/87252; I just checked

> against f8e72b8d9f2:f81653ba8c1:2dd4ceacd8b (truncated from

> LAST_UPDATED; I don't remember which field is the actual

> commit).


So, would you mind to tell me precisely how to reproduce nowadays
that bug? Which gcc version and from which distro/OS/32 or 64-bit
you're using to reproduce it? Given the stacktrace you share on
PR87252 I'm not really surprise my patch didn't help.

If I can reproduce it easily I can try to drill it further.


Thanks,
Gustavo
Hans-Peter Nilsson April 21, 2020, 9:54 p.m. | #9
On Tue, 21 Apr 2020, Gustavo Romero wrote:

> Hi Hans,


Hi Gus,

> On 4/14/20 12:33 AM, Hans-Peter Nilsson wrote:

> > Sadly this patch doesn't fix PR bootstrap/87252; I just checked

> > against f8e72b8d9f2:f81653ba8c1:2dd4ceacd8b (truncated from

> > LAST_UPDATED; I don't remember which field is the actual

> > commit).

>

> So, would you mind to tell me precisely how to reproduce nowadays

> that bug? Which gcc version and from which distro/OS/32 or 64-bit

> you're using to reproduce it? Given the stacktrace you share on

> PR87252 I'm not really surprise my patch didn't help.


I just added that detail; "Fedora 12", to the ticket.
Thanks for looking into it.

brgds, H-P

Patch

diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
index 468ffab..d0d8aa7 100644
--- a/gcc/dumpfile.c
+++ b/gcc/dumpfile.c
@@ -2076,7 +2076,7 @@  temp_dump_context::temp_dump_context (bool forcibly_enable_optinfo,
 				      bool forcibly_enable_dumping,
 				      dump_flags_t test_pp_flags)
 : m_context (),
-  m_saved (&dump_context ().get ())
+  m_saved(&dump_context::get())
 {
   dump_context::s_current = &m_context;
   if (forcibly_enable_optinfo)