Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170628151328.GD1627@brightrain.aerifal.cx>
Date: Wed, 28 Jun 2017 11:13:28 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] fix undefined behavior in ptrace

On Wed, Jun 28, 2017 at 04:25:13PM +0300, Alexander Monakov wrote:
> ---
>  src/linux/ptrace.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/linux/ptrace.c b/src/linux/ptrace.c
> index 83b8022b..ab7fcda3 100644
> --- a/src/linux/ptrace.c
> +++ b/src/linux/ptrace.c
> @@ -7,14 +7,17 @@ long ptrace(int req, ...)
>  {
>  	va_list ap;
>  	pid_t pid;
> -	void *addr, *data, *addr2;
> +	void *addr, *data, *addr2 = 0;
>  	long ret, result;
>  
>  	va_start(ap, req);
>  	pid = va_arg(ap, pid_t);
>  	addr = va_arg(ap, void *);
>  	data = va_arg(ap, void *);
> +	/* PTRACE_{READ,WRITE}{DATA,TEXT} are specific to SPARC. */
> +#ifdef PTRACE_READTEXT
>  	addr2 = va_arg(ap, void *);
> +#endif

I think there's still UB here, reading more args than were passed.
These calls to va_arg should probably be dependent on the particular
req; I don't see any reason for it to be compile-time dependent on the
presence of one particular req value.

Otherwise yes it's an improvement.

Rich

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.