Bug 29526

Summary: _dbus_get_autolaunch_address can be slow closing file descriptors
Product: dbus Reporter: Padraig O'Briain <padraig.obriain>
Component: coreAssignee: Havoc Pennington <hp>
Status: RESOLVED MOVED QA Contact: John (J5) Palmieri <johnp>
Severity: normal    
Priority: medium CC: bugzilla, msniko14
Version: 1.5   
Hardware: All   
OS: All   
Whiteboard: review-
i915 platform: i915 features:
Attachments: Proposed patch
Updated patch that applies to git master as of July 2017

Description Padraig O'Briain 2010-08-12 03:11:03 UTC
Created attachment 37806 [details]
Proposed patch

On my system _dbus_get-autolaunch_address takes almost 1.5 seconds.

This is because the maximum number of file descriptors is 65536.
Comment 1 Simon McVittie 2013-08-27 18:33:11 UTC
Sorry for the delay in getting to this...

Is closefrom() susceptible to interruption (EINTR)?

Is closefrom() async-signal-safe, i.e. safe to call after fork()?

Does closefrom() return int (with the same 0/-1 semantics as close()) on all platforms that support it?
Comment 2 Simon McVittie 2013-08-27 18:48:36 UTC
It appears that Solaris and (Free|Net|Open)BSD have closefrom(), and AIX, Irix, NetBSD can implement it as fcntl(3, F_CLOSEM, 0).
Comment 3 Simon McVittie 2014-06-11 09:44:45 UTC
(In reply to comment #1)
> Is closefrom() susceptible to interruption (EINTR)?
> 
> Is closefrom() async-signal-safe, i.e. safe to call after fork()?
> 
> Does closefrom() return int (with the same 0/-1 semantics as close()) on all
> platforms that support it?

Could someone from Solaris or *BSD please answer these?
Comment 4 Simon McVittie 2014-09-23 14:11:11 UTC
Reverting apparently random/abusive priority, status, severity, keyword changes.

"thulani makhathini", don't do that. It's because of people like you that we can't have nice things.
Comment 5 Alan Coopersmith 2017-08-11 04:40:55 UTC
Created attachment 133434 [details] [review]
Updated patch that applies to git master as of July 2017
Comment 6 Alan Coopersmith 2017-08-11 04:49:23 UTC
> Is closefrom() susceptible to interruption (EINTR)?

NetBSD & OpenBSD document that it is, Solaris & FreeBSD do not.

> Is closefrom() async-signal-safe, i.e. safe to call after fork()?

That is not documented.

> Does closefrom() return int (with the same 0/-1 semantics as close()) on all
> platforms that support it?

It returns void on FreeBSD & Solaris:
https://www.freebsd.org/cgi/man.cgi?query=closefrom&sektion=2
http://docs.oracle.com/cd/E86824_01/html/E54766/closefrom-3c.html

But OpenBSD and netbsd return an int:
https://man.openbsd.org/closefrom
http://netbsd.gw.com/cgi-bin/man-cgi?closefrom++NetBSD-current
Comment 7 Philip Withnall 2017-08-12 00:33:23 UTC
Comment on attachment 133434 [details] [review]
Updated patch that applies to git master as of July 2017

Review of attachment 133434 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-unix.c
@@ +4374,5 @@
>  void
>  _dbus_close_all (void)
>  {
> +#ifdef HAVE_CLOSEFROM
> +  closefrom (3);

Given that it can fail with EINTR on some platforms, perhaps we should use it with TEMP_FAILURE_RETRY on those platforms?
Comment 8 emaste 2017-09-14 12:58:51 UTC
(In reply to Alan Coopersmith from comment #6)
> > Is closefrom() susceptible to interruption (EINTR)?
> 
> NetBSD & OpenBSD document that it is, Solaris & FreeBSD do not.
> 
> > Is closefrom() async-signal-safe, i.e. safe to call after fork()?
> 
> That is not documented.

FreeBSD's sigaction(2) man page includes closefrom in the list of async-signal safe "Extension Interfaces".

https://www.freebsd.org/cgi/man.cgi?query=sigaction&sektion=2
Comment 9 Philip Withnall 2017-09-15 10:25:41 UTC
OpenBSD’s equivalent sigaction(2) man page does *not* list closefrom(): https://man.openbsd.org/sigaction.2

So I suspect this patch might end up being FreeBSD-only.

---

However, the current Linux-specific code uses opendir() which is definitely not async-signal-safe, so there are already problems here. _dbus_close_all() is seemingly only used from code sections between fork() and exec() so it should really be async-signal-safe (as per `man 2 fork`).

Simon, is there some reason why _dbus_close_all() is not currently async-signal-safe on Linux? It was introduced in bug #35230, which didn’t seem to discuss the async-signal-safety aspects of being sandwiched between fork() and exec() at all.
Comment 10 GitLab Migration User 2018-10-12 21:07:04 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/29.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.