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

[Pedro Alves <palves@redhat.com>]

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