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

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

Hi,

Alle martedì 12 giugno 2012, Roland McGrath ha scritto:
> That's almost right, but you should avoid calling
> __mach_port_deallocate if APORT is MACH_PORT_NULL.  Also, avoid
> shadowing the local variable ERR with one of the same name in the
> subfunction.

Hm ok, I saw other cases of shadowing by internal subroutines, and 
assumed it was okay (since inside them you want to refer to the local 
"err" instead of the outermost one.

Thanks for the further review, patch attached.
(I would use the non-ChangeLog lines of the patch header as commit log, 
would that be okay?)

-- 
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,
when not null.

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

	* sysdeps/mach/hurd/sendto.c (create_address_port): New subroutine.
	(__sendto): Use create_address_port.  Initialize APORT and deallocate
	it if not null.
--- 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_port;
+
+      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_port = __ifsock_getsockaddr (file, aport);
+	  __mach_port_deallocate (__mach_task_self (), file);
+	  if (err_port == MIG_BAD_ID || err_port == EOPNOTSUPP)
+	    /* The file did not grok the ifsock protocol.  */
+	    err_port = ENOTSOCK;
+	}
+      else
+	{
+	  err_port = __socket_create_address (port,
+					      addr->sun_family,
+					      (char *) addr,
+					      addr_len,
+					      aport);
+	}
+
+      return err_port;
     }
-  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,13 @@ __sendto (int fd,
 						   NULL,
 						   MACH_MSG_TYPE_COPY_SEND, 0,
 						   NULL, 0, &wrote);
-			      __mach_port_deallocate (__mach_task_self (),
-						      aport);
 			    }
 			  err;
 			}));
 
+  if (aport != MACH_PORT_NULL)
+    __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.