Re-evaluate floating varobj as part of varobj_invalidate.

Re-evaluate floating varobj as part of varobj_invalidate.

[Andrew Burgess]

Hi,

I have a bug & patch, but no great way to reproduce the issue as it's a use of pointer after free issue, so it depends on having the memory reused after the free.  It was originally spotted on an older version of gdb (7.0) but it still applies, it just isn't so easy to trigger.

Using x86/linux/glibc I can hook the free() call to scribble over all free'd memory, in this case I can cause the issue to be triggered on latest head.  I'm happy to post a patch for this if anyone wants to reproduce the bug this way.

So, the bug...

Using this test program (var.c):

int global_var = 3;

int
main ( void )
{
  return 0;
}

Compiled up as var.x (gcc -g -o var.x var.c), I start gdb as:

gdb -i mi2 ./var.x

Then I enter the commands:

1-var-create - @ global_var
2-file-symbol-file var.x
3-var-set-format var1 hexadecimal

That's it, gdb can segfault while running command 3-var-set-format.

The reason for the failure is this:

 * In 1-var-create we evaluate the expression "global_var" this returns a "struct value" that has a type, this type and the value are cached on the "struct varobj".  
 * During 2-file-symbol-file all the types from the objfile are free'd, this will include the type referenced by the value on the varobj, and the type cached directly on the varobj.  As part of the symbol file loading we do call varobj_invalidate which calls varobj_invalidate_iter, however, our varobj is left untouched as it is a "floating" varobj.
 * During 3-var-set-format we change the format of the varobj then call varobj_value_is_changeable_p, which calls (usually) default_value_is_changeable_p, which in turn calls get_value_type, which finally, calls check_typedef on the type which was free'd.

The patch below simply removes the special case for floating varobj objects from varobj_invalidate_iter, and causes them to be re-evaluated or marked as invalid.

Is this OK to apply?

Thanks

Andrew

gdb/Change

2012-07-25  Andrew Burgess  <aburgess@broadcom.com>

	* varobj.c (varobj_invalidate_iter): All varobj must be marked as
	invalid or reevaluated to prevent prevent references to possibly
	delete'd type objects being left in the varobj.

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 99b158e..a75a40d 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -4186,18 +4186,17 @@ _initialize_varobj (void)
 }
 
 /* Invalidate varobj VAR if it is tied to locals and re-create it if it is
-   defined on globals.  It is a helper for varobj_invalidate.  */
+   defined on globals.  It is a helper for varobj_invalidate.
+
+   This function is called after changing the symbol file, in this case the
+   pointers to "struct type" stored by the varobj are no longer valid.  All
+   varobj must be either re-evaluated, or marked as invalid here.  */
 
 static void
 varobj_invalidate_iter (struct varobj *var, void *unused)
 {
-  /* Floating varobjs are reparsed on each stop, so we don't care if the
-     presently parsed expression refers to something that's gone.  */
-  if (var->root->floating)
-    return;
-
-  /* global var must be re-evaluated.  */     
-  if (var->root->valid_block == NULL)
+  /* global and floating var must be re-evaluated.  */
+  if (var->root->floating || var->root->valid_block == NULL)
     {
       struct varobj *tmp_var;

Re: Re-evaluate floating varobj as part of varobj_invalidate.

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@broadcom.com> writes:

Andrew> I have a bug & patch, but no great way to reproduce the issue as
Andrew> it's a use of pointer after free issue,

I suspect the recent-ish change to use -lmcheck by default will cause us
to catch this bug in the test suite, at least on glibc-using systems.

Andrew> The patch below simply removes the special case for floating
Andrew> varobj objects from varobj_invalidate_iter, and causes them to
Andrew> be re-evaluated or marked as invalid.

Seems reasonable to me.

Andrew> 2012-07-25  Andrew Burgess  <aburgess@broadcom.com>
Andrew> 	* varobj.c (varobj_invalidate_iter): All varobj must be marked as
Andrew> 	invalid or reevaluated to prevent prevent references to possibly
Andrew> 	delete'd type objects being left in the varobj.

Patch is ok.

Tom

Re: Re-evaluate floating varobj as part of varobj_invalidate.

[Jan Kratochvil]

On Wed, 25 Jul 2012 19:52:22 +0200, Tom Tromey wrote:
> >>>>> "Andrew" == Andrew Burgess <aburgess@broadcom.com> writes:
> 
> Andrew> I have a bug & patch, but no great way to reproduce the issue as
> Andrew> it's a use of pointer after free issue,
> 
> I suspect the recent-ish change to use -lmcheck by default will cause us
> to catch this bug in the test suite, at least on glibc-using systems.

Yes, FSF GDB HEAD crashes for me with the posted reproducer.  Could you write
this simple testcase?


Thanks,
Jan

Re: Re-evaluate floating varobj as part of varobj_invalidate.

[Andrew Burgess]

On 26/07/2012 9:35 AM, Jan Kratochvil wrote:
> On Wed, 25 Jul 2012 19:52:22 +0200, Tom Tromey wrote:
>>>>>>> "Andrew" == Andrew Burgess <aburgess@broadcom.com> writes:
>>
>> Andrew> I have a bug & patch, but no great way to reproduce the issue as
>> Andrew> it's a use of pointer after free issue,
>>
>> I suspect the recent-ish change to use -lmcheck by default will cause us
>> to catch this bug in the test suite, at least on glibc-using systems.
> 
> Yes, FSF GDB HEAD crashes for me with the posted reproducer.  Could you write
> this simple testcase?

There's a patch below that tests this issue, it does indeed fail for me running on x86-linux before my fix patch, and works afterwards.

If this test is ok I'll commit the fix and the test patches together.

Thanks
Andrew

gdb/testsuite/ChangeLog

2012-07-27  Andrew Burgess  <aburgess@broadcom.com>

       * gdb.mi/mi-var-invalidate.exp: Create a floating variable and
       change its format immediately after reloading the binary.

diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate.exp
index 3ecbbdf..98dd3f1 100644
--- a/gdb/testsuite/gdb.mi/mi-var-invalidate.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate.exp
@@ -61,6 +61,9 @@ mi_runto do_locals_tests
 # Desc: create local variables
 mi_create_varobj linteger linteger "create local variable linteger"
 
+# Desc: create floating variable
+mi_create_floating_varobj float_simple array "create floating variable"
+
 #
 # Reload the same binary.
 # Global variable should remain, local should be invalidated.
@@ -69,6 +72,12 @@ mi_delete_breakpoints
 mi_gdb_load ${binfile_bis}
 mi_runto main
 
+# Change format of floating variable immediately after reload reveals a
+# bug where gdb still uses a free'd pointer.
+mi_gdb_test "-var-set-format float_simple hexadecimal" \
+        "\\^done,format=\"hexadecimal\",value=\"\\\[-1\\\]\"" \
+       "set format variable float_simple"
+
 # Check local variable is "invalid".
 mi_gdb_test "-var-update linteger" \
        "\\^done,changelist=\\\[\{name=\"linteger\",in_scope=\"invalid\",has_more=\"0\"\}\\\]" \

Re: Re-evaluate floating varobj as part of varobj_invalidate.

[Jan Kratochvil]

On Fri, 27 Jul 2012 10:20:18 +0200, Andrew Burgess wrote:
> If this test is ok I'll commit the fix and the test patches together.
>        * gdb.mi/mi-var-invalidate.exp: Create a floating variable and
>        change its format immediately after reloading the binary.

Yes, check it in, please.


Thanks,
Jan

Re: Re-evaluate floating varobj as part of varobj_invalidate.

[Andrew Burgess]

On 27/07/2012 3:06 PM, Jan Kratochvil wrote:
> On Fri, 27 Jul 2012 10:20:18 +0200, Andrew Burgess wrote:
>> If this test is ok I'll commit the fix and the test patches together.
>>         * gdb.mi/mi-var-invalidate.exp: Create a floating variable and
>>         change its format immediately after reloading the binary.
>
> Yes, check it in, please.

Committed: http://sourceware.org/ml/gdb-cvs/2012-07/msg00243.html

Cheers,
Andrew