Re: [PATCH][gdb/guile] Don't allow libguile to change libgmp mem fns

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Tom de Vries <tdevries@suse.de> [2021-05-03 10:54:29 +0200]:

> Hi,
> 
> Since gdb commit 880ae75a2b7 "gdb delay guile initialization until
> gdbscm_finish_initialization" I'm running into:
> ...
> (gdb) print My_Var > 10.0^M
> free(): invalid pointer^M
> ERROR: GDB process no longer exists
> GDB process exited with wait status 5995 exp9 0 0 CHILDKILLED SIGABRT SIGABRT
> UNRESOLVED: gdb.ada/fixed_cmp.exp: gnat_encodings=all: print My_Var > 10.0
> ...
> 
> The problem is that both gdb and libguile try to set the libgmp memory functions,
> and since the gdb commit the ones from libguile are effective, which results
> in gdb freeing some memory in a way that is not compatible with the way that
> memory was actually allocated.
> 
> The fact that libguile tries to set the libgmp memory functions is a bug which
> should be fixed starting version v3.0.6.
> 
> Meanwhile, work around this in gdb by not allowing libguile to set the libgomp
> memory functions.

Thanks for looking into this, and sorry for causing the breakage.

I had a read through the bug, and this solution seems to make sense,
however, I had two thoughts.

First, I already had to solve a similar problem for Python when doing
this work.  For Python the issue related to which signal handlers were
installed.

Though the problem was Python specific, I figured that it was cleaner
to make the solution generic, so I placed the fix in
gdb/extension.c:ext_lang_initialization - look for the use of
scoped_default_sigint.

I wonder if this fix should similarly be placed at this level?  I'll
leave this choice up to you, I don't feel strongly on this, but
thought it might be worth mentioning.

For the second thought, see below...

> 
> Tested on x86_64-linux.
> 
> Any comments?
> 
> Thanks,
> - Tom
> 
> [gdb/guile] Don't allow libguile to change libgmp mem fns
> 
> gdb/ChangeLog:
> 
> 2021-05-03  Tom de Vries  <tdevries@suse.de>
> 
> 	PR guile/27806
> 	* guile/guile.c (gdbscm_initialize): Save and restore libgmp memory
> 	functions.
> 
> ---
>  gdb/guile/guile.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
> index bdf15cd498b..6ee8b3f47ce 100644
> --- a/gdb/guile/guile.c
> +++ b/gdb/guile/guile.c
> @@ -662,10 +662,32 @@ gdbscm_initialize (const struct extension_language_defn *extlang)
>    {
>      gdb::block_signals blocker;
>  
> +    /* There are libguile versions (f.i. v3.0.5) that by default call
> +       mp_get_memory_functions during initialization to install custom
> +       libgmp memory functions.  This is considered a bug and should be
> +       fixed starting v3.0.6.
> +       Before gdb commit 880ae75a2b7 "gdb delay guile initialization until
> +       gdbscm_finish_initialization", that bug had no effect for gdb,
> +       because gdb subsequently called mp_get_memory_functions to install
> +       its own custom functions in _initialize_gmp_utils.  However, since
> +       aforementioned gdb commit the initialization order is reversed,
> +       allowing libguile to install a custom malloc that is incompatible
> +       with the custom free as used in gmp-utils.c, resulting in a
> +       "double free or corruption (out)" error.
> +       Work around the libguile bug by saving the libgmp memory functions
> +       before guile initialization, and restoring them afterwards.  */
> +    void *(*alloc_func) (size_t);
> +    void *(*realloc_func) (void *, size_t, size_t);
> +    void (*free_func) (void *, size_t);
> +    mp_get_memory_functions (&alloc_func, &realloc_func, &free_func);

I think any time we do SAVE-VALUE -> WORK -> RESTORE-VALUE, we should
be wrapping this up in an RAII class.  Right now I don't believe
scm_with_guile can throw an exception, but you never know how the code
will change in the future, and creating an RAII class now just makes
things future proof.

Thanks,
Andrew

> +
>      /* scm_with_guile is the most portable way to initialize Guile.  Plus
>         we need to initialize the Guile support while in Guile mode (e.g.,
>         called from within a call to scm_with_guile).  */
>      scm_with_guile (call_initialize_gdb_module, NULL);
> +
> +    /* Restore libgmp memory functions.  */
> +    mp_set_memory_functions (alloc_func, realloc_func, free_func);
>    }
>  
>    /* Set Guile's backtrace to match the "set guile print-stack" default.