On Sun, Mar 18, 2018 at 01:43:23AM +0100, Mark Wielaard wrote:
[...]
> @@ -115,12 +116,80 @@ __libdwfl_ptrace_attach (pid_t tid, bool *tid_was_stoppedp)
>    return true;
>  }
>  
> +#ifdef HAVE_PROCESS_VM_READV
> +static bool
> +read_cached_memory (struct __libdwfl_pid_arg *pid_arg,
> +		    Dwarf_Addr addr, Dwarf_Word *result)
> +{
> +  /* Let the ptrace fallback deal with the corner case of the address
> +     possibly crossing a page boundery.  */
> +  if ((addr & ((Dwarf_Addr)__LIBDWFL_REMOTE_MEM_CACHE_SIZE - 1))
> +      > (Dwarf_Addr)__LIBDWFL_REMOTE_MEM_CACHE_SIZE - sizeof (unsigned long))

It looks odd that the variable that is going to be assigned has type
Dwarf_Word, while the size being checked has type unsigned long.
Shouldn't it be sizeof(*result) instead?

> +    return false;
> +
> +  struct __libdwfl_remote_mem_cache *mem_cache = pid_arg->mem_cache;
> +  if (mem_cache == NULL)
> +    {
> +      size_t mem_cache_size = sizeof (struct __libdwfl_remote_mem_cache);
> +      mem_cache = (struct __libdwfl_remote_mem_cache *) malloc (mem_cache_size);
> +      if (mem_cache == NULL)
> +	return false;
> +
> +      mem_cache->addr = 0;
> +      mem_cache->len = 0;
> +      pid_arg->mem_cache = mem_cache;
> +    }
> +
> +  unsigned char *d;
> +  if (addr >= mem_cache->addr && addr - mem_cache->addr < mem_cache->len)
> +    {
> +      d = &mem_cache->buf[addr - mem_cache->addr];
> +      *result = *(unsigned long *) d;

Likewise, shouldn't it be memcpy(result, d, sizeof(*result)) instead?


-- 
ldv