diff mbox

Fix use-after-free in record_btrace_start_replaying

Message ID 20181019185840.532-1-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey Oct. 19, 2018, 6:58 p.m. UTC 
  -fsanitize=address showed a use-after-free in
record_btrace_start_replaying.  The bug occurred because
get_thread_current_frame returned a frame_info, but this object was
then invalidated before the return by ~scoped_restore_current_thread.

This patch fixes the problem by renaming get_thread_current_frame and
having it return a frame id.

gdb/ChangeLog
2018-10-19  Tom Tromey  <tom@tromey.com>

	* record-btrace.c (get_thread_current_frame_id): Rename from
	get_thread_current_frame.  Return a frame_id.
	(record_btrace_start_replaying): Update.
---
 gdb/ChangeLog       |  6 ++++++
 gdb/record-btrace.c | 19 ++++++++-----------
 2 files changed, 14 insertions(+), 11 deletions(-)

Comments

Kevin Buettner Oct. 20, 2018, 9:01 p.m. UTC | #1 
On Fri, 19 Oct 2018 12:58:40 -0600
Tom Tromey <tom@tromey.com> wrote:

> -fsanitize=address showed a use-after-free in
> record_btrace_start_replaying.  The bug occurred because
> get_thread_current_frame returned a frame_info, but this object was
> then invalidated before the return by ~scoped_restore_current_thread.
> 
> This patch fixes the problem by renaming get_thread_current_frame and
> having it return a frame id.
> 
> gdb/ChangeLog
> 2018-10-19  Tom Tromey  <tom@tromey.com>
> 
> 	* record-btrace.c (get_thread_current_frame_id): Rename from
> 	get_thread_current_frame.  Return a frame_id.
> 	(record_btrace_start_replaying): Update.

LGTM.

Kevin
Metzger, Markus T Oct. 22, 2018, 6:46 a.m. UTC | #2 
> -fsanitize=address showed a use-after-free in record_btrace_start_replaying.
> The bug occurred because get_thread_current_frame returned a frame_info,
> but this object was then invalidated before the return by
> ~scoped_restore_current_thread.
> 
> This patch fixes the problem by renaming get_thread_current_frame and having
> it return a frame id.
> 
> gdb/ChangeLog
> 2018-10-19  Tom Tromey  <tom@tromey.com>
> 
> 	* record-btrace.c (get_thread_current_frame_id): Rename from
> 	get_thread_current_frame.  Return a frame_id.
> 	(record_btrace_start_replaying): Update.

Looks good to me.

Thanks,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 483b00e32a1..c9662d7ae4b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@ 
+2018-10-19  Tom Tromey  <tom@tromey.com>
+
+	* record-btrace.c (get_thread_current_frame_id): Rename from
+	get_thread_current_frame.  Return a frame_id.
+	(record_btrace_start_replaying): Update.
+
 2018-10-19  Tom Tromey  <tom@tromey.com>
 
 	* symfile.c (reread_symbols): Clear "static_links".
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index aabe9f5e27e..c0e3341a1f6 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1967,10 +1967,10 @@  record_btrace_resume_thread (struct thread_info *tp,
 
 /* Get the current frame for TP.  */
 
-static struct frame_info *
-get_thread_current_frame (struct thread_info *tp)
+static struct frame_id
+get_thread_current_frame_id (struct thread_info *tp)
 {
-  struct frame_info *frame;
+  struct frame_id id;
   int executing;
 
   /* Set current thread, which is implicitly used by
@@ -1989,10 +1989,10 @@  get_thread_current_frame (struct thread_info *tp)
   executing = tp->executing;
   set_executing (inferior_ptid, false);
 
-  frame = NULL;
+  id = null_frame_id;
   TRY
     {
-      frame = get_current_frame ();
+      id = get_frame_id (get_current_frame ());
     }
   CATCH (except, RETURN_MASK_ALL)
     {
@@ -2006,7 +2006,7 @@  get_thread_current_frame (struct thread_info *tp)
   /* Restore the previous execution state.  */
   set_executing (inferior_ptid, executing);
 
-  return frame;
+  return id;
 }
 
 /* Start replaying a thread.  */
@@ -2031,13 +2031,11 @@  record_btrace_start_replaying (struct thread_info *tp)
      subroutines after we started replaying.  */
   TRY
     {
-      struct frame_info *frame;
       struct frame_id frame_id;
       int upd_step_frame_id, upd_step_stack_frame_id;
 
       /* The current frame without replaying - computed via normal unwind.  */
-      frame = get_thread_current_frame (tp);
-      frame_id = get_frame_id (frame);
+      frame_id = get_thread_current_frame_id (tp);
 
       /* Check if we need to update any stepping-related frame id's.  */
       upd_step_frame_id = frame_id_eq (frame_id,
@@ -2068,8 +2066,7 @@  record_btrace_start_replaying (struct thread_info *tp)
       registers_changed_thread (tp);
 
       /* The current frame with replaying - computed via btrace unwind.  */
-      frame = get_thread_current_frame (tp);
-      frame_id = get_frame_id (frame);
+      frame_id = get_thread_current_frame_id (tp);
 
       /* Replace stepping related frames where necessary.  */
       if (upd_step_frame_id)