[PATCH] gdb/record-full: Use xmalloc instead of alloca for large buffers.

[PATCH] gdb/record-full: Use xmalloc instead of alloca for large buffers.

[Marcin Kościelnicki]

On the newly added s390 target, it's possible for a single instruction
to write practically unbounded amount of memory (eg. MVCLE).  This caused
a stack overflow when alloca was used.

gdb/ChangeLog:

	* record-full.c (record_full_exec_insn): Use xmalloc for large buffers.
---
 gdb/ChangeLog     |  4 ++++
 gdb/record-full.c | 11 ++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 38a42ea..f4c0f57 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2015-11-02  Marcin Kościelnicki  <koriakin@0x04.net>
+
+	* record-full.c (record_full_exec_insn): Use xmalloc for large buffers.
+
 2015-10-30  Pedro Alves  <palves@redhat.com>
 
 	* breakpoint.c (breakpoint_in_range_p)
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 595e357..04d64ba 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -65,6 +65,8 @@
 
 #define RECORD_FULL_FILE_MAGIC	netorder32(0x20091016)
 
+#define RECORD_MEMORY_XMALLOC_THRESHOLD 0x1000
+
 /* These are the core structs of the process record functionality.
 
    A record_full_entry is a record of the value change of a register
@@ -726,7 +728,11 @@ record_full_exec_insn (struct regcache *regcache,
 	/* Nothing to do if the entry is flagged not_accessible.  */
         if (!entry->u.mem.mem_entry_not_accessible)
           {
-            gdb_byte *mem = (gdb_byte *) alloca (entry->u.mem.len);
+            gdb_byte *mem;
+	    if (entry->u.mem.len >= RECORD_MEMORY_XMALLOC_THRESHOLD)
+	      mem = (gdb_byte *) xmalloc (entry->u.mem.len);
+	    else
+	      mem = (gdb_byte *) alloca (entry->u.mem.len);
 
             if (record_debug > 1)
               fprintf_unfiltered (gdb_stdlog,
@@ -771,6 +777,9 @@ record_full_exec_insn (struct regcache *regcache,
 		      record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT;
 		  }
               }
+
+	    if (entry->u.mem.len >= RECORD_MEMORY_XMALLOC_THRESHOLD)
+	      xfree(mem);
           }
       }
       break;
-- 
2.6.2

Re: [PATCH] gdb/record-full: Use xmalloc instead of alloca for large buffers.

[Ulrich Weigand]

Marcin KoÅcielnicki <koriakin at 0x04 dot net> wrote:

> On the newly added s390 target, it's possible for a single instruction
> to write practically unbounded amount of memory (eg. MVCLE).  This caused
> a stack overflow when alloca was used.
> 
> gdb/ChangeLog:
> 
> 	* record-full.c (record_full_exec_insn): Use xmalloc for large buffers.

I think this may leak memory if some code between the xmalloc and the xfree
throws a GDB exception.  Usually, this is protected against by calling the
xfree via the make_cleanup mechanism ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] gdb/record-full: Use xmalloc instead of alloca for large buffers.

[Joel Brobecker]

> > gdb/ChangeLog:
> > 
> > 	* record-full.c (record_full_exec_insn): Use xmalloc for large buffers.
> 
> I think this may leak memory if some code between the xmalloc and the xfree
> throws a GDB exception.  Usually, this is protected against by calling the
> xfree via the make_cleanup mechanism ...

Also, why not just call xmalloc every time instead of doing
a combination of alloca and xmalloc? I don't see enough benefits
to justify the extra complication.

-- 
Joel

[PATCH v2] gdb/record-full: Use xmalloc instead of alloca for temporary memory storage.

[Marcin Kościelnicki]

On the newly added s390 target, it's possible for a single instruction
to write practically unbounded amount of memory (eg. MVCLE).  This caused
a stack overflow when alloca was used.

gdb/ChangeLog:

	* record-full.c (record_full_exec_insn): Use xmalloc for temporary
	memory storage.
---
Done and done.

 gdb/ChangeLog     | 5 +++++
 gdb/record-full.c | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 532535d..5b20fa2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2015-11-03  Marcin Kościelnicki  <koriakin@0x04.net>
 
+	* record-full.c (record_full_exec_insn): Use xmalloc for temporary
+	memory storage.
+
+2015-11-03  Marcin Kościelnicki  <koriakin@0x04.net>
+
 	* MAINTAINERS (Write After Approval): Add Marcin Kościelnicki.
 
 2015-10-30  Pedro Alves  <palves@redhat.com>
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 595e357..03b3d41 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -726,7 +726,8 @@ record_full_exec_insn (struct regcache *regcache,
 	/* Nothing to do if the entry is flagged not_accessible.  */
         if (!entry->u.mem.mem_entry_not_accessible)
           {
-            gdb_byte *mem = (gdb_byte *) alloca (entry->u.mem.len);
+            gdb_byte *mem = (gdb_byte *) xmalloc (entry->u.mem.len);
+            struct cleanup *cleanup = make_cleanup (xfree, mem);
 
             if (record_debug > 1)
               fprintf_unfiltered (gdb_stdlog,
@@ -771,6 +772,8 @@ record_full_exec_insn (struct regcache *regcache,
 		      record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT;
 		  }
               }
+
+	    do_cleanups (cleanup);
           }
       }
       break;
-- 
2.6.2

Re: [PATCH v2] gdb/record-full: Use xmalloc instead of alloca for temporary memory storage.

[Ulrich Weigand]

Marcin Kościelnicki  <koriakin@0x04.net>  wrote:

> On the newly added s390 target, it's possible for a single instruction
> to write practically unbounded amount of memory (eg. MVCLE).  This caused
> a stack overflow when alloca was used.
> 
> gdb/ChangeLog:
> 
> 	* record-full.c (record_full_exec_insn): Use xmalloc for temporary
> 	memory storage.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com