Re: [PATCH] Memory reads and writes should have size_t length

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

On Fri, 01 Jun 2012 21:59:58 +0200, Siddhesh Poyarekar wrote:
> On Fri, 1 Jun 2012 19:48:09 +0200, Jan wrote:
> > This patch goes again more far than what is needed, couldn't this be
> > ssize_t? Making it unsigned could be some other cleanup.
> > 
> 
> I took the liberty of changing signs here because this patch in itself
> is small enough (and independent)

But read_memory has 120 callers to check, other functions have hundreds of
other callers to check.  I have found at least the patch below to catch some
of these cases but it is far from complete.

Currently negative values were harmless on 32-bit hosts but now they will
overwrite GDB address space:

#include <stdio.h>
void target_read(long long x) {
  printf("%llx, 0<x==%d\n",x,0<x);
}
void target_read_memory_old(int    x) { target_read(x); }
void target_read_memory_new(size_t x) { target_read(x); }
int main (void) {
  target_read_memory_old(-1LL);
  target_read_memory_new(-1LL);
  return 0;
}

gcc -o 54   54.c -Wall -g     ;./54
ffffffffffffffff, 0<x==0
ffffffffffffffff, 0<x==0
gcc -o 5432 54.c -Wall -g -m32;./5432
ffffffffffffffff, 0<x==0
ffffffff, 0<x==1


> and if it does cause a regression, it
> should be pretty easy to isolate even with a simple bisect,

I do not think we need to cause regressions here, ssize_t has no practical
disadvantages compared to size_t, just it is not so clean/nice.


> This patch can be tested independently, so I figured this was OK. What
> do you think?

Unless someone else is going to protect all the hundreds/thousands of callers I
do not think it is worth it and ssize_t is good enough.


Thanks,
Jan


diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index 0e441fb..1636216 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -427,6 +427,8 @@ read_fat_string_value (char *dest, struct value *val, int max_len)
      The lower bound is always 1, so we only need to read the upper bound.  */
   bounds_val = value_ind (value_field (val, bounds_fieldno));
   len = value_as_long (value_field (bounds_val, upper_bound_fieldno));
+  if (len < 0)
+    error (_("Invalid task format.  Aborting."));
 
   /* Make sure that we do not read more than max_len characters...  */
   if (len > max_len)
diff --git a/gdb/valops.c b/gdb/valops.c
index feb47f5..ca4ee26 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1282,7 +1282,7 @@ value_assign (struct value *toval, struct value *fromval)
 		&& ((LONGEST) changed_addr % TYPE_LENGTH (type)) == 0)
 	      changed_len = TYPE_LENGTH (type);
 
-	    if (changed_len > (int) sizeof (LONGEST))
+	    if (changed_len > (int) sizeof (LONGEST) || changed_len < 0)
 	      error (_("Can't handle bitfields which "
 		       "don't fit in a %d bit word."),
 		     (int) sizeof (LONGEST) * HOST_CHAR_BIT);