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

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

[-- Attachment #1: Type: text/plain, Size: 2492 bytes --]

On 5/3/21 10:10 PM, Ludovic Courtès wrote:
> Hi Tom,
> 
> Tom de Vries <tdevries@suse.de> skribis:
> 
>> The fact that libguile tries to set the libgmp memory functions is a bug which
>> should be fixed starting version v3.0.6.
> 
> Yes.  Building Guile with mini-GMP is recommended in 3.0.6 and later.
> 
>> +++ 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);
>> +
>>      /* 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);
> 
> This code would lead to memory leaks because Guile would still think it
> has its memory functions installed so it would never explicitly free GMP
> memory.
> 
> Instead, you can do:
> 
>   scm_install_gmp_memory_functions = 0;
> 
> before the first call to ‘scm_with_guile’.  That works with 3.0, 2.2,
> and 2.0.
> 
> HTH!

It does, thanks for the review :)

Committed as below.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-guile-Don-t-allow-libguile-to-change-libgmp-mem-fns.patch --]
[-- Type: text/x-patch, Size: 2643 bytes --]

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

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.

Tested on x86_64-linux.

gdb/ChangeLog:

2021-05-03  Tom de Vries  <tdevries@suse.de>

	PR guile/27806
	* guile/guile.c	(gdbscm_initialize): Don't let guile change libgmp
	memory functions.

---
 gdb/guile/guile.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
index bdf15cd498b..c6959f5b713 100644
--- a/gdb/guile/guile.c
+++ b/gdb/guile/guile.c
@@ -662,6 +662,22 @@ 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 disabling the installation of the
+       libgmp memory functions by guile initialization.  */
+    scm_install_gmp_memory_functions = 0;
+
     /* 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).  */