Re: [PATCH PR gdb/15236] gdbserver write to linux memory with zero length corrupts stack

Pedro Alves
2013-03-06T19:07:28Z

Hi Jeremy,

Thanks for the diagnosis and the patch.

On 03/06/2013 06:03 PM, Jeremy Bennett wrote:
> PROBLEM:
>
> The function linux_write_memory () in linux-low.c allocates a buffer on
> the stack to hold a copy of the data to be written.
>
> register PTRACE_XFER_TYPE *buffer = (PTRACE_XFER_TYPE *)
> alloca (count * sizeof (PTRACE_XFER_TYPE));
>
> "count" is the number of bytes to be written, rounded up to the nearest
> multiple of sizeof (PTRACE_XFER_TYPE) and allowing for not being an
> aligned address. The function later uses
>
> buffer[0] = ptrace (PTRACE_PEEKTEXT, pid,
> (PTRACE_ARG3_TYPE) (uintptr_t) addr, 0);
>
> The problem is that this function can be called to write zero bytes on
> an aligned address, for example when receiving an X packet of length 0
> (used to test if 8-bit write is supported). Under these circumstances,
> count can be zero.
>
> Since in this case, buffer[0] may never have been allocated, the stack
> is corrupted and gdbserver may crash.
>
> Demonstrated with the port of GDB 7.5.1 for the Synopsys
> arc-linux-uclibc- target, currently under development at:
>
> https://github.com/foss-for-synopsys-dwc-arc-processors/gdb
>
> (to be submitted to the FSF in due course).
>
> SOLUTION:
>
> Writing zero bytes should always succeed. The patch below returns
> successfully early if the length is zero, so avoiding the stack
> corruption.
>
> Verified on the ARC GDB 7.5.1 port.
>
> CHANGELOG ENTRY:
>
> 2013-03-06 Jeremy Bennett <jeremy.bennett@embecosm.com>
>
> PR gdb/15236
> * linux-low.c (linux_write_memory): Return early success if len is
> zero.
>

> +2013-03-06 Jeremy Bennett <jeremy.bennett@embecosm.com>
> +
> + PR gdb/15236
> + * linux-low.c (linux_write_memory): Return early success if len is
> + zero.

All caps when talking about the value of a variable. So, if LEN is zero.

> +
> 2012-04-29 Yao Qi <yao@codesourcery.com>
>
> * server.h: Move some code to ...
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index bbb0693..8e576bd 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -4421,7 +4421,14 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
>
> /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's
> memory at MEMADDR. On failure (cannot write to the inferior)
> - returns the value of errno. */
> + returns the value of errno.
> +
> + 6-Mar-13, Jeremy Bennett: [PR gdb/15236] This function can be called with
> + length 0 (for example with a zero length X packet). If memaddr is aligned
> + to sizeof (PTRACE_XFER_TYPE), then count will be zero and nothing may be
> + allocated for buffer (architecture dependent). The function must return
> + early in this circumstance, to avoid stack corruption when assigning
> + to buffer[0]. */

I appreciate the thorough description of the issue.

But, that's really a too long comment in the function header, the place
people look at the learn about the function's _interface_, on a subject that
really is not an detail of the function's external interface. Comments
on implementation details should go within the function body. The date/name/PR
number are just unnecessary. In fact, I'd rather just remove the whole
comment -- it's useful for the patch description, but otherwise, in the
code, to the reader, I think it distracts more than it adds value.

Also, double-space after periods.

>
> static int
> linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
> @@ -4440,6 +4447,10 @@ linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
>
> int pid = lwpid_of (get_thread_lwp (current_inferior));
>
> + if (0 == len) {

We don't use that style of putting the constant on the lhs
in GDB.

Opening { goes on new line.

> + return 0; /* Zero length write always succeeds. */
> + }
> +

Single-line statements in if blocks don't get wrapped with {}'s.

Comment is put above the return instead of on the side (we do
have a few back sheep). The comment then makes the if block more
than one line, so, the {}'s remain. IOW, write as:

if (len == 0)
{
/* Zero length write always succeeds. */
return 0;
}

OK with those changes.

Thanks,
--
Pedro Alves