Re: Off-by-one bug at unix_mkname ?

[Willy Tarreau <willy@w.ods.org>]

Hi,

On Mon, Mar 28, 2005 at 05:00:05PM +0900, Tetsuo Handa wrote:
> Hi,
> 
> It seems to me that the following code is off-by-one bug.
> 
> http://lxr.linux.no/source/net/unix/af_unix.c#L191
> http://lxr.linux.no/source/net/unix/af_unix.c?v=2.4.28#L182
> 
> I think
> ((char *)sunaddr)[len]=0;
> should be
> ((char *)sunaddr)[len-1]=0;

it seems you're right, or the first test in the function is wrong, so
there's clearly something to be fixed there :

static int unix_mkname(struct sockaddr_un * sunaddr, int len, unsigned *hashp)
{
	if (len <= sizeof(short) || len > sizeof(*sunaddr))
                                    ^^^^^^^^^^^^^^^^^^^^^^
		return -EINVAL;
	if (!sunaddr || sunaddr->sun_family != AF_UNIX)
		return -EINVAL;
	if (sunaddr->sun_path[0]) {
		((char *)sunaddr)[len]=0;
                        ^^^^^^^^^^^^^^
		len = strlen(sunaddr->sun_path)+1+sizeof(short);
		return len;
	}

	*hashp = unix_hash_fold(csum_partial((char*)sunaddr, len, 0));
	return len;
}


Then, I would propose this patch (both for 2.4 and 2.6) :

--- ./net/unix/af_unix.c.bad	Sat Mar 26 07:42:49 2005
+++ ./net/unix/af_unix.c	Mon Mar 28 10:11:25 2005
@@ -179,7 +179,7 @@
 	if (!sunaddr || sunaddr->sun_family != AF_UNIX)
 		return -EINVAL;
 	if (sunaddr->sun_path[0]) {
-		((char *)sunaddr)[len]=0;
+		((char *)sunaddr)[len-1]=0;
 		len = strlen(sunaddr->sun_path)+1+sizeof(short);
 		return len;
 	}

Regards,
Willy