[PATCH 06/10] wayland-server: reject socket paths longer than 108 bytes

[Pekka Paalanen]

Hi,

a nice patch series in general. A minor comment below...

On Sun, 1 Jul 2012 17:52:09 +0000
nobled <nobled at dreamwidth.org> wrote:

> Attempting to write anything longer into the embedded char
> array would create a non-null-terminated string, and all
> later reads would run off the end into invalid memory.
> 
> This is a hard limitation of AF_LOCAL/AF_UNIX sockets.
> ---
>  src/wayland-server.c |   16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 8b24a71..d141682 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1153,7 +1153,8 @@ WL_EXPORT int
>  wl_display_add_socket(struct wl_display *display, const char *name)
>  {
>  	struct wl_socket *s;
> -	socklen_t size, name_size;
> +	socklen_t size;
> +	int name_size;
>  	const char *runtime_dir;
> 
>  	runtime_dir = getenv("XDG_RUNTIME_DIR");
> @@ -1185,6 +1186,19 @@ wl_display_add_socket(struct wl_display
> *display, const char *name)
>  	s->addr.sun_family = AF_LOCAL;
>  	name_size = snprintf(s->addr.sun_path, sizeof s->addr.sun_path,
>  			     "%s/%s", runtime_dir, name) + 1;
> +
> +	assert(name_size > 0);

This added assert doesn't feel right. I guess you are trying to catch
the ancient libc that would return -1 on truncation?
Could you just roll this check into the condition below?

> +	if (name_size > (int)sizeof s->addr.sun_path) {
> +		wl_log("error: socket path \"%s/%s\" plus null terminator"
> +		       " exceeds 108 bytes\n", runtime_dir, name);

Might as well print the size from sizeof instead of hardcoding, no?

> +		close(s->fd);
> +		free(s);
> +		/* to prevent programs reporting
> +		 * "failed to add socket: Success" */
> +		errno = ENAMETOOLONG;
> +		return -1;
> +	};
> +
>  	wl_log("using socket %s\n", s->addr.sun_path);
> 
>  	s->fd_lock = get_socket_lock(s);

Thanks

-- 
Pekka Paalanen
http://www.iki.fi/pq/