[PATCH] Fix segfault on empty else

[PATCH] Fix segfault on empty else

[Andrew STUBBS]

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

The attached patch fixes a segmentation fault that occurs when a GDB 
script has an empty else clause.

E.g.

if $cond
   echo here\n
else
   # boom
end

The command structure is apparently read correctly (from the user's 
perspective), but GDB will crash when it tries to a) execute the else 
clause ($cond == 0), or b) free the command, if the command is not in a 
define or the user-defined command is redefined.

This problem is caused by a pointer that is only initialised when it is 
first used, which is never when there are no commands.

:ADDPATCH CLI:

Andrew Stubbs

[-- Attachment #2: empty-else.patch --]
[-- Type: text/plain, Size: 716 bytes --]

2006-06-20  Andrew Stubbs  <andrew.stubbs@st.com>

        * cli/cli-script.c (realloc_body_list): Zero new parts of body_list.


Index: src/gdb/cli/cli-script.c
===================================================================
--- src.orig/gdb/cli/cli-script.c	2006-04-07 14:31:15.000000000 +0100
+++ src/gdb/cli/cli-script.c	2006-06-20 15:15:25.000000000 +0100
@@ -701,6 +701,7 @@ realloc_body_list (struct command_line *
     xmalloc (sizeof (struct command_line *) * new_length);
 
   memcpy (body_list, command->body_list, sizeof (struct command_line *) * n);
+  memset (body_list + n, 0, sizeof (struct command_line *) * (new_length - n));
 
   xfree (command->body_list);
   command->body_list = body_list;

Re: [PATCH] Fix segfault on empty else

[Daniel Jacobowitz]

On Tue, Jun 20, 2006 at 03:33:37PM +0100, Andrew STUBBS wrote:
> 2006-06-20  Andrew Stubbs  <andrew.stubbs@st.com>
> 
>         * cli/cli-script.c (realloc_body_list): Zero new parts of body_list.

OK, thanks!

:REVIEWMAIL:

Want to add a corresponding test?  Though it likely wouldn't crash,
unless you tried running the testsuite under valgrind.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Fix segfault on empty else

[Andrew STUBBS]

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

Daniel Jacobowitz wrote:
> On Tue, Jun 20, 2006 at 03:33:37PM +0100, Andrew STUBBS wrote:
>> 2006-06-20  Andrew Stubbs  <andrew.stubbs@st.com>
>>
>>         * cli/cli-script.c (realloc_body_list): Zero new parts of body_list.
> 
> OK, thanks!

Thanks, committed.

> Want to add a corresponding test?  Though it likely wouldn't crash,
> unless you tried running the testsuite under valgrind.

How about the attached? It is somewhat tricky trying to reliably 
reproduce the problem without valgrind, as you say.

I have put in a few commands that are intended to run through the same 
code and will, most likely, allocate memory in the same place, in order 
to ensure that the crash occurs. This is a little hopeful, but what else 
can I do?

This works for me, but could you please confirm that it works in your 
setup/host (without the patch to fix the problem of course).

Thanks

Andrew

[-- Attachment #2: empty-else-test.patch --]
[-- Type: text/plain, Size: 3985 bytes --]

2006-06-21  Andrew Stubbs  <andrew.stubbs@st.com>

	* gdb.base/ifelse.exp: New file.


Index: src/gdb/testsuite/gdb.base/ifelse.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/ifelse.exp	2006-06-21 11:35:04.000000000 +0100
@@ -0,0 +1,138 @@
+# Copyright 2006 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.
+
+# This test checks that the if .. else .. end construct works and may
+# contain empty bodies without crashing.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+gdb_exit
+gdb_start
+
+# First test that the if command works with an empty body
+# Test with different conditions because the body is ignored
+# if it is not executed.
+
+#    with true condition
+send_gdb "if 1\nend\necho got here\\n\n"
+gdb_expect {
+    -re ".*got here.*$gdb_prompt $" {
+        pass "if 1 with empty body"
+    }
+    eof {
+	fail "if 1 with empty body (crash)"
+	gdb_exit
+	gdb_start
+    }
+    timeout {
+	fail "if 1 with empty body (timeout)"
+	gdb_exit
+	gdb_start
+    }
+}
+
+#    with false condition
+send_gdb "if 0\nend\necho got here\\n\n"
+gdb_expect {
+    -re ".*got here.*$gdb_prompt $" {
+        pass "if 0 with empty body"
+    }
+    eof {
+	fail "if 0 with empty body (crash)"
+	gdb_exit
+	gdb_start
+    }
+    timeout {
+	fail "if 0 with empty body (timeout)"
+	gdb_exit
+	gdb_start
+    }
+}
+
+# Second, do the same tests with an empty else body.
+# This fails in GDB <=6.5
+
+# Unfortunately it was an uninitialised memory problem so
+# sometimes it just works. Preceed it with an if else end with
+# bodies and hopefully the memory with be dirty and the problem
+# will show itself (this works at time of writing).
+
+send_gdb "if 1\necho true\\n\nelse\necho false\\n\nend\n"
+
+#    with true condition
+send_gdb "if 1\nelse\nend\necho got here\\n\n"
+gdb_expect {
+    -re ".*got here.*$gdb_prompt $" {
+        pass "if 1 .. else with empty body"
+    }
+    eof {
+	fail "if 1 .. else with empty body (crash)"
+	gdb_exit
+	gdb_start
+    }
+    timeout {
+	fail "if 1 .. else with empty body (timeout)"
+	gdb_exit
+	gdb_start
+    }
+}
+
+# dirty memory
+send_gdb "if 1\necho true\\n\nelse\necho false\\n\nend\n"
+
+#    with false condition
+send_gdb "if 0\nelse\nend\necho got here\\n\n"
+gdb_expect {
+    -re ".*got here.*$gdb_prompt $" {
+        pass "if 0 .. else with empty body"
+    }
+    eof {
+	fail "if 0 .. else with empty body (crash)"
+	gdb_exit
+	gdb_start
+    }
+    timeout {
+	fail "if 0 .. else with empty body (timeout)"
+	gdb_exit
+	gdb_start
+    }
+}
+
+send_gdb "set confirm off\n"
+
+# Test that a define with an empty else can be replaced.
+# If there is memory corruption then free will fail.
+# dirty memory
+send_gdb "if 1\necho true\\n\nelse\necho false\\n\nend\n"
+# create
+send_gdb "define abc\nif 1\nelse\nend\nend\n"
+# replace
+send_gdb "define abc\necho got here\\n\nend\n"
+# call
+send_gdb "abc\n"
+gdb_expect {
+    -re ".*\[\r\n]got here\[\n\r].*$gdb_prompt $" {
+        pass "replace define with if .. else with empty body"
+    }
+    eof {
+	fail "replace define with if .. else with empty body (crash)"
+    }
+    timeout {
+	fail "replace define with if .. else with empty body (timeout)"
+    }
+}

Re: [PATCH] Fix segfault on empty else

[Daniel Jacobowitz]

On Wed, Jun 21, 2006 at 11:50:35AM +0100, Andrew STUBBS wrote:
> How about the attached? It is somewhat tricky trying to reliably 
> reproduce the problem without valgrind, as you say.
> 
> I have put in a few commands that are intended to run through the same 
> code and will, most likely, allocate memory in the same place, in order 
> to ensure that the crash occurs. This is a little hopeful, but what else 
> can I do?
> 
> This works for me, but could you please confirm that it works in your 
> setup/host (without the patch to fix the problem of course).

:REVIEWMAIL:

Works fine for me - very clever.  This patch is OK.  I'd recommend two
changes, at your discretion:

> +send_gdb "if 1\necho true\\n\nelse\necho false\\n\nend\n"

Due to the patterns you're matching, this is safe, but in general it's
bad style to use send_gdb without a matching gdb_expect.  If you use
gdb_test with the third argument empty, it will not be registered
as a "test", and no pass message will be issued - unless the test
fails, that is.  So this is useful for tests which aren't part of the
testcase, and are always assumed to pass.

> +#    with true condition
> +send_gdb "if 1\nelse\nend\necho got here\\n\n"
> +gdb_expect {

gdb_test_multiple is a little nicer here.  You can still supply your
own eof handler; the only difference is that it will pick
up other quirky bits like internal error messages (and the default
timeout handler should be fine for you here).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Fix segfault on empty else

[Andrew STUBBS]

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

Daniel Jacobowitz wrote:
> Works fine for me - very clever.  This patch is OK.  I'd recommend two
> changes, at your discretion:
> 
>> +send_gdb "if 1\necho true\\n\nelse\necho false\\n\nend\n"
> 
> Due to the patterns you're matching, this is safe, but in general it's
> bad style to use send_gdb without a matching gdb_expect.  If you use
> gdb_test with the third argument empty, it will not be registered
> as a "test", and no pass message will be issued - unless the test
> fails, that is.  So this is useful for tests which aren't part of the
> testcase, and are always assumed to pass.

I could certainly do this.

>> +#    with true condition
>> +send_gdb "if 1\nelse\nend\necho got here\\n\n"
>> +gdb_expect {
> 
> gdb_test_multiple is a little nicer here.  You can still supply your
> own eof handler; the only difference is that it will pick
> up other quirky bits like internal error messages (and the default
> timeout handler should be fine for you here).

I have tried to do this, but have hit trouble. Please find my best 
effort attached.

The problem is that it now calls perror which means that any test that 
fails due to a crash is marked UNRESOLVED, not FAIL. It also prevents 
any further tests in the file from running. So the result is that 
failures are not flagged up as clearly as they should be - you just get 
one unresolved test case instead of the proper handful of failures that 
would set the alarm bells ringing.

It does do the right thing when everything passes.

Andrew

[-- Attachment #2: ifelse.exp --]
[-- Type: text/plain, Size: 2991 bytes --]

# Copyright 2006 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.

# This test checks that the if .. else .. end construct works and may
# contain empty bodies without crashing.

if $tracelevel then {
    strace $tracelevel
}

gdb_exit
gdb_start

# First test that the if command works with an empty body
# Test with different conditions because the body is ignored
# if it is not executed.

#    with true condition
if {[gdb_test_multiple "if 1\nend\necho got here\\n" "if 1 with empty body" {
	-re ".*got here.*$gdb_prompt $" {pass "if 1 with empty body"}
     }] != 0} {
    gdb_exit
    gdb_start
}

#    with false condition
if {[gdb_test_multiple "if 0\nend\necho got here\\n" "if 0 with empty body" {
	-re ".*got here.*$gdb_prompt $" {pass "if 0 with empty body"}
     }] != 0} {
    gdb_exit
    gdb_start
}

# Second, do the same tests with an empty else body.
# This fails in GDB <=6.5

# Unfortunately it was an uninitialised memory problem so
# sometimes it just works. Preceed it with an if else end with
# bodies and hopefully the memory with be dirty and the problem
# will show itself (this works at time of writing).

gdb_test "if 1\necho true\\n\nelse\necho false\\n\nend" "true" ""

#    with true condition
if {[gdb_test_multiple "if 1\nelse\nend\necho got here\\n" \
		       "if 1 .. else with empty body" {
	-re ".*got here.*$gdb_prompt $" {pass "if 1 .. else with empty body"}
     }
] != 0} {
    gdb_exit
    gdb_start
}

# dirty memory
gdb_test "if 1\necho true\\n\nelse\necho false\\n\nend" "true" ""

#    with false condition
if {[gdb_test_multiple "if 0\nelse\nend\necho got here\\n" \
		       "if 0 .. else with empty body" {
	-re ".*got here.*$gdb_prompt $" {pass "if 0 .. else with empty body"}
     }] != 0} {
    gdb_exit
    gdb_start
}

gdb_test "set confirm off" "" ""

# Test that a define with an empty else can be replaced.
# If there is memory corruption then free will fail.
# dirty memory
gdb_test "if 1\necho true\\n\nelse\necho false\\n\nend" "true" ""
# create
gdb_test "define abc\nif 1\nelse\nend\nend" "" "create define with empty else"
# call
gdb_test "abc" "" "call original define"
# replace
gdb_test "define abc\necho got here\\n\nend" "" \
	 "replace define with if .. else with empty body"
# call
gdb_test "abc" "got here" "call replacement define"

Re: [PATCH] Fix segfault on empty else

[Daniel Jacobowitz]

On Thu, Jul 06, 2006 at 05:18:46PM +0100, Andrew STUBBS wrote:
> #    with true condition
> if {[gdb_test_multiple "if 1\nend\necho got here\\n" "if 1 with empty body" {
> 	-re ".*got here.*$gdb_prompt $" {pass "if 1 with empty body"}
>      }] != 0} {
>     gdb_exit
>     gdb_start
> }

Does this work better?

set message "if 1 with empty body"
gdb_test_multiple "if 1\nend\necho got here\n" $message {
  -re "got here.*$gdb_prompt $" {
    pass $message
  }
  eof {
    fail $message
    gdb_exit
    gdb_start
  }
}


-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Fix segfault on empty else

[Andrew STUBBS]

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

Daniel Jacobowitz wrote:
> Does this work better?

Yes much better, thanks. It now runs all the tests, no matter what, and 
reports crashes as failures. The only exception is the very last one, 
which really is unresolved following the failure of the previous test.

How about the attached patch then?

:ADDPATCH testsuite:

Andrew Stubbs

[-- Attachment #2: empty-else-test.patch --]
[-- Type: text/plain, Size: 3652 bytes --]

2006-07-07  Andrew Stubbs  <andrew.stubbs@st.com>

	* gdb.base/ifelse.exp: New file.


Index: src/gdb/testsuite/gdb.base/ifelse.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/ifelse.exp	2006-07-07 11:59:28.000000000 +0100
@@ -0,0 +1,105 @@
+# Copyright 2006 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.
+
+# This test checks that the if .. else .. end construct works and may
+# contain empty bodies without crashing.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+gdb_exit
+gdb_start
+
+# First test that the if command works with an empty body
+# Test with different conditions because the body is ignored
+# if it is not executed.
+
+#    with true condition
+set message "if 1 with empty body"
+gdb_test_multiple "if 1\nend\necho got here\\n" $message {
+    -re ".*got here.*$gdb_prompt $" {pass $message}
+    eof {
+	fail "$message (crashed)"
+	gdb_exit
+	gdb_start
+    }
+}
+
+#    with false condition
+set message "if 0 with empty body"
+gdb_test_multiple "if 0\nend\necho got here\\n" $message {
+    -re ".*got here.*$gdb_prompt $" {pass $message}
+    eof {
+	fail "$message (crashed)"
+	gdb_exit
+	gdb_start
+    }
+}
+
+# Second, do the same tests with an empty else body.
+# This fails in GDB <=6.5
+
+# Unfortunately it was an uninitialised memory problem so
+# sometimes it just works. Preceed it with an if else end with
+# bodies and hopefully the memory with be dirty and the problem
+# will show itself (this works at time of writing).
+
+gdb_test "if 1\necho true\\n\nelse\necho false\\n\nend" "true" ""
+
+#    with true condition
+set message "if 1 .. else with empty body"
+gdb_test_multiple "if 1\nelse\nend\necho got here\\n" $message {
+    -re ".*got here.*$gdb_prompt $" {pass $message}
+    eof {
+	fail "$message (crashed)"
+	gdb_exit
+	gdb_start
+    }
+}
+
+# dirty memory
+gdb_test "if 1\necho true\\n\nelse\necho false\\n\nend" "true" ""
+
+#    with false condition
+set message "if 0 .. else with empty body"
+gdb_test_multiple "if 0\nelse\nend\necho got here\\n" $message {
+    -re ".*got here.*$gdb_prompt $" {pass $message}
+    eof {
+	fail "$message (crashed)"
+	gdb_exit
+	gdb_start
+    }
+}
+
+gdb_test "set confirm off" "" ""
+
+# Test that a define with an empty else can be replaced.
+# If there is memory corruption then free will fail.
+# dirty memory
+gdb_test "if 1\necho true\\n\nelse\necho false\\n\nend" "true" ""
+# create
+gdb_test "define abc\nif 1\nelse\nend\nend" "" "create define with empty else"
+# call (note that condition is 1 so should pass)
+gdb_test "abc" "" "call original define"
+# replace
+set message "replace define with if .. else with empty body"
+gdb_test_multiple "define abc\necho got here\\n\nend" $message {
+    -re "$gdb_prompt $" {pass $message}
+    eof {fail "$message (crashed)"}
+}
+# call
+gdb_test "abc" "got here" "call replacement define"

Re: [PATCH] Fix segfault on empty else

[Daniel Jacobowitz]

On Fri, Jul 07, 2006 at 12:07:15PM +0100, Andrew STUBBS wrote:
> Daniel Jacobowitz wrote:
> >Does this work better?
> 
> Yes much better, thanks. It now runs all the tests, no matter what, and 
> reports crashes as failures. The only exception is the very last one, 
> which really is unresolved following the failure of the previous test.
> 
> How about the attached patch then?

:REVIEWMAIL:

Yeah, this is OK.  Thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Fix segfault on empty else

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> Yeah, this is OK.  Thanks.

Excellent, committed thanks.

Andrew