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

Jan Kratochvil
2012-06-04T21:54:43Z

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);