Re: [PATCH,HURD] sendto: do not crash when addr is NULL

[Pino Toscano <toscano dot pino at tiscali dot it>]

Hi,

Alle lunedì 21 maggio 2012, Roland McGrath ha scritto:
> Put the name of a local variable in all caps when mentioning it in
> the log entry or comments.

Noted, and done.

> It looks like the existing code also has a leak of APORT in the case
> where HURD_DPORT_USE fails (i.e. invalid fd).  So you should
> reorganize in some way that fixes that too.

Done.

> It might be simplest just to move the creation of the address port
> (both the AF_LOCAL case and the default case) into a subroutine and
> just call that inside HURD_DPORT_USE.  It means that the descriptor
> structure and port are kept alive around the AF_LOCAL case's calls,
> but that seems OK.

Done.

Updated patch attached.

-- 
Pino Toscano

Hurd: sendto: do not crash when ADDR is NULL

Create a new create_address_port subroutine to isolate the address port creation
(for both local and remove sockets), and use it inside HURD_DPORT_USE.
Also intialize APORT to MACH_PORT_NULL and make sure to always deallocate it.

2012-06-07  Pino Toscano  <toscano.pino@tiscali.it>

	* sysdeps/mach/hurd/sendto.c (create_address_port): New subroutine.
	(__sendto): Use create_address_port.  Initialize APORT and always
	deallocate it.
--- a/sysdeps/mach/hurd/sendto.c
+++ b/sysdeps/mach/hurd/sendto.c
@@ -33,37 +33,50 @@ __sendto (int fd,
 	  const struct sockaddr_un *addr,
 	  socklen_t addr_len)
 {
-  addr_port_t aport;
+  addr_port_t aport = MACH_PORT_NULL;
   error_t err;
   size_t wrote;
 
-  if (addr->sun_family == AF_LOCAL)
+  /* Get an address port for the desired destination address.  */
+  error_t create_address_port (io_t port,
+			       const struct sockaddr_un *addr,
+			       socklen_t addr_len,
+			       addr_port_t *aport)
     {
-      /* For the local domain, we must look up the name as a file and talk
-	 to it with the ifsock protocol.  */
-      file_t file = __file_name_lookup (addr->sun_path, 0, 0);
-      if (file == MACH_PORT_NULL)
-	return -1;
-      err = __ifsock_getsockaddr (file, &aport);
-      __mach_port_deallocate (__mach_task_self (), file);
-      if (err == MIG_BAD_ID || err == EOPNOTSUPP)
-	/* The file did not grok the ifsock protocol.  */
-	err = ENOTSOCK;
-      if (err)
-	return __hurd_fail (err);
+      error_t err;
+
+      if (addr->sun_family == AF_LOCAL)
+	{
+	  /* For the local domain, we must look up the name as a file and talk
+	     to it with the ifsock protocol.  */
+	  file_t file = __file_name_lookup (addr->sun_path, 0, 0);
+	  if (file == MACH_PORT_NULL)
+	    return errno;
+	  err = __ifsock_getsockaddr (file, aport);
+	  __mach_port_deallocate (__mach_task_self (), file);
+	  if (err == MIG_BAD_ID || err == EOPNOTSUPP)
+	    /* The file did not grok the ifsock protocol.  */
+	    err = ENOTSOCK;
+	}
+      else
+	{
+	  err = __socket_create_address (port,
+					 addr->sun_family,
+					 (char *) addr,
+					 addr_len,
+					 aport);
+	}
+
+      return err;
     }
-  else
-    err = EIEIO;
 
-  /* Get an address port for the desired destination address.  */
   err = HURD_DPORT_USE (fd,
 			({
-			  if (err)
-			    err = __socket_create_address (port,
-							   addr->sun_family,
-							   (char *) addr,
-							   addr_len,
-							   &aport);
+			  if (addr != NULL)
+			    err = create_address_port (port, addr, addr_len,
+						       &aport);
+			  else
+			    err = 0;
 			  if (! err)
 			    {
 			      /* Send the data.  */
@@ -72,12 +85,12 @@ __sendto (int fd,
 						   NULL,
 						   MACH_MSG_TYPE_COPY_SEND, 0,
 						   NULL, 0, &wrote);
-			      __mach_port_deallocate (__mach_task_self (),
-						      aport);
 			    }
 			  err;
 			}));
 
+  __mach_port_deallocate (__mach_task_self (), aport);
+
   return err ? __hurd_sockfail (fd, flags, err) : wrote;
 }

Attachment: signature.asc
Description: This is a digitally signed message part.