Process record: Fix null deref when loading empty core file

Message ID 1526075698-20880-1-git-send-email-modchipv12@gmail.com
State New, archived
Headers

Commit Message

Andrew D'Addesio May 11, 2018, 9:54 p.m. UTC
  Fix a null dereference in the "record full restore" command. If the
supplied file contains no records, the arch list will be empty, so
no need to copy to the record list.

Also remove a redundant "record_full_arch_list_tail->next = NULL;"
assignment, as our arch list is already non-circular by design.

gdb/ChangeLog:
2018-05-11  Andrew D'Addesio  <modchipv12@gmail.com>

	* record-full.c (record_full_restore): Avoid null deref when
	appending the arch list to the record list.
---
 gdb/record-full.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
  

Comments

Andrew D'Addesio May 11, 2018, 10 p.m. UTC | #1
Here's an explanation of the bug:

Description:

gdb segfaults (null dereference) if the user attempts to run the command
"record full restore" on a file containing an empty execution history
log.

Steps to reproduce the bug:

1. Compile the following hello world using:
gcc -Wall -nostartfiles -o helloworld helloworld.S

    #include <asm/unistd.h>

    .intel_syntax noprefix
    .global _start

    .data
    msg:
        .ascii "hello, world!\n"
    msg_end:

    .text
    _start:
        mov rax, __NR_write
        mov rdi, 1 # STDOUT_FILENO
        lea rsi, [rip + msg]
        mov rdx, (msg_end - msg)
        syscall
        mov rax, __NR_exit
        mov rdi, 0 # EXIT_SUCCESS
        syscall

2. Launch gdb using: gdb ./helloworld

3. Execute these commands:

    break _start
    run
    record full

    # Uncommenting the next line prevents the crash:
    # stepi

    record save foo.log
    record stop
    record full restore foo.log # Segfault!

gdb terminates with the following segfault:

Program received signal SIGSEGV, Segmentation fault.
0x00000000007ff697 in record_full_restore () at
../../binutils-gdb/gdb/record-full.c:2491
2491      record_full_arch_list_head->prev = &record_full_first;
(gdb) backtrace
#0  0x00000000007ff697 in record_full_restore () at
../../binutils-gdb/gdb/record-full.c:2491
#1  0x00000000007fcade in record_full_core_open_1 (name=0x1e5af34
"foo.log", from_tty=1) at ../../binutils-gdb/gdb/record-full.c:940
#2  0x00000000007fcc3a in record_full_open (name=0x1e5af34 "foo.log",
from_tty=1) at ../../binutils-gdb/gdb/record-full.c:984
#3  0x00000000007ffbb3 in cmd_record_full_restore (args=0x1e5af34
"foo.log", from_tty=1) at ../../binutils-gdb/gdb/record-full.c:2532
#4  0x000000000048078c in do_const_cfunc (c=0x1d55830, args=0x1e5af34
"foo.log", from_tty=1) at ../../binutils-gdb/gdb/cli/cli-decode.c:106
#5  0x0000000000483884 in cmd_func (cmd=0x1d55830, args=0x1e5af34
"foo.log", from_tty=1) at ../../binutils-gdb/gdb/cli/cli-decode.c:1857
#6  0x00000000008b094f in execute_command (p=0x1e5af3a "g",
from_tty=1) at ../../binutils-gdb/gdb/top.c:630
#7  0x000000000070288c in command_handler (command=0x1e5af20 "") at
../../binutils-gdb/gdb/event-top.c:583
#8  0x0000000000702c90 in command_line_handler (rl=0x1e39190 "record
full restore foo.log") at ../../binutils-gdb/gdb/event-top.c:774
#9  0x0000000000702020 in gdb_rl_callback_handler (rl=0x1e39190
"record full restore foo.log") at
../../binutils-gdb/gdb/event-top.c:213
#10 0x00007fbda62b76f5 in rl_callback_read_char () from
/lib/x86_64-linux-gnu/libreadline.so.6
#11 0x0000000000701f0c in gdb_rl_callback_read_char_wrapper_noexcept
() at ../../binutils-gdb/gdb/event-top.c:175
#12 0x0000000000701f8b in gdb_rl_callback_read_char_wrapper
(client_data=0x1b63420) at ../../binutils-gdb/gdb/event-top.c:192
#13 0x0000000000702720 in stdin_event_handler (error=0,
client_data=0x1b63420) at ../../binutils-gdb/gdb/event-top.c:511
#14 0x00000000007007ed in handle_file_event (file_ptr=0x1e63d50,
ready_mask=1) at ../../binutils-gdb/gdb/event-loop.c:733
#15 0x0000000000700d9f in gdb_wait_for_event (block=1) at
../../binutils-gdb/gdb/event-loop.c:859
#16 0x00000000006ffbc7 in gdb_do_one_event () at
../../binutils-gdb/gdb/event-loop.c:347
#17 0x00000000006ffc0e in start_event_loop () at
../../binutils-gdb/gdb/event-loop.c:371
#18 0x0000000000793909 in captured_command_loop () at
../../binutils-gdb/gdb/main.c:330
#19 0x0000000000794def in captured_main (data=0x7fffc2ce81e0) at
../../binutils-gdb/gdb/main.c:1157
#20 0x0000000000794ec4 in gdb_main (args=0x7fffc2ce81e0) at
../../binutils-gdb/gdb/main.c:1173
#21 0x000000000040df4c in main (argc=2, argv=0x7fffc2ce82e8) at
../../binutils-gdb/gdb/gdb.c:32

This patch fixes the segfault. I also attached a passing testsuite.

Andrew
1c1
< Test Run By daddesio on Fri May 11 16:16:55 2018
---
> Test Run By daddesio on Fri May 11 16:29:35 2018
45265c45265
< FAIL: gdb.mi/list-thread-groups-available.exp: list available thread groups (unexpected output)
---
> PASS: gdb.mi/list-thread-groups-available.exp: list available thread groups
48105c48105
< FAIL: gdb.multi/multi-arch-exec.exp: first_arch=1: selected_thread=1: follow_exec_mode=same: continue across exec that changes architecture
---
> FAIL: gdb.multi/multi-arch-exec.exp: first_arch=1: selected_thread=1: follow_exec_mode=same: continue across exec that changes architecture (timeout)
56084c56084
< PASS: gdb.threads/non-ldr-exit.exp: program exits normally
---
> KFAIL: gdb.threads/non-ldr-exit.exp: program exits normally (PRMS: gdb/18717)
56703,56704c56703
< PASS: gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=1: inferior 1 exited
< PASS: gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=1: no threads left
---
> KFAIL: gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=1: inferior 1 exited (memory error) (PRMS: gdb/18749)
56708,56709c56707
< PASS: gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=0: inferior 1 exited
< PASS: gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=0: no threads left
---
> KFAIL: gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=0: inferior 1 exited (memory error) (PRMS: gdb/18749)
56718,56719c56716
< PASS: gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=0: inferior 1 exited
< PASS: gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=0: no threads left
---
> KFAIL: gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=0: inferior 1 exited (prompt) (PRMS: gdb/18749)
57621c57618
< PASS: gdb.threads/thread-specific-bp.exp: non-stop: thread-specific breakpoint was deleted
---
> FAIL: gdb.threads/thread-specific-bp.exp: non-stop: thread-specific breakpoint was deleted (timeout)
58382c58379
< # of expected passes		54575
---
> # of expected passes		54568
58386c58383
< # of known failures		62
---
> # of known failures		66
  
Joel Brobecker May 30, 2018, 11:25 p.m. UTC | #2
Hi Andrew,

The patch you sent seems reasonable to me, and I'd be ready to
approve it. But the information you sent below is extremely useful
for understanding the conditions necessary to reproduce the issue
you are fixing. This is both useful for understanding and reviewing
your patch, but also for others who might either find this thread,
or else be looking at your patch, and wondering what you were trying
to do, when you made your change.

With that in mind, can you create a commit which contains the
explanation you just gave in the revision log, followed by whatever
explanation you feel is necessary to provide as to how you fixed
the problem, followed by the ChangeLog entry and a note of how
the fix was tested (which platform, which tests that were a FAIL
now became a PASS).

Browsing the gdb-patches mailing-list for example of how others
submit patches should give you a fair idea of what we prefer.

Do not hesitate to Cc: me on the resubmission of this specific patch,
and I will make every effort to review promptly.

Thank you!

On Fri, May 11, 2018 at 05:00:29PM -0500, Andrew D'Addesio wrote:

> Here's an explanation of the bug:
> 
> Description:
[snip]
  

Patch

diff --git a/gdb/record-full.c b/gdb/record-full.c
index 79f5c0f..edd30fb 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -2486,11 +2486,13 @@  record_full_restore (void)
 
   discard_cleanups (old_cleanups);
 
-  /* Add record_full_arch_list_head to the end of record list.  */
-  record_full_first.next = record_full_arch_list_head;
-  record_full_arch_list_head->prev = &record_full_first;
-  record_full_arch_list_tail->next = NULL;
-  record_full_list = &record_full_first;
+  /* Append the arch list to the record list.  */
+  if (record_full_arch_list_head != NULL)
+    {
+      record_full_first.next = record_full_arch_list_head;
+      record_full_arch_list_head->prev = &record_full_first;
+      record_full_list = &record_full_first;
+    }
 
   /* Update record_full_insn_max_num.  */
   if (record_full_insn_num > record_full_insn_max_num)