[patch] gdbserver: Fix overlapping memcpy (safe now)

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi,

I thought it is causing a crash but that was a different cause.  Anyway
I already had the patch, it violates ISO C memcpy definition
	If copying takes place between objects that overlap, the behavior is
	undefined.
and it also complains in valgrind:
	Source and destination overlap in memcpy(0x4c81da8, 0x4c81da8, 1)
	   at: memcpy@@GLIBC_2.14 (mc_replace_strmem.c:653)
	   by: check_mem_write (mem-break.c:1102)
	   by: write_inferior_memory (target.c:66)
	   by: uninsert_raw_breakpoint (mem-break.c:751)
	   by: uninsert_breakpoints_at (mem-break.c:784)

I find too fragile to pass a pointer into internal buffer into a set of
functions modifying that buffer.

No regressions on {x86_64,x86_64-m32,i686}-fedora16-linux-gnu gdbserver mode.

I will check it in without comments, I find it safe.



Thanks,
Jan


gdb/gdbserver/
2011-12-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix overlapping memcpy.
	* mem-break.c (set_raw_breakpoint_at): New variable buf.  Use it for
	the read_inferior_memory transfer.
	(delete_fast_tracepoint_jump): New variable buf.  Use it for the
	write_inferior_memory transfer.
	(set_fast_tracepoint_jump): New variable buf.  Use it for the
	read_inferior_memory and write_inferior_memory transfers.
	(uninsert_fast_tracepoint_jumps_at, reinsert_fast_tracepoint_jumps_at)
	(delete_raw_breakpoint, uninsert_raw_breakpoint): New variable buf.
	Use it for the write_inferior_memory transfer.
	(check_mem_read, check_mem_write): New gdb_asserts for overlapping
	buffers.

--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -122,6 +122,7 @@ set_raw_breakpoint_at (CORE_ADDR where)
   struct process_info *proc = current_process ();
   struct raw_breakpoint *bp;
   int err;
+  unsigned char buf[MAX_BREAKPOINT_LEN];
 
   if (breakpoint_data == NULL)
     error ("Target does not support breakpoints.");
@@ -140,7 +141,7 @@ set_raw_breakpoint_at (CORE_ADDR where)
   /* Note that there can be fast tracepoint jumps installed in the
      same memory range, so to get at the original memory, we need to
      use read_inferior_memory, which masks those out.  */
-  err = read_inferior_memory (where, bp->old_data, breakpoint_len);
+  err = read_inferior_memory (where, buf, breakpoint_len);
   if (err != 0)
     {
       if (debug_threads)
@@ -151,6 +152,7 @@ set_raw_breakpoint_at (CORE_ADDR where)
       free (bp);
       return NULL;
     }
+  memcpy (bp->old_data, buf, breakpoint_len);
 
   err = (*the_target->write_memory) (where, breakpoint_data,
 				     breakpoint_len);
@@ -257,6 +259,7 @@ delete_fast_tracepoint_jump (struct fast_tracepoint_jump *todel)
 	  if (--bp->refcount == 0)
 	    {
 	      struct fast_tracepoint_jump *prev_bp_link = *bp_link;
+	      unsigned char *buf;
 
 	      /* Unlink it.  */
 	      *bp_link = bp->next;
@@ -270,9 +273,9 @@ delete_fast_tracepoint_jump (struct fast_tracepoint_jump *todel)
 		 pass the current shadow contents, because
 		 write_inferior_memory updates any shadow memory with
 		 what we pass here, and we want that to be a nop.  */
-	      ret = write_inferior_memory (bp->pc,
-					   fast_tracepoint_jump_shadow (bp),
-					   bp->length);
+	      buf = alloca (bp->length);
+	      memcpy (buf, fast_tracepoint_jump_shadow (bp), bp->length);
+	      ret = write_inferior_memory (bp->pc, buf, bp->length);
 	      if (ret != 0)
 		{
 		  /* Something went wrong, relink the jump.  */
@@ -315,6 +318,7 @@ set_fast_tracepoint_jump (CORE_ADDR where,
   struct process_info *proc = current_process ();
   struct fast_tracepoint_jump *jp;
   int err;
+  unsigned char *buf;
 
   /* We refcount fast tracepoint jumps.  Check if we already know
      about a jump at this address.  */
@@ -333,12 +337,12 @@ set_fast_tracepoint_jump (CORE_ADDR where,
   jp->length = length;
   memcpy (fast_tracepoint_jump_insn (jp), insn, length);
   jp->refcount = 1;
+  buf = alloca (length);
 
   /* Note that there can be trap breakpoints inserted in the same
      address range.  To access the original memory contents, we use
      `read_inferior_memory', which masks out breakpoints.  */
-  err = read_inferior_memory (where,
-			      fast_tracepoint_jump_shadow (jp), jp->length);
+  err = read_inferior_memory (where, buf, length);
   if (err != 0)
     {
       if (debug_threads)
@@ -349,6 +353,7 @@ set_fast_tracepoint_jump (CORE_ADDR where,
       free (jp);
       return NULL;
     }
+  memcpy (fast_tracepoint_jump_shadow (jp), buf, length);
 
   /* Link the jump in.  */
   jp->inserted = 1;
@@ -363,8 +368,7 @@ set_fast_tracepoint_jump (CORE_ADDR where,
      the current shadow contents, because write_inferior_memory
      updates any shadow memory with what we pass here, and we want
      that to be a nop.  */
-  err = write_inferior_memory (where, fast_tracepoint_jump_shadow (jp),
-			       length);
+  err = write_inferior_memory (where, buf, length);
   if (err != 0)
     {
       if (debug_threads)
@@ -403,6 +407,8 @@ uninsert_fast_tracepoint_jumps_at (CORE_ADDR pc)
 
   if (jp->inserted)
     {
+      unsigned char *buf;
+
       jp->inserted = 0;
 
       /* Since there can be trap breakpoints inserted in the same
@@ -414,9 +420,9 @@ uninsert_fast_tracepoint_jumps_at (CORE_ADDR pc)
 	 pass the current shadow contents, because
 	 write_inferior_memory updates any shadow memory with what we
 	 pass here, and we want that to be a nop.  */
-      err = write_inferior_memory (jp->pc,
-				   fast_tracepoint_jump_shadow (jp),
-				   jp->length);
+      buf = alloca (jp->length);
+      memcpy (buf, fast_tracepoint_jump_shadow (jp), jp->length);
+      err = write_inferior_memory (jp->pc, buf, jp->length);
       if (err != 0)
 	{
 	  jp->inserted = 1;
@@ -434,6 +440,7 @@ reinsert_fast_tracepoint_jumps_at (CORE_ADDR where)
 {
   struct fast_tracepoint_jump *jp;
   int err;
+  unsigned char *buf;
 
   jp = find_fast_tracepoint_jump_at (where);
   if (jp == NULL)
@@ -461,8 +468,9 @@ reinsert_fast_tracepoint_jumps_at (CORE_ADDR where)
      to pass the current shadow contents, because
      write_inferior_memory updates any shadow memory with what we pass
      here, and we want that to be a nop.  */
-  err = write_inferior_memory (where,
-			       fast_tracepoint_jump_shadow (jp), jp->length);
+  buf = alloca (jp->length);
+  memcpy (buf, fast_tracepoint_jump_shadow (jp), jp->length);
+  err = write_inferior_memory (where, buf, jp->length);
   if (err != 0)
     {
       jp->inserted = 0;
@@ -517,6 +525,7 @@ delete_raw_breakpoint (struct process_info *proc, struct raw_breakpoint *todel)
 	  if (bp->inserted)
 	    {
 	      struct raw_breakpoint *prev_bp_link = *bp_link;
+	      unsigned char buf[MAX_BREAKPOINT_LEN];
 
 	      *bp_link = bp->next;
 
@@ -529,8 +538,8 @@ delete_raw_breakpoint (struct process_info *proc, struct raw_breakpoint *todel)
 		 to pass the current shadow contents, because
 		 write_inferior_memory updates any shadow memory with
 		 what we pass here, and we want that to be a nop.  */
-	      ret = write_inferior_memory (bp->pc, bp->old_data,
-					   breakpoint_len);
+	      memcpy (buf, bp->old_data, breakpoint_len);
+	      ret = write_inferior_memory (bp->pc, buf, breakpoint_len);
 	      if (ret != 0)
 		{
 		  /* Something went wrong, relink the breakpoint.  */
@@ -738,6 +747,7 @@ uninsert_raw_breakpoint (struct raw_breakpoint *bp)
   if (bp->inserted)
     {
       int err;
+      unsigned char buf[MAX_BREAKPOINT_LEN];
 
       bp->inserted = 0;
       /* Since there can be fast tracepoint jumps inserted in the same
@@ -748,8 +758,8 @@ uninsert_raw_breakpoint (struct raw_breakpoint *bp)
 	 that we need to pass the current shadow contents, because
 	 write_inferior_memory updates any shadow memory with what we
 	 pass here, and we want that to be a nop.  */
-      err = write_inferior_memory (bp->pc, bp->old_data,
-				   breakpoint_len);
+      memcpy (buf, bp->old_data, breakpoint_len);
+      err = write_inferior_memory (bp->pc, buf, breakpoint_len);
       if (err != 0)
 	{
 	  bp->inserted = 1;
@@ -975,6 +985,9 @@ check_mem_read (CORE_ADDR mem_addr, unsigned char *buf, int mem_len)
       CORE_ADDR start, end;
       int copy_offset, copy_len, buf_offset;
 
+      gdb_assert (fast_tracepoint_jump_shadow (jp) >= buf + mem_len
+		  || buf >= fast_tracepoint_jump_shadow (jp) + (jp)->length);
+
       if (mem_addr >= bp_end)
 	continue;
       if (jp->pc >= mem_end)
@@ -1004,6 +1017,9 @@ check_mem_read (CORE_ADDR mem_addr, unsigned char *buf, int mem_len)
       CORE_ADDR start, end;
       int copy_offset, copy_len, buf_offset;
 
+      gdb_assert (bp->old_data >= buf + mem_len
+		  || buf >= &bp->old_data[sizeof (bp->old_data)]);
+
       if (mem_addr >= bp_end)
 	continue;
       if (bp->pc >= mem_end)
@@ -1052,6 +1068,11 @@ check_mem_write (CORE_ADDR mem_addr, unsigned char *buf,
       CORE_ADDR start, end;
       int copy_offset, copy_len, buf_offset;
 
+      gdb_assert (fast_tracepoint_jump_shadow (jp) >= myaddr + mem_len
+		  || myaddr >= fast_tracepoint_jump_shadow (jp) + (jp)->length);
+      gdb_assert (fast_tracepoint_jump_insn (jp) >= buf + mem_len
+		  || buf >= fast_tracepoint_jump_insn (jp) + (jp)->length);
+
       if (mem_addr >= jp_end)
 	continue;
       if (jp->pc >= mem_end)
@@ -1082,6 +1103,9 @@ check_mem_write (CORE_ADDR mem_addr, unsigned char *buf,
       CORE_ADDR start, end;
       int copy_offset, copy_len, buf_offset;
 
+      gdb_assert (bp->old_data >= myaddr + mem_len
+		  || myaddr >= &bp->old_data[sizeof (bp->old_data)]);
+
       if (mem_addr >= bp_end)
 	continue;
       if (bp->pc >= mem_end)