Problem report #15

Problem report #15

[Dan Nicolaescu]

CID: 15
Checker: FORWARD_NULL (help)
File: emacs/src/process.c
Function: conv_lisp_to_sockaddr
Description: Incrementing possibly NULL value "cp"

Event assign_zero: Variable "cp" assigned value 0.
Also see events: [dereference]

2335 	  register unsigned char *cp = NULL;
2336 	  register int i;
2337 	
2338 	  bzero (sa, len);
2339 	  sa->sa_family = family;
2340 	

At conditional (1): "address & 7 == 4" taking true path
At conditional (2): "((0), (address & -8))->size & 1073741824 == 0" taking true path

2341 	  if (VECTORP (address))
2342 	    {
2343 	      p = XVECTOR (address);

At conditional (3): "family == 2" taking false path

2344 	      if (family == AF_INET)
2345 		{
2346 		  struct sockaddr_in *sin = (struct sockaddr_in *) sa;
2347 		  len = sizeof (sin->sin_addr) + 1;
2348 		  i = XINT (p->contents[--len]);
2349 		  sin->sin_port = htons (i);
2350 		  cp = (unsigned char *)&sin->sin_addr;
2351 		}
2352 	#ifdef AF_INET6

At conditional (4): "family == 10" taking false path

2353 	      else if (family == AF_INET6)
2354 		{
2355 		  struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *) sa;
2356 		  uint16_t *ip6 = (uint16_t *)&sin6->sin6_addr;
2357 		  len = sizeof (sin6->sin6_addr) + 1;
2358 		  i = XINT (p->contents[--len]);
2359 		  sin6->sin6_port = htons (i);
2360 		  for (i = 0; i < len; i++)
2361 		    if (INTEGERP (p->contents[i]))
2362 		      {
2363 			int j = XFASTINT (p->contents[i]) & 0xffff;
2364 			ip6[i] = ntohs (j);
2365 		      }
2366 		  return;
2367 		}
2368 	#endif
2369 	    }
2370 	  else if (STRINGP (address))
2371 	    {
2372 	#ifdef HAVE_LOCAL_SOCKETS
2373 	      if (family == AF_LOCAL)
2374 		{
2375 		  struct sockaddr_un *sockun = (struct sockaddr_un *) sa;
2376 		  cp = SDATA (address);
2377 		  for (i = 0; i < sizeof (sockun->sun_path) && *cp; i++)
2378 		    sockun->sun_path[i] = *cp++;
2379 		}
2380 	#endif
2381 	      return;
2382 	    }
2383 	  else
2384 	    {
2385 	      p = XVECTOR (XCDR (address));
2386 	      cp = (unsigned char *)sa + sizeof (sa->sa_family);
2387 	    }
2388 	

At conditional (5): "i < len" taking true path

2389 	  for (i = 0; i < len; i++)

At conditional (6): "(p)->contents[i] & 7 == 0" taking true path

2390 	    if (INTEGERP (p->contents[i]))

Event dereference: Incrementing possibly NULL value "cp"
Also see events: [assign_zero]

2391 	      *cp++ = XFASTINT (p->contents[i]) & 0xff;
2392 	}

Re: Problem report #15

[Stuart D. Herring]

> 2344 	      if (family == AF_INET)
> 2345 		{
> [...]
> 2351 		}
> 2352 	#ifdef AF_INET6
>
> At conditional (4): "family == 10" taking false path
>
> 2353 	      else if (family == AF_INET6)
> 2354 		{
> [...]
> 2367 		}
> 2368 	#endif
> 2369 	    }

Shouldn't there be some sort of else signal bad-family-choice?  It looks
like get_lisp_to_sockaddr_size can store anything into family...

> At conditional (5): "i < len" taking true path
>
> 2389 	  for (i = 0; i < len; i++)
>
> At conditional (6): "(p)->contents[i] & 7 == 0" taking true path
>
> 2390 	    if (INTEGERP (p->contents[i]))
>
> Event dereference: Incrementing possibly NULL value "cp"
> Also see events: [assign_zero]
>
> 2391 	      *cp++ = XFASTINT (p->contents[i]) & 0xff;

For that matter, shouldn't there be some sort of signal when there are
things that aren't integers present?  I really don't expect

[save-buffers-kill-emacs 64 #<subr symbol-function> 241]

to silently become 64.241.?.?.  I think this is, perhaps, two bugs.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: Problem report #15

[Richard Stallman]

FAMILY can't be invalid because get_lisp_to_sockaddr_size
has made sure it is valid.

(The comment said size_lisp_to_sockaddr; I fixed that.)

Re: Problem report #15

[Stuart D. Herring]

> FAMILY can't be invalid because get_lisp_to_sockaddr_size
> has made sure it is valid.
>
> (The comment said size_lisp_to_sockaddr; I fixed that.)

I don't see how get_lisp_to_sockaddr_size does that.  If `address' is a
Lisp vector but has length, say, 15, it seems that `family' (`familyp') is
never assigned.  As far as I can tell, no one ever checks the value of
family between there and where it is compared to AF_INET and AF_INET6.  If
it compares unequal to each of those, then `cp' in conv_lisp_to_sockaddr
will be left NULL and the last line in conv_lisp_to_sockaddr will
segfault.

I think this bug is real, then -- probably just need to signal a Lisp
error in conv_lisp_to_sockaddr if the address is a vector but family is
neither AF_INET nor AF_INET6.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: Problem report #15

[Richard Stallman]

    I don't see how get_lisp_to_sockaddr_size does that.  If `address' is a
    Lisp vector but has length, say, 15, it seems that `family' (`familyp') is
    never assigned.  As far as I can tell, no one ever checks the value of
    family between there and where it is compared to AF_INET and AF_INET6.

Yes, you're right.  I think I have fixed it now.

However, I wonder, why not make conv_lisp_to_sockaddr signal
a Lisp error if the family is not recognized.  Is there any
reason not to do this?

Re: Problem report #15

[Stuart D. Herring]

> Yes, you're right.  I think I have fixed it now.
>
> However, I wonder, why not make conv_lisp_to_sockaddr signal
> a Lisp error if the family is not recognized.  Is there any
> reason not to do this?

That's exactly what I suggested in the previous mail.  However, it might
be good for get_lisp_to_sockaddr_size to signal if it can't figure out a
reasonable size, so that code doesn't break in the future if a new address
family or so is added to one and not the other.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.