Re: [PATCH] Fix gdb.interrupt race

[Tom de Vries <tdevries@suse.de>]

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)