[Bug c++/99613] New: Static variable destruction order race condition

[Bug c++/99613] New: Static variable destruction order race condition

[michalz at nvidia dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

            Bug ID: 99613
           Summary: Static variable destruction order race condition
           Product: gcc
           Version: 7.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: michalz at nvidia dot com
  Target Milestone: ---

Created attachment 50394
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50394&action=edit
Race condition demo

Function static variables should be destroyed in the reverse order of
initialization. However, the lock is released before calling atexit to register
the destructor, allowing for a race condition to occur if multiple threads are
accessing static variables for the first time.

The bug affects all versions of GCC that I've tried (6.4, 7.3, 8.2, 9.3, 10.2)
on multiple platforms.

The error in output assembly can be seen when compiling the following code:

struct S{
   S();
   ~S();
};

S& foo(){
    static S s;
    return s;
}

The attached file contains a working example that triggers the race condition
(it's a race condition, so sometimes it succeeds).

[Bug c++/99613] Static variable destruction order race condition

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
      Known to fail|                            |10.2.1
           Keywords|                            |wrong-code
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-03-16

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.

;; Function S& foo() (_Z3foov)
;; enabled by -tree-original


{
  static struct S s;

    static struct S s;
  if (<<cleanup_point __atomic_load_1 (&_ZGVZ3foovE1s, 2) == 0>>)
    {
      if (<<cleanup_point __cxa_guard_acquire (&_ZGVZ3foovE1s) != 0>>)
        {
          <<cleanup_point <<< Unknown tree: expr_stmt
  TARGET_EXPR <D.2361, 0>;, S::S (&s);;, D.2361 = 1;;, __cxa_guard_release
(&_ZGVZ3foovE1s);;, __cxa_atexit ((void (*<Tf1>) (void *)) __dt_comp , (void *)
&s, (void *) &__dso_handle); >>>>>;
        }
    }
  return <retval> = (struct S &) &s;
}

[Bug c++/99613] Static variable destruction order race condition

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I don't see a problem with that.
__cxa_guard_acquire should succeed just once unless the constructor throws (but
if it throws, then __cxa_atexit won't be called by that thread where it throwed
and so will be called by some other one that runs the constructor afterwards.

[Bug c++/99613] Static variable destruction order race condition

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |redi at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Or do you mean it is possible that for two unrelated variables
variable 1 with its guard          variable 2 with its guard
__cxa_guard_acquire succeeds
ctor is called
                                   __cxa_guard_acquire succeeds
                                   ctor is called
                                   __cxa_guard_release is called
                                   __cxa_atexit is called
__cxa_guard_release is called
__cxa_atexit is called
?
I don't see how swapping __cxa_atexit and __cxa_guard_release would help,
__cxa_guard_acquire at least on most targets isn't a global synchronization
primitive that would serialize all local constructors, and making it
serializing would be very expensive.

[Bug c++/99613] Static variable destruction order race condition

[michalz at nvidia dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #4 from Michal Zientkiewicz <michalz at nvidia dot com> ---
The problem is that the order of destruction is incorrect if there's a race
condition.
Consider 2 threads initializing static variables S1 and S2:

Thread A             Thread B

acquire
construct S1
release
                     acquire
                     construct S2
                     release
                     atexit destroy S2
atexit destroy S1

Construction order:
S1 S2

Destruction order:
S1 S2

Should be
S2 S1


The demo program triggers that behavior (not always, of course, but frequently
enough to see it fail in say, 10 runs).

[Bug c++/99613] Static variable destruction order race condition

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The variables can be constructed even exactly at the same time, or at least the
ctors can be started concurrently and end concurrently.  I don't think you can
rely on a particular ordering of the destructors in that case.

[Bug c++/99613] Static variable destruction order race condition

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #3)
> Or do you mean it is possible that for two unrelated variables
> variable 1 with its guard          variable 2 with its guard
> __cxa_guard_acquire succeeds
> ctor is called
>                                    __cxa_guard_acquire succeeds
>                                    ctor is called
>                                    __cxa_guard_release is called
>                                    __cxa_atexit is called
> __cxa_guard_release is called
> __cxa_atexit is called
> ?
> I don't see how swapping __cxa_atexit and __cxa_guard_release would help,
> __cxa_guard_acquire at least on most targets isn't a global synchronization
> primitive that would serialize all local constructors, and making it
> serializing would be very expensive.

I think other threads running into an active init of another wait anyway
(the init can fail), so they are in fact already serializing?  Moving
the cxa_atexit inside the guard would fix the race then.

[Bug c++/99613] Static variable destruction order race condition

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Swapping __cxa_guard_release with __cxa_atexit would "fix" the case where the
user program would in all threads access all the local variables in the same
order.  So all threads first access f<0>(), then f<1>(), etc.  Even in that
case right now it can happen that say thread0 wins the __cxa_guard_acquire for
f<0>(), calls constructor, releases guard, then sleeps, and thread1 sees f<0>()
is already initialized, wins __cxa_guard_acquire for f<1>(), calls constructor,
releases guard, calls __cxa_atexit, then thread0 wakes up and calls
__cxa_atexit.
But, unless I'm misreading the testcase, that is not what that test is doing,
there each thread calls just one of the f<N>() functions, so I don't see how
the
swapping would help there.  Only using a global lock for all the local statics
would fix that, and I think there is no way we are willing to do that (Jon,
please correct me if I'm wrong).

[Bug c++/99613] Static variable destruction order race condition

[michalz at nvidia dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #8 from Michal Zientkiewicz <michalz at nvidia dot com> ---
Jakub: You read coorectly, I was checking for global construction/destruction
order of many variables. I agree that a global lock is a heavy-handed solution
- and likely the only one that would always guarantee destruction order.

The example I posted is not very useful - the variables are independent. There
are cases where moving atexit would help:

Thread A                   Thread B
acquire lock on S1
initialize S1
release lock on S1

                           acquire lock on S2
                           initialize S2, which depends on S1
                           S1 is seen as initialized -> proceed
                           release lock on S2
                           atexit destroy S2
atexit destroy S1

Now we have reversed destruction order on _dependent_ variables and moving
atexit inside the per-variable lock scope would fix the issue.

[Bug c++/99613] Static variable destruction order race condition

[michalz at nvidia dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #9 from Michal Zientkiewicz <michalz at nvidia dot com> ---
(My previous comment may seem unclear, so let me clarify):
The _demo_ is not very useful, but the example in the example from the previous
comment is.

[Bug c++/99613] Static variable destruction order race condition

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Even the swapping of the two calls would be IMHO a significant slowdown.
Because __cxa_atexit under the hood holds a global lock (fortunately not across
the duration of the whole user ctor, but across the internal bookkeeping it
needs to do).

[Bug c++/99613] Static variable destruction order race condition

[michalz at nvidia dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #11 from Michal Zientkiewicz <michalz at nvidia dot com> ---
Created attachment 50396
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50396&action=edit
Demo with dependent variables

I added a new demo which shows that dependent variable gets destroyed after its
dependencies.
When running, I recommend to launch `stress` for all CPU cores in a separate
terminal - this gives me near 100% reproduction rate.

[Bug c++/99613] Static variable destruction order race condition

[michalz at nvidia dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #12 from Michal Zientkiewicz <michalz at nvidia dot com> ---
As a side note - Clang emits the call to atexit between acquire and release and
the last demo works fine when compiled with Clang, but not GCC.

[Bug c++/99613] Static variable destruction order race condition

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I guess before changing anything it would be good if the C++ people could
discuss this perhaps in the committee, what really wants the standard to
ensure, whether the most expensive case of total order of the local static
ctors, or just make this case work when the ctors depend on other ctors, or no
requirements at all.

[Bug c++/99613] Static variable destruction order race condition

[michalz at nvidia dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #14 from Michal Zientkiewicz <michalz at nvidia dot com> ---
https://eel.is/c++draft/basic.start.term#3

If the completion of the constructor or dynamic initialization of an object
with static storage duration strongly happens before that of another, the
completion of the destructor of the second is sequenced before the initiation
of the destructor of the first.
If the completion of the constructor or dynamic initialization of an object
with thread storage duration is sequenced before that of another, the
completion of the destructor of the second is sequenced before the initiation
of the destructor of the first.
If an object is initialized statically, the object is destroyed in the same
order as if the object was dynamically initialized.
For an object of array or class type, all subobjects of that object are
destroyed before any block variable with static storage duration initialized
during the construction of the subobjects is destroyed.
If the destruction of an object with static or thread storage duration exits
via an exception, the function std​::​terminate is called ([except.terminate]).

I think that makes at least the dependent variable case a requirement.

[Bug c++/99613] Static variable destruction order race condition

[jacobhemstad at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

Jake Hemstad <jacobhemstad at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jacobhemstad at gmail dot com

--- Comment #15 from Jake Hemstad <jacobhemstad at gmail dot com> ---
https://itanium-cxx-abi.github.io/cxx-abi/abi.html#once-ctor

Likewise, note that the Itanium ABI has an example of how `__cxa_atexit` and
`__cxa_guard_release` should be ordered and it is consistent with the order
Clang produces.

[Bug c++/99613] Static variable destruction order race condition

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 50399
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50399&action=edit
gcc11-pr99613.patch

Untested patch that swaps the two calls.

[Bug c++/99613] Static variable destruction order race condition

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #17 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think for the non-dependent case there's no good fix but the standard can be
read in a way that only the dependent case has well-defined order of
destruction.

[Bug c++/99613] Static variable destruction order race condition

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #18 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:0251051db64f13c9a31a05c8133c31dc50b2b235

commit r11-7696-g0251051db64f13c9a31a05c8133c31dc50b2b235
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Mar 16 21:17:44 2021 +0100

    c++: Ensure correct destruction order of local statics [PR99613]

    As mentioned in the PR, if end of two constructions of local statics
    is strongly ordered, their destructors should be run in the reverse order.
    As we run __cxa_guard_release before calling __cxa_atexit, it is possible
    that we have two threads that access two local statics in the same order
    for the first time, one thread wins the __cxa_guard_acquire on the first
    one but is rescheduled in between the __cxa_guard_release and __cxa_atexit
    calls, then the other thread is scheduled and wins __cxa_guard_acquire
    on the second one and calls __cxa_quard_release and __cxa_atexit and only
    afterwards the first thread calls its __cxa_atexit.  This means a variable
    whose completion of the constructor strongly happened after the completion
    of the other one will be destructed after the other variable is destructed.

    The following patch fixes that by swapping the __cxa_guard_release and
    __cxa_atexit calls.

    2021-03-16  Jakub Jelinek  <jakub@redhat.com>

            PR c++/99613
            * decl.c (expand_static_init): For thread guards, call __cxa_atexit
            before calling __cxa_guard_release rather than after it. 
Formatting
            fixes.

[Bug c++/99613] Static variable destruction order race condition

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #19 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:1703937a05b8b95bc29d2de292387dfd9eb7c9a3

commit r10-9487-g1703937a05b8b95bc29d2de292387dfd9eb7c9a3
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Mar 16 21:17:44 2021 +0100

    c++: Ensure correct destruction order of local statics [PR99613]

    As mentioned in the PR, if end of two constructions of local statics
    is strongly ordered, their destructors should be run in the reverse order.
    As we run __cxa_guard_release before calling __cxa_atexit, it is possible
    that we have two threads that access two local statics in the same order
    for the first time, one thread wins the __cxa_guard_acquire on the first
    one but is rescheduled in between the __cxa_guard_release and __cxa_atexit
    calls, then the other thread is scheduled and wins __cxa_guard_acquire
    on the second one and calls __cxa_quard_release and __cxa_atexit and only
    afterwards the first thread calls its __cxa_atexit.  This means a variable
    whose completion of the constructor strongly happened after the completion
    of the other one will be destructed after the other variable is destructed.

    The following patch fixes that by swapping the __cxa_guard_release and
    __cxa_atexit calls.

    2021-03-16  Jakub Jelinek  <jakub@redhat.com>

            PR c++/99613
            * decl.c (expand_static_init): For thread guards, call __cxa_atexit
            before calling __cxa_guard_release rather than after it. 
Formatting
            fixes.

    (cherry picked from commit 0251051db64f13c9a31a05c8133c31dc50b2b235)

[Bug c++/99613] Static variable destruction order race condition

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #20 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:cb6efb9d909323e41fa0a8abbe805eb32d1659ea

commit r9-9432-gcb6efb9d909323e41fa0a8abbe805eb32d1659ea
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Mar 16 21:17:44 2021 +0100

    c++: Ensure correct destruction order of local statics [PR99613]

    As mentioned in the PR, if end of two constructions of local statics
    is strongly ordered, their destructors should be run in the reverse order.
    As we run __cxa_guard_release before calling __cxa_atexit, it is possible
    that we have two threads that access two local statics in the same order
    for the first time, one thread wins the __cxa_guard_acquire on the first
    one but is rescheduled in between the __cxa_guard_release and __cxa_atexit
    calls, then the other thread is scheduled and wins __cxa_guard_acquire
    on the second one and calls __cxa_quard_release and __cxa_atexit and only
    afterwards the first thread calls its __cxa_atexit.  This means a variable
    whose completion of the constructor strongly happened after the completion
    of the other one will be destructed after the other variable is destructed.

    The following patch fixes that by swapping the __cxa_guard_release and
    __cxa_atexit calls.

    2021-03-16  Jakub Jelinek  <jakub@redhat.com>

            PR c++/99613
            * decl.c (expand_static_init): For thread guards, call __cxa_atexit
            before calling __cxa_guard_release rather than after it. 
Formatting
            fixes.

    (cherry picked from commit 1703937a05b8b95bc29d2de292387dfd9eb7c9a3)

[Bug c++/99613] Static variable destruction order race condition

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

--- Comment #21 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-8 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:bd2a59b7b3f049d81566163056ae1206d4fa3e6d

commit r8-10896-gbd2a59b7b3f049d81566163056ae1206d4fa3e6d
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Mar 16 21:17:44 2021 +0100

    c++: Ensure correct destruction order of local statics [PR99613]

    As mentioned in the PR, if end of two constructions of local statics
    is strongly ordered, their destructors should be run in the reverse order.
    As we run __cxa_guard_release before calling __cxa_atexit, it is possible
    that we have two threads that access two local statics in the same order
    for the first time, one thread wins the __cxa_guard_acquire on the first
    one but is rescheduled in between the __cxa_guard_release and __cxa_atexit
    calls, then the other thread is scheduled and wins __cxa_guard_acquire
    on the second one and calls __cxa_quard_release and __cxa_atexit and only
    afterwards the first thread calls its __cxa_atexit.  This means a variable
    whose completion of the constructor strongly happened after the completion
    of the other one will be destructed after the other variable is destructed.

    The following patch fixes that by swapping the __cxa_guard_release and
    __cxa_atexit calls.

    2021-03-16  Jakub Jelinek  <jakub@redhat.com>

            PR c++/99613
            * decl.c (expand_static_init): For thread guards, call __cxa_atexit
            before calling __cxa_guard_release rather than after it. 
Formatting
            fixes.

    (cherry picked from commit 1703937a05b8b95bc29d2de292387dfd9eb7c9a3)

[Bug c++/99613] Static variable destruction order race condition

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
         Resolution|---                         |FIXED

--- Comment #22 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.

[Bug c++/99613] Static variable destruction order race condition

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99613

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |8.5