# HG changeset patch
# User Reinhard Resch <r_resch@a1.net>
# Date 1670081819 18000
# Node ID d9970470108ac02d2bbe36996ee072d99c5ab2cd
# Parent  ec1f340916359a33d584cf836f5707c64a9676df
Remove several race conditions with signal handler (bug #61370).

This patch removes several race conditions between the interpreter and the
Ctrl-C signal handler, as described in more detail in bug #61370.

* libinterp/corefcn/sighandlers.h: Make can_interrupt atomic.

* libinterp/corefcn/sighandlers.cc: Make can_interrupt, signals_caught atomic,
use atomic value read for signals_caught, octave_interrupt_state,
change 1 to true.

* liboctave/util/quit.h: Move octave_interrupt_state and octave_signal_caught
to C++ only code, rewrite octave_quit() function, rewrite OCTAVE_QUIT macro
to use octave_quit_c() function.

* liboctave/util/quit.cc: Make octave_interrupt_state and octave_signal_caught
atomic, add new wrapper function octave_quit_c().

* libinterp/corefcn/interpreter.h: Make octave_initialized atomic.

* libinterp/corefcn/interpreter.cc: Make octave_initialized atomic,
change 0 to false.

* liboctave/util/action-container.h: New namespace "util", make m_val atomic.

* liboctave/util/oct-atomic.c: Reorder preprocessor if-elif-else ladder.

* liboctave/util/cmd-edit.cc: Change 0 to false.

diff -r ec1f34091635 -r d9970470108a libinterp/corefcn/interpreter.cc
--- a/libinterp/corefcn/interpreter.cc	Sat Dec 03 13:55:14 2022 +0100
+++ b/libinterp/corefcn/interpreter.cc	Sat Dec 03 10:36:59 2022 -0500
@@ -91,7 +91,7 @@
 bool octave_interpreter_ready = false;
 
 // TRUE means we've processed all the init code and we are good to go.
-bool octave_initialized = false;
+std::atomic<bool> octave_initialized{false};
 
 OCTAVE_BEGIN_NAMESPACE(octave)
 
@@ -1950,7 +1950,7 @@
 
   can_interrupt = true;
   octave_interrupt_state = 0;
-  octave_signal_caught = 0;
+  octave_signal_caught = false;
   octave_restore_signal_mask ();
   catch_interrupts ();
 }
diff -r ec1f34091635 -r d9970470108a libinterp/corefcn/interpreter.h
--- a/libinterp/corefcn/interpreter.h	Sat Dec 03 13:55:14 2022 +0100
+++ b/libinterp/corefcn/interpreter.h	Sat Dec 03 10:36:59 2022 -0500
@@ -28,6 +28,7 @@
 
 #include "octave-config.h"
 
+#include <atomic>
 #include <map>
 #include <set>
 #include <stack>
@@ -66,7 +67,7 @@
 extern OCTINTERP_API bool octave_interpreter_ready;
 
 // TRUE means we've processed all the init code and we are good to go.
-extern OCTINTERP_API bool octave_initialized;
+extern OCTINTERP_API std::atomic<bool> octave_initialized;
 
 #include "oct-time.h"
 
diff -r ec1f34091635 -r d9970470108a libinterp/corefcn/sighandlers.cc
--- a/libinterp/corefcn/sighandlers.cc	Sat Dec 03 13:55:14 2022 +0100
+++ b/libinterp/corefcn/sighandlers.cc	Sat Dec 03 10:36:59 2022 -0500
@@ -27,6 +27,7 @@
 #  include "config.h"
 #endif
 
+#include <atomic>
 #include <csignal>
 #include <cstdlib>
 
@@ -65,7 +66,7 @@
 int pipe_handler_error_count = 0;
 
 // TRUE means we can be interrupted.
-bool can_interrupt = false;
+std::atomic<bool> can_interrupt{false};
 
 // TRUE means we should try to enter the debugger on SIGINT.
 bool Vdebug_on_interrupt = false;
@@ -82,7 +83,7 @@
 static bool Vsigterm_dumps_octave_core = true;
 
 // List of signals we have caught since last call to signal_handler.
-static bool *signals_caught = nullptr;
+static std::atomic<bool> *signals_caught = nullptr;
 
 static void
 my_friendly_exit (int sig, bool save_vars = true)
@@ -194,10 +195,10 @@
 
   for (int sig = 0; sig < octave_num_signals (); sig++)
     {
-      if (signals_caught[sig])
+      bool expected = true;
+
+      if (signals_caught[sig].compare_exchange_strong (expected, false))
         {
-          signals_caught[sig] = false;
-
           if ((have_sigchld && sig == sigchld)
               || (have_sigcld && sig == sigcld))
             {
@@ -223,9 +224,22 @@
               // FIXME: is this really needed?  Does it do anything
               // useful now?
 
-              if (pipe_handler_error_count++ > 100
-                  && octave_interrupt_state >= 0)
-                octave_interrupt_state++;
+              const int curr_pipe_handler_error_count = pipe_handler_error_count++;
+
+              if (curr_pipe_handler_error_count > 100)
+                {
+                  sig_atomic_t curr_interrupt_state
+                  = octave_interrupt_state.load();
+
+                  sig_atomic_t new_interrupt_state;
+
+                  do
+                    new_interrupt_state = curr_interrupt_state + 1;
+                  while (curr_interrupt_state >= 0 &&
+                     ! octave_interrupt_state.compare_exchange_weak
+                     (curr_interrupt_state, new_interrupt_state));
+                }
+
             }
           else if (have_sighup && sig == sighup)
             my_friendly_exit (sighup, Vsighup_dumps_octave_core);
@@ -276,7 +290,7 @@
   // signal watcher thread so it should probably be more careful about
   // how it accesses global objects.
 
-  octave_signal_caught = 1;
+  octave_signal_caught = true;
 
   signals_caught[sig] = true;
 
@@ -296,7 +310,7 @@
 
       if (can_interrupt)
         {
-          octave_signal_caught = 1;
+          octave_signal_caught = true;
           octave_interrupt_state++;
         }
     }
@@ -365,7 +379,7 @@
 install_signal_handlers (void)
 {
   if (! signals_caught)
-    signals_caught = new bool [octave_num_signals ()];
+    signals_caught = new std::atomic<bool> [octave_num_signals ()];
 
   for (int i = 0; i < octave_num_signals (); i++)
     signals_caught[i] = false;
diff -r ec1f34091635 -r d9970470108a libinterp/corefcn/sighandlers.h
--- a/libinterp/corefcn/sighandlers.h	Sat Dec 03 13:55:14 2022 +0100
+++ b/libinterp/corefcn/sighandlers.h	Sat Dec 03 10:36:59 2022 -0500
@@ -36,6 +36,8 @@
 #if ! defined (octave_sighandlers_h)
 #define octave_sighandlers_h 1
 
+#include <atomic>
+
 #include "octave-config.h"
 
 #include "child-list.h"
@@ -56,7 +58,7 @@
 extern int pipe_handler_error_count;
 
 // TRUE means we can be interrupted.
-extern OCTINTERP_API bool can_interrupt;
+extern OCTINTERP_API std::atomic<bool> can_interrupt;
 
 extern OCTINTERP_API sig_handler *
 set_signal_handler (int sig, sig_handler *h,
diff -r ec1f34091635 -r d9970470108a liboctave/util/action-container.h
--- a/liboctave/util/action-container.h	Sat Dec 03 13:55:14 2022 +0100
+++ b/liboctave/util/action-container.h	Sat Dec 03 10:36:59 2022 -0500
@@ -28,6 +28,7 @@
 
 #include "octave-config.h"
 
+#include <atomic>
 #include <cstddef>
 #include <functional>
 
@@ -39,6 +40,22 @@
 
 OCTAVE_BEGIN_NAMESPACE(octave)
 
+OCTAVE_BEGIN_NAMESPACE(util)
+
+template <typename T>
+struct atomic_traits
+{
+  typedef T type;
+};
+
+template <typename T>
+struct atomic_traits<std::atomic<T>>
+{
+  typedef T type;
+};
+
+OCTAVE_END_NAMESPACE(util)
+
 class
 action_container
 {
@@ -106,7 +123,8 @@
 
   private:
 
-    T *m_ptr, m_val;
+    T *m_ptr;
+    typename util::atomic_traits<T>::type m_val;
   };
 
   // Deletes a class allocated using new.
diff -r ec1f34091635 -r d9970470108a liboctave/util/cmd-edit.cc
--- a/liboctave/util/cmd-edit.cc	Sat Dec 03 13:55:14 2022 +0100
+++ b/liboctave/util/cmd-edit.cc	Sat Dec 03 10:36:59 2022 -0500
@@ -785,7 +785,7 @@
 void
 gnu_readline::do_handle_interrupt_signal (void)
 {
-  octave_signal_caught = 0;
+  octave_signal_caught = false;
   octave_interrupt_state = 0;
 
   ::octave_rl_recover_from_interrupt ();
diff -r ec1f34091635 -r d9970470108a liboctave/util/oct-atomic.c
--- a/liboctave/util/oct-atomic.c	Sat Dec 03 13:55:14 2022 +0100
+++ b/liboctave/util/oct-atomic.c	Sat Dec 03 10:36:59 2022 -0500
@@ -29,9 +29,23 @@
 
 #include "oct-atomic.h"
 
+#if defined (__GNUC__)
+
+octave_idx_type
+octave_atomic_increment (octave_idx_type *x)
+{
+  return __sync_add_and_fetch (x,  1);
+}
+
+octave_idx_type
+octave_atomic_decrement (octave_idx_type *x)
+{
+  return __sync_sub_and_fetch (x, 1);
+}
+
 /* Some versions of GCC can't compile stdatomic.h with -fopenmp.  */
 
-#if defined (OCTAVE_STDATOMIC_H_OK)
+#elif defined (OCTAVE_STDATOMIC_H_OK)
 #  include <stdatomic.h>
 
 octave_idx_type
@@ -52,20 +66,6 @@
   return *x;
 }
 
-#elif defined (__GNUC__)
-
-octave_idx_type
-octave_atomic_increment (octave_idx_type *x)
-{
-  return __sync_add_and_fetch (x,  1);
-}
-
-octave_idx_type
-octave_atomic_decrement (octave_idx_type *x)
-{
-  return __sync_sub_and_fetch (x, 1);
-}
-
 #elif defined (_MSC_VER)
 #  include <intrin.h>
 
diff -r ec1f34091635 -r d9970470108a liboctave/util/quit.cc
--- a/liboctave/util/quit.cc	Sat Dec 03 13:55:14 2022 +0100
+++ b/liboctave/util/quit.cc	Sat Dec 03 10:36:59 2022 -0500
@@ -27,6 +27,7 @@
 #  include "config.h"
 #endif
 
+#include <atomic>
 #include <cstring>
 
 #include <ostream>
@@ -35,9 +36,9 @@
 
 #include "quit.h"
 
-sig_atomic_t octave_interrupt_state = 0;
+std::atomic<sig_atomic_t> octave_interrupt_state{0};
 
-volatile sig_atomic_t octave_signal_caught = 0;
+volatile std::atomic<bool> octave_signal_caught{false};
 
 void (*octave_signal_hook) (void) = nullptr;
 void (*octave_interrupt_hook) (void) = nullptr;
@@ -95,6 +96,11 @@
     }
 }
 
+void octave_quit_c (void)
+{
+  octave_quit ();
+}
+
 OCTAVE_END_NAMESPACE(octave)
 
 void
@@ -103,10 +109,12 @@
   if (octave_signal_hook)
     octave_signal_hook ();
 
-  if (octave_interrupt_state > 0)
-    {
-      octave_interrupt_state = -1;
+  sig_atomic_t curr_interrupt_state = octave_interrupt_state.load ();
 
-      throw octave::interrupt_exception ();
-    }
+  while (curr_interrupt_state > 0 &&
+    ! octave_interrupt_state.compare_exchange_weak (curr_interrupt_state, -1))
+    ;
+
+  if (curr_interrupt_state > 0)
+    throw octave::interrupt_exception ();
 }
diff -r ec1f34091635 -r d9970470108a liboctave/util/quit.h
--- a/liboctave/util/quit.h	Sat Dec 03 13:55:14 2022 +0100
+++ b/liboctave/util/quit.h	Sat Dec 03 10:36:59 2022 -0500
@@ -30,6 +30,7 @@
 
 /* The signal header is just needed for the sig_atomic_t type.  */
 #if defined (__cplusplus)
+#  include <atomic>
 #  include <csignal>
 #  include <iosfwd>
 #  include <list>
@@ -223,9 +224,14 @@
     0: no interrupt pending
   < 0: handling interrupt
 */
-extern OCTAVE_API sig_atomic_t octave_interrupt_state;
+
+#if defined (__cplusplus)
 
-extern OCTAVE_API volatile sig_atomic_t octave_signal_caught;
+extern OCTAVE_API std::atomic<int> octave_interrupt_state;
+
+extern OCTAVE_API volatile std::atomic<bool> octave_signal_caught;
+
+#endif
 
 extern OCTAVE_API void octave_handle_signal (void);
 
@@ -233,27 +239,19 @@
 
 inline void octave_quit (void)
 {
-  if (octave_signal_caught)
-    {
-      octave_signal_caught = 0;
-      octave_handle_signal ();
-    }
+  bool expected = true;
+
+  if (octave_signal_caught.compare_exchange_strong (expected, false))
+    octave_handle_signal ();
 }
 
 #define OCTAVE_QUIT octave_quit ()
 
 #else
 
-#define OCTAVE_QUIT                             \
-  do                                            \
-    {                                           \
-      if (octave_signal_caught)                 \
-        {                                       \
-          octave_signal_caught = 0;             \
-          octave_handle_signal ();              \
-        }                                       \
-    }                                           \
-  while (0)
+extern OCTAVE_API void octave_quit_c (void);
+#define OCTAVE_QUIT octave_quit_c ()
+
 #endif
 
 /* The following macros are obsolete.  Interrupting immediately by