Re: port leak in select

2016-12-02 Thread Richard Braun 
On Fri, Dec 02, 2016 at 10:43:04AM +0100, Samuel Thibault wrote:
> Or simply
> 
>   (d[i].reply_port != MACH_PORT_NULL))

Aren't there cases where an entry could remain uninitialized ?

-- 
Richard Braun

Re: port leak in select

2016-12-02 Thread Samuel Thibault 
Richard Braun, on Fri 02 Dec 2016 10:28:48 +0100, wrote:
> > as well as in the error case of __io_select_request, and then just check
> > against MACH_PORT_NULL before destroy.
> 
> So the condition would look like
> 
> if (d[i].type && (d[i].reply_port != MACH_PORT_NULL))

Or simply

  (d[i].reply_port != MACH_PORT_NULL))

Samuel

Re: port leak in select

2016-12-02 Thread Richard Braun 
On Fri, Dec 02, 2016 at 08:41:21AM +0100, Samuel Thibault wrote:
> About the port leak in select discussed on IRC, I checked history a bit,
> this (000ef460744786571f51604e6de631b7168e239a):
> 
> -  if (d[i].type)
> +  if (d[i].type & ~SELECT_ERROR)
> __mach_port_destroy (__mach_task_self (), d[i].reply_port);
> 
> was added when handling EBADF , for which there is no reply port to
> destroy. Then I added this (099f8d2b7ecedc4f6fc895d2c35912f995289c24):

Indeed, so type != 0 doesn't imply port creation.

> I guess we could simply do this:
> 
>   for (i = firstfd; i <= lastfd; ++i)
> - if (d[i].type & ~SELECT_ERROR)
> + if (d[i].type & SELECT_ERROR)
> +   {
> + d[i].reply_port = MACH_PORT_NULL;
> + }
> else
> {
>   int type = d[i].type;
>   d[i].reply_port = __mach_reply_port ();
> 
> as well as in the error case of __io_select_request, and then just check
> against MACH_PORT_NULL before destroy.

So the condition would look like

if (d[i].type && (d[i].reply_port != MACH_PORT_NULL))

I agree.

In the mean time, I've built libc packages that partially fix the issue
(there will still be spurious destruction attempts on EBADF, which could
destroy a legitimate right) but the leak is so severe I found it useful.

If you want those, add

deb http://ftp.sceen.net/debian sid main

to sources.list, and add the GPG key D50E6AD4 to APT.

-- 
Richard Braun

port leak in select

2016-12-02 Thread Samuel Thibault 
Hello,

About the port leak in select discussed on IRC, I checked history a bit,
this (000ef460744786571f51604e6de631b7168e239a):

-  if (d[i].type)
+  if (d[i].type & ~SELECT_ERROR)
__mach_port_destroy (__mach_task_self (), d[i].reply_port);

was added when handling EBADF , for which there is no reply port to
destroy. Then I added this (099f8d2b7ecedc4f6fc895d2c35912f995289c24):

-  if (d[i].type & ~SELECT_ERROR)
+  if (d[i].type & ~(SELECT_ERROR | SELECT_RETURNED))
__mach_port_destroy (__mach_task_self (), d[i].reply_port);

For the case where io_select returns an error.  Yesterday evening I
didn't remember which case this was, but IIRC that's when a tty gets
io_revoke()d, in which case the RPC doesn't happen at all since the
target port was destroyed.


I guess we could simply do this:

  for (i = firstfd; i <= lastfd; ++i)
- if (d[i].type & ~SELECT_ERROR)
+ if (d[i].type & SELECT_ERROR)
+   {
+ d[i].reply_port = MACH_PORT_NULL;
+   }
  else
{
  int type = d[i].type;
  d[i].reply_port = __mach_reply_port ();

as well as in the error case of __io_select_request, and then just check
against MACH_PORT_NULL before destroy.

Samuel