Fix gdb.interrupt race

  gdb.interrupt was introduced to implement DAP request cancellation.
However, because it can be run from another thread, and because I
didn't look deeply enough at the implementation, it turns out to be
racy.

The fix here is to lock accesses to certain globals in extension.c.

Note that this won't work in the case where configure detects that the
C++ compiler doesn't provide thread support.  In this patch I chose to
simply let this race, but another choice would be to disable DAP
entirely in this situation.

Regression tested on x86-64 Fedora 38.  I also ran gdb.dap/pause.exp
in a thread-sanitizer build tree to make sure the reported race is
gone.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31263
---
 gdb/extension.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
  

On 2/23/24 17:20, Tom Tromey wrote:
> gdb.interrupt was introduced to implement DAP request cancellation.
> However, because it can be run from another thread, and because I
> didn't look deeply enough at the implementation, it turns out to be
> racy.
> 
> The fix here is to lock accesses to certain globals in extension.c.
> 

I've tested this on x86_64-linux, and see the failing runs go from 2/10 
to 0/10.

> Note that this won't work in the case where configure detects that the
> C++ compiler doesn't provide thread support.  In this patch I chose to
> simply let this race, but another choice would be to disable DAP
> entirely in this situation.
> 

I vote disable.  If somebody has such a platform and wants DAP, they can 
submit a patch that enables it for them in a way that is safe.

Anyway, since the patch documents the choice explicitly, this LGTM as is.

Approved-By: Tom de Vries <tdevries@suse.de>
Tested-By: Tom de Vries <tdevries@suse.de>

Thanks,
- Tom


> Regression tested on x86-64 Fedora 38.  I also ran gdb.dap/pause.exp
> in a thread-sanitizer build tree to make sure the reported race is
> gone.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31263
> ---
>   gdb/extension.c | 39 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
> 
> diff --git a/gdb/extension.c b/gdb/extension.c
> index 42e05199d2c..3a77d355a78 100644
> --- a/gdb/extension.c
> +++ b/gdb/extension.c
> @@ -634,6 +634,21 @@ breakpoint_ext_lang_cond_says_stop (struct breakpoint *b)
>      This requires cooperation with the extension languages so the support
>      is defined here.  */
>   
> +#if CXX_STD_THREAD
> +
> +#include <mutex>
> +
> +/* DAP needs a way to interrupt the main thread, so we added
> +   gdb.interrupt.  However, as this can run from any thread, we need
> +   locking for the current extension language.  If threading is not
> +   available, we simply let this race.
> +
> +   This lock is held for accesses to quit_flag, active_ext_lang, and
> +   cooperative_sigint_handling_disabled.  */
> +static std::recursive_mutex ext_lang_mutex;
> +
> +#endif /* CXX_STD_THREAD */
> +
>   /* This flag tracks quit requests when we haven't called out to an
>      extension language.  it also holds quit requests when we transition to
>      an extension language that doesn't have cooperative SIGINT handling.  */
> @@ -689,6 +704,10 @@ static bool cooperative_sigint_handling_disabled = false;
>   
>   scoped_disable_cooperative_sigint_handling::scoped_disable_cooperative_sigint_handling ()
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>     /* Force the active extension language to the GDB scripting
>        language.  This ensures that a previously saved SIGINT is moved
>        to the quit_flag global, as well as ensures that future SIGINTs
> @@ -706,6 +725,10 @@ scoped_disable_cooperative_sigint_handling::scoped_disable_cooperative_sigint_ha
>   
>   scoped_disable_cooperative_sigint_handling::~scoped_disable_cooperative_sigint_handling ()
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>     cooperative_sigint_handling_disabled = m_prev_cooperative_sigint_handling_disabled;
>     restore_active_ext_lang (m_prev_active_ext_lang_state);
>   }
> @@ -744,6 +767,10 @@ scoped_disable_cooperative_sigint_handling::~scoped_disable_cooperative_sigint_h
>   struct active_ext_lang_state *
>   set_active_ext_lang (const struct extension_language_defn *now_active)
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>   #if GDB_SELF_TEST
>     if (selftests::hook_set_active_ext_lang)
>       selftests::hook_set_active_ext_lang ();
> @@ -796,6 +823,10 @@ set_active_ext_lang (const struct extension_language_defn *now_active)
>   void
>   restore_active_ext_lang (struct active_ext_lang_state *previous)
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>     if (cooperative_sigint_handling_disabled)
>       {
>         /* See set_active_ext_lang.  */
> @@ -832,6 +863,10 @@ restore_active_ext_lang (struct active_ext_lang_state *previous)
>   void
>   set_quit_flag (void)
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>     if (active_ext_lang->ops != NULL
>         && active_ext_lang->ops->set_quit_flag != NULL)
>       active_ext_lang->ops->set_quit_flag (active_ext_lang);
> @@ -856,6 +891,10 @@ set_quit_flag (void)
>   int
>   check_quit_flag (void)
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>     int result = 0;
>   
>     for (const struct extension_language_defn *extlang : extension_languages)
  

Tom> I vote disable.  If somebody has such a platform and wants DAP, they
Tom> can submit a patch that enables it for them in a way that is safe.

Easily done, see the appended.

I think this is really only an issue for mingw with certain versions of
GCC -- it's fixed in FSF GCC now.

Let me know what you think.

Tom

commit eb5e215bfb810b9c8771462562365b2fae18afe6
Author: Tom Tromey <tromey@adacore.com>
Date:   Fri Feb 23 08:59:40 2024 -0700

    Fix gdb.interrupt race
    
    gdb.interrupt was introduced to implement DAP request cancellation.
    However, because it can be run from another thread, and because I
    didn't look deeply enough at the implementation, it turns out to be
    racy.
    
    The fix here is to lock accesses to certain globals in extension.c.
    
    Note that this won't work in the case where configure detects that the
    C++ compiler doesn't provide thread support.  This version of the
    patch disables DAP entirely in this situation.
    
    Regression tested on x86-64 Fedora 38.  I also ran gdb.dap/pause.exp
    in a thread-sanitizer build tree to make sure the reported race is
    gone.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31263

diff --git a/gdb/extension.c b/gdb/extension.c
index 9f403500497..b420350926d 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -646,6 +646,21 @@ breakpoint_ext_lang_cond_says_stop (struct breakpoint *b)
    This requires cooperation with the extension languages so the support
    is defined here.  */
 
+#if CXX_STD_THREAD
+
+#include <mutex>
+
+/* DAP needs a way to interrupt the main thread, so we added
+   gdb.interrupt.  However, as this can run from any thread, we need
+   locking for the current extension language.  If threading is not
+   available, we simply let this race.
+
+   This lock is held for accesses to quit_flag, active_ext_lang, and
+   cooperative_sigint_handling_disabled.  */
+static std::recursive_mutex ext_lang_mutex;
+
+#endif /* CXX_STD_THREAD */
+
 /* This flag tracks quit requests when we haven't called out to an
    extension language.  it also holds quit requests when we transition to
    an extension language that doesn't have cooperative SIGINT handling.  */
@@ -701,6 +716,10 @@ static bool cooperative_sigint_handling_disabled = false;
 
 scoped_disable_cooperative_sigint_handling::scoped_disable_cooperative_sigint_handling ()
 {
+#if CXX_STD_THREAD
+  std::lock_guard guard (ext_lang_mutex);
+#endif /* CXX_STD_THREAD */
+
   /* Force the active extension language to the GDB scripting
      language.  This ensures that a previously saved SIGINT is moved
      to the quit_flag global, as well as ensures that future SIGINTs
@@ -718,6 +737,10 @@ scoped_disable_cooperative_sigint_handling::scoped_disable_cooperative_sigint_ha
 
 scoped_disable_cooperative_sigint_handling::~scoped_disable_cooperative_sigint_handling ()
 {
+#if CXX_STD_THREAD
+  std::lock_guard guard (ext_lang_mutex);
+#endif /* CXX_STD_THREAD */
+
   cooperative_sigint_handling_disabled = m_prev_cooperative_sigint_handling_disabled;
   restore_active_ext_lang (m_prev_active_ext_lang_state);
 }
@@ -756,6 +779,10 @@ scoped_disable_cooperative_sigint_handling::~scoped_disable_cooperative_sigint_h
 struct active_ext_lang_state *
 set_active_ext_lang (const struct extension_language_defn *now_active)
 {
+#if CXX_STD_THREAD
+  std::lock_guard guard (ext_lang_mutex);
+#endif /* CXX_STD_THREAD */
+
 #if GDB_SELF_TEST
   if (selftests::hook_set_active_ext_lang)
     selftests::hook_set_active_ext_lang ();
@@ -808,6 +835,10 @@ set_active_ext_lang (const struct extension_language_defn *now_active)
 void
 restore_active_ext_lang (struct active_ext_lang_state *previous)
 {
+#if CXX_STD_THREAD
+  std::lock_guard guard (ext_lang_mutex);
+#endif /* CXX_STD_THREAD */
+
   if (cooperative_sigint_handling_disabled)
     {
       /* See set_active_ext_lang.  */
@@ -844,6 +875,10 @@ restore_active_ext_lang (struct active_ext_lang_state *previous)
 void
 set_quit_flag (void)
 {
+#if CXX_STD_THREAD
+  std::lock_guard guard (ext_lang_mutex);
+#endif /* CXX_STD_THREAD */
+
   if (active_ext_lang->ops != NULL
       && active_ext_lang->ops->set_quit_flag != NULL)
     active_ext_lang->ops->set_quit_flag (active_ext_lang);
@@ -868,6 +903,10 @@ set_quit_flag (void)
 int
 check_quit_flag (void)
 {
+#if CXX_STD_THREAD
+  std::lock_guard guard (ext_lang_mutex);
+#endif /* CXX_STD_THREAD */
+
   int result = 0;
 
   for (const struct extension_language_defn *extlang : extension_languages)
diff --git a/gdb/python/py-dap.c b/gdb/python/py-dap.c
index 2034105c939..9a00130fe90 100644
--- a/gdb/python/py-dap.c
+++ b/gdb/python/py-dap.c
@@ -92,10 +92,14 @@ call_dap_fn (const char *fn_name)
 void
 dap_interp::init (bool top_level)
 {
+#if CXX_STD_THREAD
   call_dap_fn ("run");
 
   current_ui->input_fd = -1;
   current_ui->m_input_interactive_p = false;
+#else
+  error (_("GDB was compiled without threading, which DAP requires"));
+#endif
 }
 
 void
  

On 2/27/24 19:37, Tom Tromey wrote:
> Tom> I vote disable.  If somebody has such a platform and wants DAP, they
> Tom> can submit a patch that enables it for them in a way that is safe.
> 
> Easily done, see the appended.
> 
> I think this is really only an issue for mingw with certain versions of
> GCC -- it's fixed in FSF GCC now.
> 
> Let me know what you think.
> 

LGTM, just the "let this race" comment needs updating.

Thanks,
- Tom

> Tom
> 
> commit eb5e215bfb810b9c8771462562365b2fae18afe6
> Author: Tom Tromey <tromey@adacore.com>
> Date:   Fri Feb 23 08:59:40 2024 -0700
> 
>      Fix gdb.interrupt race
>      
>      gdb.interrupt was introduced to implement DAP request cancellation.
>      However, because it can be run from another thread, and because I
>      didn't look deeply enough at the implementation, it turns out to be
>      racy.
>      
>      The fix here is to lock accesses to certain globals in extension.c.
>      
>      Note that this won't work in the case where configure detects that the
>      C++ compiler doesn't provide thread support.  This version of the
>      patch disables DAP entirely in this situation.
>      
>      Regression tested on x86-64 Fedora 38.  I also ran gdb.dap/pause.exp
>      in a thread-sanitizer build tree to make sure the reported race is
>      gone.
>      
>      Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31263
> 
> diff --git a/gdb/extension.c b/gdb/extension.c
> index 9f403500497..b420350926d 100644
> --- a/gdb/extension.c
> +++ b/gdb/extension.c
> @@ -646,6 +646,21 @@ breakpoint_ext_lang_cond_says_stop (struct breakpoint *b)
>      This requires cooperation with the extension languages so the support
>      is defined here.  */
>   
> +#if CXX_STD_THREAD
> +
> +#include <mutex>
> +
> +/* DAP needs a way to interrupt the main thread, so we added
> +   gdb.interrupt.  However, as this can run from any thread, we need
> +   locking for the current extension language.  If threading is not
> +   available, we simply let this race.
> +
> +   This lock is held for accesses to quit_flag, active_ext_lang, and
> +   cooperative_sigint_handling_disabled.  */
> +static std::recursive_mutex ext_lang_mutex;
> +
> +#endif /* CXX_STD_THREAD */
> +
>   /* This flag tracks quit requests when we haven't called out to an
>      extension language.  it also holds quit requests when we transition to
>      an extension language that doesn't have cooperative SIGINT handling.  */
> @@ -701,6 +716,10 @@ static bool cooperative_sigint_handling_disabled = false;
>   
>   scoped_disable_cooperative_sigint_handling::scoped_disable_cooperative_sigint_handling ()
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>     /* Force the active extension language to the GDB scripting
>        language.  This ensures that a previously saved SIGINT is moved
>        to the quit_flag global, as well as ensures that future SIGINTs
> @@ -718,6 +737,10 @@ scoped_disable_cooperative_sigint_handling::scoped_disable_cooperative_sigint_ha
>   
>   scoped_disable_cooperative_sigint_handling::~scoped_disable_cooperative_sigint_handling ()
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>     cooperative_sigint_handling_disabled = m_prev_cooperative_sigint_handling_disabled;
>     restore_active_ext_lang (m_prev_active_ext_lang_state);
>   }
> @@ -756,6 +779,10 @@ scoped_disable_cooperative_sigint_handling::~scoped_disable_cooperative_sigint_h
>   struct active_ext_lang_state *
>   set_active_ext_lang (const struct extension_language_defn *now_active)
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>   #if GDB_SELF_TEST
>     if (selftests::hook_set_active_ext_lang)
>       selftests::hook_set_active_ext_lang ();
> @@ -808,6 +835,10 @@ set_active_ext_lang (const struct extension_language_defn *now_active)
>   void
>   restore_active_ext_lang (struct active_ext_lang_state *previous)
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>     if (cooperative_sigint_handling_disabled)
>       {
>         /* See set_active_ext_lang.  */
> @@ -844,6 +875,10 @@ restore_active_ext_lang (struct active_ext_lang_state *previous)
>   void
>   set_quit_flag (void)
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>     if (active_ext_lang->ops != NULL
>         && active_ext_lang->ops->set_quit_flag != NULL)
>       active_ext_lang->ops->set_quit_flag (active_ext_lang);
> @@ -868,6 +903,10 @@ set_quit_flag (void)
>   int
>   check_quit_flag (void)
>   {
> +#if CXX_STD_THREAD
> +  std::lock_guard guard (ext_lang_mutex);
> +#endif /* CXX_STD_THREAD */
> +
>     int result = 0;
>   
>     for (const struct extension_language_defn *extlang : extension_languages)
> diff --git a/gdb/python/py-dap.c b/gdb/python/py-dap.c
> index 2034105c939..9a00130fe90 100644
> --- a/gdb/python/py-dap.c
> +++ b/gdb/python/py-dap.c
> @@ -92,10 +92,14 @@ call_dap_fn (const char *fn_name)
>   void
>   dap_interp::init (bool top_level)
>   {
> +#if CXX_STD_THREAD
>     call_dap_fn ("run");
>   
>     current_ui->input_fd = -1;
>     current_ui->m_input_interactive_p = false;
> +#else
> +  error (_("GDB was compiled without threading, which DAP requires"));
> +#endif
>   }
>   
>   void
  

Tom> LGTM, just the "let this race" comment needs updating.

Thanks.  I've updated that and now I'm going to check it in.

Tom