rawhide's gdb segfaults, w/patch

rawhide's gdb segfaults, w/patch

[Jim Meyering]

Hi,

I was looking at http://bugzilla.redhat.com/365111 and did this
on a rawhide x86_64 system:

  $ printf '#include <stdio.h>\nint main(){printf("foo");return 0;}\n' > k.c
  $ gdb -q a.out
  (gdb) b printf
  Breakpoint 1 at 0x4003a0
  (gdb) r
  Starting program: /t/a.out

  Breakpoint 1, __printf (format=0x4005bc "foo") at printf.c:30
  30      {
  (gdb) b mmap64
  Breakpoint 2 at 0x3f742e2ba0
  (gdb) c
  Continuing.

  Breakpoint 2, 0x0000003f742e2ba0 in mmap64 () from /lib64/libc.so.6
  (gdb) ret (void*)-1
  foozsh: segmentation fault  gdb a.out

Debugging the debugger suggests it's due to a NULL dereference:
you can't apply SYMBOL_TYPE to a NULL pointer:

    gdb/symtab.h:#define SYMBOL_TYPE(symbol) (symbol)->type

  $ gdb -q --args gdb -q a.out
  (gdb) r                                                                          Starting program: /usr/bin/gdb -q a.out
  warning: the debug information found in "/usr/lib/debug//lib64/libselinux.so.1.debug" does not match "/lib64/libselinux.so.1" (CRC mismatch).

  warning: the debug information found in "/usr/lib/debug/lib64/libselinux.so.1.debug" does not match "/lib64/libselinux.so.1" (CRC mismatch).

  [Thread debugging using libthread_db enabled]
  (gdb) b printf
  Breakpoint 1 at 0x4003a0
  (gdb) r
  Starting program: /t/a.out
  Detaching after fork from child process 4853.
  Detaching after fork from child process 4854.

  Breakpoint 1, __printf (format=0x4005bc "foo") at printf.c:30
  30      {
  (gdb) b mmap64
  Breakpoint 2 at 0x3f742e2ba0
  (gdb) c
  Continuing.

  Breakpoint 2, 0x0000003f742e2ba0 in mmap64 () from /lib64/libc.so.6
  (gdb) ret (void*)-1

  Program received signal SIGSEGV, Segmentation fault.
  return_command (retval_exp=0xbe8644 "(void*)-1", from_tty=1)
      at ../../gdb/stack.c:1878
  1878          else if (using_struct_return (SYMBOL_TYPE (thisfun), return_type))
  (gdb) bt
  #0  return_command (retval_exp=0xbe8644 "(void*)-1", from_tty=1)
      at ../../gdb/stack.c:1878
  #1  0x0000000000448daa in execute_command (p=0xbe864c "1", from_tty=1)
      at ../../gdb/top.c:457
  #2  0x00000000004feeb7 in command_handler (command=0xbe8640 "ret (void*)-1")
      at ../../gdb/event-top.c:519
  #3  0x00000000004ffbac in command_line_handler (rl=<value optimized out>)
      at ../../gdb/event-top.c:744
  #4  0x0000003248e27e7e in rl_callback_read_char () at ../callback.c:205
  #5  0x00000000004ff009 in rl_callback_read_char_wrapper (client_data=0xcd9d40)
      at ../../gdb/event-top.c:179
  #6  0x00000000004fd8a8 in process_event () at ../../gdb/event-loop.c:394
  #7  0x00000000004feb3b in gdb_do_one_event (data=<value optimized out>)
      at ../../gdb/event-loop.c:459
  #8  0x00000000004f8af4 in catch_errors (func=0x4fe8b0 <gdb_do_one_event>,
      func_args=0x0, errstring=0x66750c "", mask=<value optimized out>)
      at ../../gdb/exceptions.c:516
  #9  0x0000000000497e78 in tui_command_loop (data=<value optimized out>)
      at ../../gdb/tui/tui-interp.c:156
  #10 0x00000000004412c9 in captured_command_loop (data=0xcd9d40)
      at ../../gdb/main.c:99
  #11 0x00000000004f8af4 in catch_errors (func=0x4412c0 <captured_command_loop>,
      func_args=0x0, errstring=0x66750c "", mask=<value optimized out>)
      at ../../gdb/exceptions.c:516
  #12 0x0000000000441dee in captured_main (data=<value optimized out>)
      at ../../gdb/main.c:838
  #13 0x00000000004f8af4 in catch_errors (func=0x441300 <captured_main>,
      func_args=0x7fffffffe490, errstring=0x66750c "", mask=<value optimized out>)
      at ../../gdb/exceptions.c:516
  #14 0x00000000004412b4 in gdb_main (args=<value optimized out>)
      at ../../gdb/main.c:847
  #15 0x0000000000441282 in main (argc=<value optimized out>, argv=0x4)
      at ../../gdb/gdb.c:47
  (gdb) p thisfun
  $1 = (struct symbol *) 0x0
  (gdb) p return_type
  $2 = (struct type *) 0xc81f20


Here's an untested and quite possibly-wrong patch.
I.e., if the warning should be given even when "thisfun" is NULL,
it would have to be different.

From f092f666efa15a6451c8549f7cdc5f791ae838ed Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Sun, 28 Dec 2008 18:03:39 +0100
Subject: [PATCH] avoid NULL dereference

* stack.c (return_command): Guard use of SYMBOL_TYPE (thisfun).
---
 gdb/ChangeLog |    5 +++++
 gdb/stack.c   |    3 ++-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index aa64ed3..4eac798 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2008-12-28  Jim Meyering  <meyering@redhat.com>
+
+	avoid NULL dereference
+	* stack.c (return_command): Guard use of SYMBOL_TYPE (thisfun).
+
 2008-12-28  Pedro Alves  <pedro@codesourcery.com>

 	* linux-fork.c (linux_fork_detach): New.
diff --git a/gdb/stack.c b/gdb/stack.c
index 51dd1bc..7ff58b1 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1823,7 +1823,8 @@ return_command (char *retval_exp, int from_tty)
            is discarded, side effects such as "return i++" still
            occur.  */
 	return_value = NULL;
-      else if (using_struct_return (SYMBOL_TYPE (thisfun), return_type))
+      else if (thisfun != NULL
+	       && using_struct_return (SYMBOL_TYPE (thisfun), return_type))
 	{
 	  query_prefix = "\
 The location at which to store the function's return value is unknown.\n\
--
1.6.1.302.gccd4d

Re: rawhide's gdb segfaults, w/patch

[Jan Kratochvil]

Hi Jim,

thanks but your patch needed a second part.  It is a regression from:
http://sourceware.org/ml/gdb-cvs/2008-04/msg00136.html

On Sun, 28 Dec 2008 18:30:14 +0100, Jim Meyering wrote:
> Here's an untested and quite possibly-wrong patch.
> I.e., if the warning should be given even when "thisfun" is NULL,
> it would have to be different.

I find it right as gdbarch_return_value_ftype even has a comment:
FUNCTYPE may be NULL in which case the return convention is computed based
only on VALTYPE.

This new argument and functype|func_type is only used in sh-tdep.c
http://sourceware.org/ml/gdb-patches/2008-04/msg00277.html
and even there it can be safely NULL.

This new argument and functype|func_type was added by
http://sourceware.org/ml/gdb-patches/2008-04/msg00276.html


I find just questionable whether the new testcase should be in
gdb.base/nodebug.exp or gdb.base/return*.exp but may be any way is OK.


Thanks,
Jan


gdb/
2009-01-09  Jim Meyering  <meyering@redhat.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	Avoid NULL dereference.
	* stack.c (return_command): Guard use of SYMBOL_TYPE (thisfun).
	New variable func_type.

gdb/testsuite/
2009-01-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/nodebug.exp (return from function with no debug info): New.

--- gdb/stack.c	3 Jan 2009 05:57:53 -0000	1.183
+++ gdb/stack.c	9 Jan 2009 20:30:25 -0000
@@ -1823,7 +1823,8 @@ return_command (char *retval_exp, int fr
            is discarded, side effects such as "return i++" still
            occur.  */
 	return_value = NULL;
-      else if (using_struct_return (SYMBOL_TYPE (thisfun), return_type))
+      else if (thisfun != NULL
+	       && using_struct_return (SYMBOL_TYPE (thisfun), return_type))
 	{
 	  query_prefix = "\
 The location at which to store the function's return value is unknown.\n\
@@ -1856,10 +1857,12 @@ If you continue, the return value that y
     {
       struct type *return_type = value_type (return_value);
       struct gdbarch *gdbarch = get_regcache_arch (get_current_regcache ());
-      gdb_assert (gdbarch_return_value (gdbarch, SYMBOL_TYPE (thisfun),
-      					return_type, NULL, NULL, NULL)
+      struct type *func_type = thisfun == NULL ? NULL : SYMBOL_TYPE (thisfun);
+
+      gdb_assert (gdbarch_return_value (gdbarch, func_type, return_type, NULL,
+					NULL, NULL)
 		  == RETURN_VALUE_REGISTER_CONVENTION);
-      gdbarch_return_value (gdbarch, SYMBOL_TYPE (thisfun), return_type,
+      gdbarch_return_value (gdbarch, func_type, return_type,
 			    get_current_regcache (), NULL /*read*/,
 			    value_contents (return_value) /*write*/);
     }
--- gdb/testsuite/gdb.base/nodebug.exp	3 Jan 2009 05:58:03 -0000	1.10
+++ gdb/testsuite/gdb.base/nodebug.exp	9 Jan 2009 20:30:26 -0000
@@ -215,5 +215,12 @@ if [runto inner] then {
     if [runto middle] then {
 	gdb_test "backtrace 10" "#0.*middle.*#1.*top.*#2.*main.*" \
 	    "backtrace from middle in nodebug.exp"
+
+	# Test return from a function with no debug info (symbol; still it may
+	# have a minimal-symbol).  In gdb.base/return*.exp we would need to
+	# build a separate executable with no "debug" option.
+	gdb_test "return 0" "#0 .* top \\(.*"				     \
+		 "return from function with no debug info"		     \
+		 "Make selected stack frame return now\\? \\(y or n\\) " "y"
     }
 }

Re: rawhide's gdb segfaults, w/patch

[Jim Meyering]

Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> thanks but your patch needed a second part.  It is a regression from:
> http://sourceware.org/ml/gdb-cvs/2008-04/msg00136.html
>
> On Sun, 28 Dec 2008 18:30:14 +0100, Jim Meyering wrote:
>> Here's an untested and quite possibly-wrong patch.
>> I.e., if the warning should be given even when "thisfun" is NULL,
>> it would have to be different.
>
> I find it right as gdbarch_return_value_ftype even has a comment:
> FUNCTYPE may be NULL in which case the return convention is computed based
> only on VALTYPE.
>
> This new argument and functype|func_type is only used in sh-tdep.c
> http://sourceware.org/ml/gdb-patches/2008-04/msg00277.html
> and even there it can be safely NULL.

Hi Jan,

Thanks for dealing with that!

Re: rawhide's gdb segfaults, w/patch

[Jim Meyering]

Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> thanks but your patch needed a second part.  It is a regression from:
> http://sourceware.org/ml/gdb-cvs/2008-04/msg00136.html
>
> On Sun, 28 Dec 2008 18:30:14 +0100, Jim Meyering wrote:
>> Here's an untested and quite possibly-wrong patch.
>> I.e., if the warning should be given even when "thisfun" is NULL,
>> it would have to be different.
>
> I find it right as gdbarch_return_value_ftype even has a comment:
> FUNCTYPE may be NULL in which case the return convention is computed based
> only on VALTYPE.
>
> This new argument and functype|func_type is only used in sh-tdep.c
> http://sourceware.org/ml/gdb-patches/2008-04/msg00277.html
> and even there it can be safely NULL.
>
> This new argument and functype|func_type was added by
> http://sourceware.org/ml/gdb-patches/2008-04/msg00276.html
>
>
> I find just questionable whether the new testcase should be in
> gdb.base/nodebug.exp or gdb.base/return*.exp but may be any way is OK.

Thanks again.
This bug still affects rawhide's gdb-6.8.50.20081214-1.fc11.x86_64.
Is there anything I can do to help get the patch in?

Re: rawhide's gdb segfaults, w/patch

[Jan Kratochvil]

On Sun, 18 Jan 2009 20:04:44 +0100, Jim Meyering wrote:
> This bug still affects rawhide's gdb-6.8.50.20081214-1.fc11.x86_64.

Sorry I have not made an update with the fix so far.  But please note Rawhide
GDB is offtopic for <gdb-patches@sourceware.org>.


Regards,
Jan

Re: rawhide's gdb segfaults, w/patch

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> gdb/
Jan> 2009-01-09  Jim Meyering  <meyering@redhat.com>
Jan> 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	Avoid NULL dereference.
Jan> 	* stack.c (return_command): Guard use of SYMBOL_TYPE (thisfun).
Jan> 	New variable func_type.
Jan> gdb/testsuite/
Jan> 2009-01-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	* gdb.base/nodebug.exp (return from function with no debug info): New.

I looked at this and I think it is ok.  Please check it in.  Thanks.

Tom

Re: rawhide's gdb segfaults, w/patch

[Jan Kratochvil]

On Mon, 02 Feb 2009 19:43:59 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> gdb/
> Jan> 2009-01-09  Jim Meyering  <meyering@redhat.com>
> Jan> 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
> Jan> 	Avoid NULL dereference.
> Jan> 	* stack.c (return_command): Guard use of SYMBOL_TYPE (thisfun).
> Jan> 	New variable func_type.
> 
> I looked at this and I think it is ok.  Please check it in.  Thanks.

Checked-in (this fix, not the testcase).
	http://sourceware.org/ml/gdb-cvs/2009-02/msg00069.html


> Jan> gdb/testsuite/
> Jan> 2009-01-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
> Jan> 	* gdb.base/nodebug.exp (return from function with no debug info): New.

Posted another fix + testcase which covers even the case above:
	http://sourceware.org/ml/gdb-patches/2009-02/msg00243.html


Thanks,
Jan