Re: [RFA] breakpoint.c: Avoid double freeing in breakpoint_re_set_one

Re: [RFA] breakpoint.c: Avoid double freeing in breakpoint_re_set_one

[Paul Hilfinger]

Here is a follow-up on my earlier patch.  I found a couple of other 
instances of potential double freeing in the same routine.

OK?

Paul Hilfinger

2004-01-12  Paul N. Hilfinger  <hilfinger@gnat.com>

	* breakpoint.c: Update copyright to include 2004.
	(breakpoint_re_set_one): Set b->cond, b->val, and b->exp to NULL 
	after freeing so that error during re-parsing or evaluation
	of expressions associated with breakpoint don't eventually
	lead to re-freeing of storage.
	
Index: current-public.41/gdb/breakpoint.c
--- current-public.41/gdb/breakpoint.c Sun, 04 Jan 2004 17:51:24 -0800 hilfingr (GdbPub/g/23_breakpoint 1.1.1.7.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1 644)
+++ current-public.41(w)/gdb/breakpoint.c Tue, 13 Jan 2004 01:17:24 -0800 hilfingr (GdbPub/g/23_breakpoint 1.1.1.7.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1 644)
@@ -1,8 +1,8 @@
 /* Everything about breakpoints, for GDB.
 
    Copyright 1986, 1987, 1988, 1989, 1990, 1991, 1992, 1993, 1994,
-   1995, 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003 Free Software
-   Foundation, Inc.
+   1995, 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004 
+   Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -7009,6 +7009,8 @@ breakpoint_re_set_one (void *bint)
 	      s = b->cond_string;
 	      if (b->cond)
 		xfree (b->cond);
+	      /* Avoid re-freeing b->cond if error during parse_exp_1. */
+	      b->cond = NULL;
 	      b->cond = parse_exp_1 (&s, block_for_pc (sals.sals[i].pc), 0);
 	    }
 
@@ -7077,11 +7079,15 @@ breakpoint_re_set_one (void *bint)
       /* So for now, just use a global context.  */
       if (b->exp)
 	xfree (b->exp);
+      /* Avoid re-freeing b->exp if error during parse_expression. */
+      b->exp = NULL;
       b->exp = parse_expression (b->exp_string);
       b->exp_valid_block = innermost_block;
       mark = value_mark ();
       if (b->val)
 	value_free (b->val);
+      /* Avoid re-freeing b->val if error during evaluate_expression. */
+      b->val = NULL;
       b->val = evaluate_expression (b->exp);
       release_value (b->val);
       if (VALUE_LAZY (b->val) && breakpoint_enabled (b))
@@ -7092,6 +7098,8 @@ breakpoint_re_set_one (void *bint)
 	  s = b->cond_string;
 	  if (b->cond)
 	    xfree (b->cond);
+	  /* Avoid re-freeing b->cond if error during parse_exp_1. */
+	  b->cond = NULL;
 	  b->cond = parse_exp_1 (&s, (struct block *) 0, 0);
 	}
       if (breakpoint_enabled (b))

Re: [RFA] breakpoint.c: Avoid double freeing in breakpoint_re_set_one

[Andrew Cagney]

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

> Here is a follow-up on my earlier patch.  I found a couple of other 
> instances of potential double freeing in the same routine.

You wouldn't have a test case - at least something that causes this code 
to go through the motions?  That way on systems with pedantic memory 
managers there'd bee some sort of error.

Anyway, I've committed the attached (slightly tweaked).

Andrew


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 3176 bytes --]

2004-01-27  Andrew Cagney  <cagney@redhat.com>

	* source.c (ambiguous_line_spec): Delete undefined declaration.
	* m32r-rom.c (m32r_set_board_address): Delete unused function.
	(m32r_set_server_address, m32r_set_download_path): Ditto.
	* remote-fileio.c (remote_fileio_to_fio_int): Ditto.

Index: m32r-rom.c
===================================================================
RCS file: /cvs/src/src/gdb/m32r-rom.c,v
retrieving revision 1.13
diff -u -r1.13 m32r-rom.c
--- m32r-rom.c	1 Aug 2003 21:14:33 -0000	1.13
+++ m32r-rom.c	27 Jan 2004 23:18:57 -0000
@@ -398,63 +398,6 @@
   monitor_open (args, &mon2000_cmds, from_tty);
 }
 
-/* Function: set_board_address
-   Tell the BootOne monitor what it's ethernet IP address is. */
-
-static void
-m32r_set_board_address (char *args, int from_tty)
-{
-  int resp_len;
-  char buf[1024];
-
-  if (args && *args)
-    {
-      monitor_printf ("ulip %s\n", args);
-      resp_len = monitor_expect_prompt (buf, sizeof (buf));
-      /* now parse the result for success */
-    }
-  else
-    error ("Requires argument (IP address for M32R-EVA board)");
-}
-
-/* Function: set_server_address
-   Tell the BootOne monitor what gdb's ethernet IP address is. */
-
-static void
-m32r_set_server_address (char *args, int from_tty)
-{
-  int resp_len;
-  char buf[1024];
-
-  if (args && *args)
-    {
-      monitor_printf ("uhip %s\n", args);
-      resp_len = monitor_expect_prompt (buf, sizeof (buf));
-      /* now parse the result for success */
-    }
-  else
-    error ("Requires argument (IP address of GDB's host computer)");
-}
-
-/* Function: set_download_path
-   Tell the BootOne monitor the default path for downloadable SREC files. */
-
-static void
-m32r_set_download_path (char *args, int from_tty)
-{
-  int resp_len;
-  char buf[1024];
-
-  if (args && *args)
-    {
-      monitor_printf ("up %s\n", args);
-      resp_len = monitor_expect_prompt (buf, sizeof (buf));
-      /* now parse the result for success */
-    }
-  else
-    error ("Requires argument (default path for downloadable SREC files)");
-}
-
 static void
 m32r_upload_command (char *args, int from_tty)
 {
Index: remote-fileio.c
===================================================================
RCS file: /cvs/src/src/gdb/remote-fileio.c,v
retrieving revision 1.5
diff -u -r1.5 remote-fileio.c
--- remote-fileio.c	30 Dec 2003 06:52:09 -0000	1.5
+++ remote-fileio.c	27 Jan 2004 23:18:57 -0000
@@ -364,12 +364,6 @@
 }
 
 static void
-remote_fileio_to_fio_int (long num, fio_int_t fnum)
-{
-  remote_fileio_to_be ((LONGEST) num, (char *) fnum, 4);
-}
-
-static void
 remote_fileio_to_fio_uint (long num, fio_uint_t fnum)
 {
   remote_fileio_to_be ((LONGEST) num, (char *) fnum, 4);
Index: source.c
===================================================================
RCS file: /cvs/src/src/gdb/source.c,v
retrieving revision 1.48
diff -u -r1.48 source.c
--- source.c	19 Jan 2004 01:20:11 -0000	1.48
+++ source.c	27 Jan 2004 23:18:57 -0000
@@ -80,8 +80,6 @@
 
 static void line_info (char *, int);
 
-static void ambiguous_line_spec (struct symtabs_and_lines *);
-
 static void source_info (char *, int);
 
 static void show_directories (char *, int);

Re: [RFA] breakpoint.c: Avoid double freeing in breakpoint_re_set_one

[Andrew Cagney]

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

> Anyway, I've committed the attached (slightly tweaked).

Doh! Try ...

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 1649 bytes --]

2004-01-27  Paul N. Hilfinger  <hilfinger@gnat.com>
	
	* breakpoint.c (breakpoint_re_set_one): Set b->cond, b->val, and
	b->exp to NULL after freeing so that error during re-parsing or
	evaluation of expressions associated with breakpoint don't
	eventually lead to re-freeing of storage.
	Committed by Andrew Cagney.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.151
diff -u -r1.151 breakpoint.c
--- breakpoint.c	27 Jan 2004 03:13:34 -0000	1.151
+++ breakpoint.c	28 Jan 2004 01:34:34 -0000
@@ -6970,12 +6970,22 @@
 
       /* So for now, just use a global context.  */
       if (b->exp)
-	xfree (b->exp);
+	{
+	  xfree (b->exp);
+	  /* Avoid re-freeing b->exp if an error during the call to
+             parse_expression.  */
+	  b->exp = NULL;
+	}
       b->exp = parse_expression (b->exp_string);
       b->exp_valid_block = innermost_block;
       mark = value_mark ();
       if (b->val)
-	value_free (b->val);
+	{
+	  value_free (b->val);
+	  /* Avoid re-freeing b->val if an error during the call to
+             evaluate_expression.  */
+	  b->val = NULL;
+	}
       b->val = evaluate_expression (b->exp);
       release_value (b->val);
       if (VALUE_LAZY (b->val) && breakpoint_enabled (b))
@@ -6985,7 +6995,12 @@
 	{
 	  s = b->cond_string;
 	  if (b->cond)
-	    xfree (b->cond);
+	    {
+	      xfree (b->cond);
+	      /* Avoid re-freeing b->exp if an error during the call
+		 to parse_exp_1.  */
+	      b->cond = NULL;
+	    }
 	  b->cond = parse_exp_1 (&s, (struct block *) 0, 0);
 	}
       if (breakpoint_enabled (b))

Re: [RFA] breakpoint.c: Avoid double freeing in breakpoint_re_set_one

[Paul Hilfinger]

> > Anyway, I've committed the attached (slightly tweaked).
> 
> Doh! Try ...

I was going to say that was quite an editing job!

> You wouldn't have a test case - at least something that causes this code 
> to go through the motions?  That way on systems with pedantic memory 
> managers there'd bee some sort of error.

As you suggest, this is a little hard to test.  I am not too familiar with
the testsuite innards yet.  Here is a draft of a possible test.  Comments?

Paul

---------------------------- gdb.base/badfree.exp --------------------
# Copyright 2004 Free Software Foundation, Inc.

# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 2 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.

# Please email any bugs, comments, and/or additions to this file to:
# bug-gdb@prep.ai.mit.edu

# Author: Paul N. Hilfinger (Hilfinger@gnat.com)

# Test that GDB cleans up properly after errors that result when a 
# breakpoint is reset. 

if $tracelevel then {
	strace $tracelevel
}

# IDT/SIM apparently doesn't have enough file descriptors to allow the
# problem checked by this test to occur.
if [istarget "mips-idt-*"] {
    return 0;
}

set testfile "badfree"
set srcfile ${testfile}.c
set binfile ${objdir}/${subdir}/${testfile}

if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug additional_flags=-DVARIABLE=var1}] != "" } {
    gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
}

set oldtimeout $timeout
set timeout 10
verbose "Timeout is now 10 seconds" 2

proc expect_to_stop_here { ident } {
    global gdb_prompt
    global decimal

    # the "at foo.c:36" output we get with -g.
    # the "in func" output we get without -g.
    gdb_expect 30 {
	-re "Breakpoint \[0-9\]*, stop_here .*$gdb_prompt $" { 
	    return 1
	}
	-re "$gdb_prompt $" { 
	    fail "running to stop_here $ident"
	    return 0
	}
	timeout { 
	    fail "running to stop_here $ident (timeout)"
	    return 0
	}
    }
    return 1
}

gdb_exit
gdb_start
gdb_reinitialize_dir $srcdir/$subdir
gdb_load ${binfile}

gdb_test "break stop_here if (var1 == 42)\n" \
    "Breakpoint.*at.* file .*$srcfile, line.*" \
    "setting conditional breakpoint on function"
gdb_run_cmd

expect_to_stop_here "first time"
    
gdb_continue_to_end "breakpoint first time through"

# Now we recompile the executable, but without a variable named "var1", first
# waiting to insure that even on fast machines, the file modification times
# are distinct. This will force GDB to reload the file on the 
# next "run" command causing an error when GDB tries to tries to reset 
# the breakpoint.

sleep 2
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug additional_flags=-DVARIABLE=var2}] != "" } {

# Complication: Since GDB generally holds an open file descriptor on the 
# executable at this point, there are some systems in which the 
# re-compilation will fail. In such cases, we'll consider the test 
# (vacuously) passed providing that re-running it succeeds as before.

    gdb_run_cmd
    expect_to_stop_here "after re-compile fails"
    gdb_continue_to_end "after re-compile fails"

} else {

    send_gdb "run\n"
    gdb_expect {
	-re ".* has changed; .*Error in re-setting .*No symbol .var1..*Program exited normally.*"\
		{ pass "running with invalidated bpt condition after executable changes" }
	timeout {
	    fail "(timeout) running with invalidated bpt condition after executable changes" }
    }

}

---------------------------- gdb.base/badfree.c --------------------
/*
 *   Test that GDB cleans up properly after errors that result when a 
 * breakpoint is reset. 
 */

/* VARIABLE is a macro defined on the compiler command line. */

#include <stdlib.h>

int VARIABLE = 42;

void stop_here ()
{
  VARIABLE *= 2;
}

main ()
{
  stop_here ();
  exit (0);
}

Re: [RFA] breakpoint.c: Avoid double freeing in breakpoint_re_set_one

[Andrew Cagney]

Yep.

Few tweaks.

> # IDT/SIM apparently doesn't have enough file descriptors to allow the
> # problem checked by this test to occur.
> if [istarget "mips-idt-*"] {
>     return 0;
> }

is this true?  If it is it looks more like a bug.  Other workarounds may 
be in the same situtation.

Just check that the braces are indented:

	gdb_expect {
	    -re ... {
	    }
	}

thanks,
Andrew

Re: [RFA] breakpoint.c: Avoid double freeing in breakpoint_re_set_one

[Paul Hilfinger]

 > 
 > > # IDT/SIM apparently doesn't have enough file descriptors to allow the
 > > # problem checked by this test to occur.
 > > if [istarget "mips-idt-*"] {
 > >     return 0;
 > > }
 > 
 > is this true?  If it is it looks more like a bug.  Other workarounds may 
 > be in the same situtation.

Well, it's just a small modification of code I found strewn around
other test cases that involved re-reading an executable and the like. 
If what you are saying is that GDB might be holding onto file descriptors
too long, then I must agree, as indicated in a comment further down in
the test:

  # Complication: Since GDB generally holds an open file descriptor on the 
  # executable at this point, there are some systems in which the 
  # re-compilation will fail....

That is, after the inferior exits, GDB (or, if you prefer, BFD) still
has the executable file open (at least that's what it looked like to
me).  I'm curious as to whether this is an important feature.


 > Just check that the braces are indented:
 > 
 > 	gdb_expect {
 > 	    -re ... {
 > 	    }
 > 	}

OK

Paul

[PATCH] Test for double freeing in breakpoint_re_set_one

[Paul Hilfinger]

Per Andrew's request, I have added the following test to the testsuite
in gdb.base.  This particular problem, since it involves memory corruption,
won't reliably show itself (i.e., on earlier versions of GDB) unless one
is "lucky" enough to have it cause a segmentation fault or one is using
a checked version of malloc/free in GDB.  I will leave to others Andrew's
question concerning file descriptors on IDT/SIM.  

Paul Hilfinger

2004-01-29  Paul N. Hilfinger  <Hilfinger@gnat.com>

	* gdb.base/chng-syms.exp: New file.
	* gdb.base/chng-syms.c: New file.

Index: merge.82/gdb/testsuite/gdb.base/Makefile.in
--- merge.82/gdb/testsuite/gdb.base/Makefile.in Tue, 17 Jun 2003 02:27:41 -0700 hilfingr (GdbPub/H/20_Makefile.i 1.2 644)
+++ merge.85/gdb/testsuite/gdb.base/Makefile.in Thu, 29 Jan 2004 03:01:11 -0800 hilfingr (GdbPub/H/20_Makefile.i 1.4 644)
@@ -3,7 +3,7 @@ srcdir = @srcdir@
 
 EXECUTABLES = all-types annota1 bitfields break \
 	call-ar-st call-rt-st call-strs callfuncs callfwmall \
-	commands compiler condbreak constvars coremaker \
+	chng-syms commands compiler condbreak constvars coremaker \
 	dbx-test display ending-run execd-prog exprs \
 	foll-exec foll-fork foll-vfork funcargs int-type interrupt jump \
 	langs list long_long mips_pro miscexprs nodebug opaque overlays \
Index: merge.82/gdb/testsuite/gdb.base/chng-syms.exp
--- merge.82/gdb/testsuite/gdb.base/chng-syms.exp Thu, 29 Jan 2004 03:12:31 -0800 hilfingr ()
+++ merge.85/gdb/testsuite/gdb.base/chng-syms.exp Thu, 29 Jan 2004 02:47:41 -0800 hilfingr (GdbPub/k/c/23_badfree.ex 1.2 644)
@@ -0,0 +1,120 @@
+# Copyright 2004 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+
+# Please email any bugs, comments, and/or additions to this file to:
+# bug-gdb@prep.ai.mit.edu
+
+# Author: Paul N. Hilfinger (Hilfinger@gnat.com)
+
+# Test that GDB cleans up properly after errors that result when a 
+# breakpoint is reset. 
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+# IDT/SIM apparently doesn't have enough file descriptors to allow the
+# problem checked by this test to occur.
+if [istarget "mips-idt-*"] {
+    return 0;
+}
+
+set testfile "chng-syms"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug additional_flags=-DVARIABLE=var1}] != "" } {
+    gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+}
+
+set oldtimeout $timeout
+set timeout 10
+verbose "Timeout is now 10 seconds" 2
+
+proc expect_to_stop_here { ident } {
+    global gdb_prompt
+    global decimal
+
+    # the "at foo.c:36" output we get with -g.
+    # the "in func" output we get without -g.
+    gdb_expect {
+	-re "Breakpoint \[0-9\]*, stop_here .*$gdb_prompt $" { 
+	    return 1
+	}
+	-re "$gdb_prompt $" { 
+	    fail "running to stop_here $ident"
+	    return 0
+	}
+	timeout { 
+	    fail "running to stop_here $ident (timeout)"
+	    return 0
+	}
+    }
+    return 1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+gdb_test "break stop_here if (var1 == 42)\n" \
+    "Breakpoint.*at.* file .*$srcfile, line.*" \
+    "setting conditional breakpoint on function"
+gdb_run_cmd
+
+expect_to_stop_here "first time"
+    
+gdb_continue_to_end "breakpoint first time through"
+
+# Now we recompile the executable, but without a variable named "var1", first
+# waiting to insure that even on fast machines, the file modification times
+# are distinct. This will force GDB to reload the file on the 
+# next "run" command, causing an error when GDB tries to tries to reset 
+# the breakpoint.
+
+sleep 2
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug additional_flags=-DVARIABLE=var2}] != "" } {
+
+# Complication: Since GDB generally holds an open file descriptor on the 
+# executable at this point, there are some systems in which the 
+# re-compilation will fail. In such cases, we'll consider the test 
+# (vacuously) passed providing that re-running it succeeds as before.
+
+    gdb_run_cmd
+    expect_to_stop_here "after re-compile fails"
+    gdb_continue_to_end "after re-compile fails"
+
+} else {
+
+    gdb_run_cmd
+    gdb_expect {
+	-re "Error in re-setting .*No symbol .var1..*Program exited normally.*" {
+	    pass "running with invalidated bpt condition after executable changes" 
+	}
+	timeout {
+	    fail "(timeout) running with invalidated bpt condition after executable changes" 
+	}
+    }
+
+}
+
+set timeout $oldtimeout
+verbose "Timeout is now $timeout seconds" 2
+return 0
Index: merge.82/gdb/testsuite/gdb.base/chng-syms.c
--- merge.82/gdb/testsuite/gdb.base/chng-syms.c Thu, 29 Jan 2004 03:12:31 -0800 hilfingr ()
+++ merge.85/gdb/testsuite/gdb.base/chng-syms.c Thu, 29 Jan 2004 02:24:00 -0800 hilfingr (GdbPub/k/c/24_badfree.c 1.1 644)
@@ -0,0 +1,22 @@
+/*
+ *   Test that GDB cleans up properly after errors that result when a 
+ * breakpoint is reset. 
+ */
+
+/* VARIABLE is a macro defined on the compiler command line. */
+
+#include <stdlib.h>
+
+int VARIABLE = 42;
+
+void stop_here ()
+{
+  VARIABLE *= 2;
+}
+
+int main ()
+{
+  stop_here ();
+  exit (0);
+}
+

Re: [PATCH] Test for double freeing in breakpoint_re_set_one

[Andrew Cagney]

> Per Andrew's request, I have added the following test to the testsuite
> in gdb.base.  This particular problem, since it involves memory corruption,
> won't reliably show itself (i.e., on earlier versions of GDB) unless one
> is "lucky" enough to have it cause a segmentation fault or one is using
> a checked version of malloc/free in GDB.  I will leave to others Andrew's
> question concerning file descriptors on IDT/SIM.  

Just FYI, I did some digging.  The IDT/SIM problem would have been on 
cygwin.  The simulators would hold open the executable making the 
instalation of a new one impossible.  Since the simulators now close the 
bfd cache (releasing the executable) that specific problem will no 
longer occure.  However, I think GDB still keeps the executable open 
leading to the second of the problems.

Andrew