Re: port leak in select
Richard Braun, on Fri 02 Dec 2016 10:56:40 +0100, wrote:
> 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 ?
Here, no, we have the same structure:
if (firstfd == -1)
/* But not if there were no ports to deal with at all.
We are just a pure timeout. */
portset = __mach_reply_port ();
else
{
portset = MACH_PORT_NULL;
for (i = firstfd; i <= lastfd; ++i)
+ if (d[i].type & SELECT_ERROR)
+ d[i].reply_port = MACH_PORT_NULL
+ else
- if (d[i].type & ~SELECT_ERROR)
{
int type = d[i].type;
d[i].reply_port = __mach_reply_port ();
if (timeout == NULL)
err = __io_select_request (d[i].io_port, d[i].reply_port, type);
else
and
if (firstfd != -1)
for (i = firstfd; i <= lastfd; ++i)
- if (d[i].type & ~(SELECT_ERROR | SELECT_RETURNED))
+ if (d[i].reply_port != MACH_PORT_NULL)
__mach_port_destroy (__mach_task_self (), d[i].reply_port);
Samuel
Re: port leak in select
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
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
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
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
