Re: [RFA] gdbserver/win32-low.c: Check Read/WriteProcessMemory return value (followup to [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function)

[Pedro Alves <palves at redhat dot com>]

On 09/02/2013 02:38 PM, Pierre Muller wrote:
>>>>> This is not compatible with returning information that only part of
> the
>>>>> request length
>>>>> was read/written.
>>>>
>>>> Well, we could just change that interface to make it possible...
>>>>
>>>> The thing I don't like with doing this only on the native
>>>> side, is that we're trying to get to a point where we
>>>> can share the target backends between GDB and gdbserver:
>>>
>>>   Well, when you look at the code inside child_xfer_memory,
>>> you can notice that the return value of ReadProcessMemory or
>>> WriteProcessMemory
>>> is discarded, which means that it does behave more or less like the
>>> new windows-nat.c code (at least in case of ERROR_PARTIAL_COPY)
>>> for other errors, it might also return garbage...
>>> anyhow, the calling code compares the returned value to the requested
>> length
>>> (LEN value)
>>
>> That's brittle...
>>
>>> so that the risk of generating a successful read_memory despite a
> failure
>>> of ReadProcessMemory function is small... (the uninitialized variable
> done
>>> would need to return the value LEN..)
>>> It could of course still happen theoretically...
>>
>> This is really no argument for not fixing gdbserver...  In fact,
>> it's an argument _for_ fixing it.
> 
>   What about this patch,
> it still does not allow to really return the number of bytes read or
> written, 
> but at least it checks correctly if the API calls succeeded.

No, as long as the read_memory/write_memory interfaces do not
support partial transfers, we should only return true if the
all of LEN was transferred.  Otherwise, things like:

static int
gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
{
...
    {
      res = read_inferior_memory (memaddr, myaddr, len);
      done_accessing_memory ();

      return res == 0 ? len : -1;
    }
}

will behave incorrectly in the ERROR_PARTIAL_COPY scenario...

-- 
Pedro Alves