Last Comment Bug 147659 - [UNIX] Helper apps inherit open file descriptors
: [UNIX] Helper apps inherit open file descriptors
Status: NEW
[sg:want][necko-backlog]
: helpwanted, sec-want
Product: Core
Classification: Components
Component: Networking: File (show other bugs)
: Trunk
: x86 Linux
: P2 major with 23 votes (vote)
: ---
Assigned To: Karl Tomlinson (:karlt)
:
Mentors:
: 368003 372734 379493 384884 414841 415804 415808 419489 457272 460936 (view as bug list)
Depends on: 678369 713802
Blocks: 372734
  Show dependency treegraph
 
Reported: 2002-05-28 11:50 PDT by tenthumbs
Modified: 2016-02-01 18:21 PST (History)
54 users (show)
mbeltzner: wanted‑next+
reed: wanted1.9.2?
karlt: wanted1.9.1?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
helper app data (3.16 KB, text/plain)
2002-05-28 11:51 PDT, tenthumbs
no flags Details
initial mime-helper (4.95 KB, patch)
2002-06-04 13:41 PDT, tenthumbs
no flags Details | Diff | Splinter Review
like this (1.20 KB, patch)
2003-07-07 09:01 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Splinter Review
Patch - sets close-on-exec for the profile lock (540 bytes, patch)
2007-01-24 22:29 PST, Jeffrey Baker
no flags Details | Diff | Splinter Review

Description tenthumbs 2002-05-28 11:50:42 PDT
I ran into this while investigating something else. I set up /usr/bin/gs 
as a helper app for postscript files. In the attachment note that gs is 
a child of one of the mozilla threads. Note also that gs has a lot of 
open file descriptors and could write to a number of interesting things 
if it were so inclined. An unpleasant thought.

Clearly, mozilla needs to enforce the usual close-on-exec behavior but I 
haven't the foggiest idea where that should be. I'll start this off in file
handling but feel free to change the component.
Comment 1 tenthumbs 2002-05-28 11:51:57 PDT
Created attachment 85280 [details]
helper app data
Comment 2 Boris Zbarsky [:bz] 2002-05-28 23:28:12 PDT
This is an nsIProcess bug...

ccing some people who may know who it belongs to.
Comment 3 tenthumbs 2002-05-30 07:21:23 PDT
Just in case there's any confusion, this bug is about closing all file 
descriptors except stdin, stdout, and stderr.
Comment 4 Mike Shaver (:shaver -- probably not reading bugmail closely) 2002-05-30 07:29:00 PDT
I think it might be a necko bug.  We should set FD_CLOEXEC on all the fds we open.

Or maybe people are getting these fds via the OpenANSIDescriptor thingy?  Then
nsLocalFileUnix needs to set FD_CLOEXEC on the underlying fd, which is perhaps
_mildly_ exciting to do portably, but hey.
Comment 5 tenthumbs 2002-05-30 07:39:06 PDT
LXR finds FD_CLOEXEC in nsprpub/pr/src/md/unix/unix.c which mumbles 
about inheritance or some such. But nspr #defines so many things that I 
can't figure out what it really means.
Comment 6 tenthumbs 2002-05-30 08:03:05 PDT
NSPR has a PR_SetFDInheritable function but no one seems to use it.

Maybe I'm more paranoid than most but the potential for mischief is 
enormous here.
Comment 7 Mike Shaver (:shaver -- probably not reading bugmail closely) 2002-05-30 08:06:23 PDT
How so?  A helper app that's running as the user can do anything Mozilla can do.
 It can write to any files, open /proc/$mozillapid/fd/*, etc.

If you don't trust your helper apps, you're screwed in ways we can't begin to
protect against.
Comment 8 tenthumbs 2002-05-31 05:46:24 PDT
An external app can't read the pipes or sockets. I'm not sure how much
of a problem that is but I certainly don't want to find out.

Yes, there are many ways to get screwed. I just prefer to eliminate as
many as possible. Armor plating is nice.

And no, I don't trust helper apps or anything else that isn't guaranteed
bug free. I've been burned too often.

Take a look at bug 125505. There I proposed a mime assistant that could
execv the real helper. You could use the same idea to sanitize the
helper environment. It would be a lot faster than fixing all of mozilla.
Comment 9 Mike Shaver (:shaver -- probably not reading bugmail closely) 2002-05-31 06:04:45 PDT
On Linux, an external app _can_ read the pipes or sockets, via the
/proc/$mozillapid/fd/* mechanism I described earlier.  There are almost
certainly similar mechanisms on other platforms, since debuggers provide such
facilities.

I'm all for mitigating the damage buggy apps can do by accident, but protection
against "mischief" on the part of a malicious helper-app author is something
we're not going to be able to provide, and I'd rather we spent the effort elsewhere.

Closing file descriptors is a correctness issue, not a security/protection one.
Comment 10 tenthumbs 2002-06-03 07:18:31 PDT
You're better than I am. I tried and I can't get an app that isn't a
mozilla child to read from anonymous pipes or socketpairs. Sure you can
attach a process with ptrace but that's almost certainly going to be
noticed by the user. On Linux, the kernel stops an attached process.

So maybe I shouldn't have used the word "mischief." I didn't mean it in
quite the nefarious sense you seem to. I meant for it to include the
typical buggy app. That's what gotten me in the past. I've never
actually been burned by a malicious app but I have been severely burned
by buggy ones.

This might be a correctness issue but protection against data loss makes
it much more important, at least to me.
Comment 11 Doug Turner (:dougt) 2002-06-03 15:02:28 PDT
wtc, what is the default inherit trait of a file descriptor opened via
PR_Open()?  Is there a way to set all file descriptors opened by an application
not-inheritable by a child process without explictly calling
|PR_SetFDInheritable| after every PR_Open?
Comment 12 Wan-Teh Chang 2002-06-03 17:11:38 PDT
The default inherit trait of a file descriptor opened via
PR_Open() is inheritable on Unix and not-inheritable on Windows.

There is no convenient way to set all file descriptors opened
by an application not-inheritable by a child process on Unix.
I've seen people who resort to a for loop that closes all the
possible Unix fd's.  That for loop is rather expensive.

A possible solution is to use a helper process whose only
purpose is to fork and exec helper apps for the Mozilla client.
You know exactly which file descriptors are open in this helper
process and you can set FD_CLOEXEC on all these fd's.
Comment 13 tenthumbs 2002-06-04 13:41:20 PDT
Created attachment 86274 [details] [diff] [review]
initial mime-helper 

Here's a first try at a mime-helper. Rather than closing descriptors it sets
FD_CLOEXEC which some problems. It prefers to read /proc/self/exe but falls
back to a brute force strategy.

It works on Linux but that's not saying much.
Comment 14 Darin Fisher 2003-07-07 07:39:05 PDT
An alternative XP solution to this problem is to try to utilize the IPC daemon.
 Since it is spawned early at startup it should not have access to most (or all)
of the file descriptors we are concerned with.  It'd be relatively easy to add a
protocol extension for spawning processes via the IPC daemon.  We'd of course
have to take care to not make XPCOM (nsIProcess) depend on the IPC daemon. 
Thoughts?
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2003-07-07 08:10:21 PDT
How about we just use the NSPR function that is meant for this purpose:

PR_NewProcessAttr
http://www.mozilla.org/projects/nspr/reference/html/prprocess.html#24416
(see the description)

and use that processattr in PR_CreateProcess:
http://www.mozilla.org/projects/nspr/reference/html/prprocess.html#24535

at this place in mozilla code:
http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsProcessCommon.cpp#283
Comment 16 Christian :Biesinger (don't email me, ping me on IRC) 2003-07-07 09:01:21 PDT
Created attachment 127178 [details] [diff] [review]
like this

note, this is currently untested
Comment 17 Darin Fisher 2003-07-07 10:39:33 PDT
biesi: well, NSPR process attr only allows us to control specific values passed
on to the child process, like stdin for example.  currently, it doesn't provide
a way to make other file descriptors close-on-exec, which is what we'd need.  my
concern is with portability... basically, we need to iterate over any open file
descriptors (other than those for stdin, et al.) and mark them as close-on-exec.
 the portability problem is how to determine the set of open file descriptors. 
we know how to do this under linux using the /proc filesystem, but that
obviously doesn't work on other platforms.  for example, what about OSX?

if we use the IPC daemon then that problem is avoided.
Comment 18 Christian :Biesinger (don't email me, ping me on IRC) 2003-07-07 10:47:22 PDT
darin: well, the first url I mentioned in my comment contains this sentence:

 No file descriptors are inherited by the new process.

Is that not what we want here?
Comment 19 Darin Fisher 2003-07-07 11:01:50 PDT
i may be mistaken, but i think NSPR's notion of inheritable file descriptors is
just a mechanism to allow a child process to get access to a PRFileDesc handle
that references the OS file descriptor.  in other words, it is just a XP
mechanism for accessing inherited file descriptors.  it does not seem to get in
the way of the native mechanisms.  it just makes it possible to get a PRFileDesc
from a native file descriptor.  in fact, it seems to create a name to PRFileDesc
dictionary, which is passed to the child process as an environment variable (at
least that's how the UNIX impl works).  so, i think the comments in the NSPR
reference are just misleading.  but, you should probably give your patch a try ;)
Comment 20 Christian :Biesinger (don't email me, ping me on IRC) 2003-07-07 11:51:29 PDT
Comment on attachment 127178 [details] [diff] [review]
like this

aww... it seems you are right, unfortunately :( the child process still has the
file descriptors open
Comment 21 Wan-Teh Chang 2003-07-07 13:33:21 PDT
One solution is to execute the following for loop in
the child process before it calls exec:

    int fd;

    for (fd = 3; fd <= MaxFdOfTheProcess; fd++) {
        (void) close(fd);
    }

You can use PR_GetSysfdTableMax as an example of getting
the value of 'MaxFdOfTheProcess'.  (PR_GetSysfdTableMax
is an unsupported function, so its code may be incorrect.)
Comment 22 tenthumbs 2003-07-08 02:19:54 PDT
In the time since I field this bug I realized that open file descriptors 
are just once aspect of the problem of gracefully exiting the 
mozilla/gre/whatever environment. In particular, you can't leave it 
properly unless you know the start-up environment, which you don't. you 
really need to think about the proper way to start before you can leave.

I don't see how you can have a clean secure XP way to do any of this so 
I don't think how IPC (whatever that is) can help. I also don't think an 
XP solution is appropriate anyway.

See bug 125505, attachment 105336 [details]. I try to handle setting 
FD_CLOEXEC by testing /proc/self but fall back to looping over all fd's.
Comment 23 Aleksey Nogin 2005-12-01 18:07:08 PST
I noticed today that the esd (ESound Daemon) spawned by TBird playing the "new mail" sound (mail.biff.play_sound.url) had also inherited all the descriptors. Is this another instance of this bug, or a different one?
Comment 24 Boris Zbarsky [:bz] 2005-12-01 20:03:26 PST
Different bug.
Comment 25 Jeffrey Baker 2007-01-24 21:28:06 PST
*** Bug 368003 has been marked as a duplicate of this bug. ***
Comment 26 Jeffrey Baker 2007-01-24 22:29:28 PST
Created attachment 252742 [details] [diff] [review]
Patch - sets close-on-exec for the profile lock
Comment 27 Boris Zbarsky [:bz] 2007-01-24 22:31:11 PST
That patch looks like it should go in a separate bug, really, since it doesn't solve this bug.  It's a good special case that we should check in, though.
Comment 28 Jeffrey Baker 2007-01-24 22:43:38 PST
OK I will think about making this a separate bug.  It is a little depressing that nsProfileLock.cpp is a preprocessor mess.  I had assumed that the #ifdef jungle was supposed to be confined to NSPR.  Alas.

Also, I developed this after getting user complaints that helper applications spawned by previous invocations would prevent Firefox from starting, but now I have to go investigate that more, because on my platform (Linux) fcntl locks are not inherited by the child.
Comment 29 dean gaudet 2007-01-25 00:06:32 PST
on ubuntu 6.06 with firefox 2.0 with an NFS $HOME i've had the problem where i've killed every firefox instance but still have an acroread instance going (which was spawned via firefox -- not the plugin, i hate the plugin) and firefox won't restart... and the acroread has a good 60 fds open which were inherited from the firefox, including a couple .parentlock fds.

when i kill the acroread i can start firefox again.

so something is inherited here.
Comment 30 dean gaudet 2007-01-25 00:17:02 PST
you know, it occurs to me that NSPR should endeavour to present the same close-on-exec behaviour on all platforms... and honestly, the unix API is totally broken in this area -- this is something which NT (actually VMS, which NT inherits from) get right.  the unix behaviour is OK when you're dealing with a half dozen fds... but real programs (especially multithreaded programs) have way more than a half dozen open.  when you spawn a process you should explicitly specify the fds to be inherited.  NSPR should default to setting F_CLOEXEC on everything on unix.
Comment 31 Jeffrey Baker 2007-01-25 10:18:00 PST
Unfortunately in NSPR the same path is used to spawn an unrelated process and to fork your own process.  If you explicitly close all the FDs when NSPR forks, half the unit tests fail.
Comment 32 Boris Zbarsky [:bz] 2007-01-25 10:22:29 PST
Sounds like we could use an NSPR bug about passing more information into that code, eh?
Comment 33 Jeffrey Baker 2007-03-16 11:46:26 PDT
Now that this is blocking a bug I'm not allowed to view, we can only assume that the security implications here have finally been realized?
Comment 34 Boris Zbarsky [:bz] 2007-03-16 11:49:33 PDT
They were realized all along.  All it means in this case is someone filed a duplicate of this bug with the security flag set and someone else didn't mark it duplicate.
Comment 35 Dean Serenevy 2007-06-18 13:36:50 PDT
*** Bug 379493 has been marked as a duplicate of this bug. ***
Comment 36 Adam Guthrie 2007-06-26 18:19:22 PDT
*** Bug 384884 has been marked as a duplicate of this bug. ***
Comment 37 Mike Hommey [:glandium] 2007-07-31 22:04:46 PDT
(In reply to comment #20)
> (From update of attachment 127178 [details] [diff] [review])
> aww... it seems you are right, unfortunately :( the child process still has the
> file descriptors open

But shouldn't it close them ? If so, let's just add something like in comment #21 to NSPR.
Comment 38 Doug Turner (:dougt) 2007-10-08 16:08:28 PDT
mass reassigning to nobody.
Comment 39 Mike Schroepfer 2007-11-01 18:54:50 PDT
This come up recently: http://timesinker.blogspot.com/2007/11/found-big-bug-in-firefox-helper-apps.html
Comment 40 Karl Tomlinson (:karlt) 2007-11-02 04:57:16 PDT
(In reply to comment #12)
> The default inherit trait of a file descriptor opened via
> PR_Open() is inheritable on Unix and not-inheritable on Windows.

Any reason why we can't make the behavior of PR_Open() consistent on both platforms by getting PR_Open to make the file descriptor close-on-exec?

(It does get a little tricky with threads: it would be best if one thread didn't exec while another is between open and fcntl, but isn't NSPR the best place to deal with this?)
Comment 41 dean gaudet 2007-11-02 08:37:05 PDT
as you note you can't create any fd (i.e. via open/socket/accept/dup/dup2/F_DUPFD/...) without a race condition between the creation and setting F_CLOEXEC in a threaded program.  unix wasn't designed for threads.  there are a few choices:

(1) all fd creation under a mutex... gross.

(2) portable:  after fork() use dup2 to set up stdin/out/err then getrlimit(RLIMIT_NOFILE) and iterate for (int i = 2; i < N; ++i) { close(i); }

(3) not portable:  linux (and perhaps others) is slowly acquiring O_CLOEXEC, F_DUPFD_CLOEXEC and other such flags so that you can cause fds to be created with F_CLOEXEC set from the start.

imho (2) is the best solution.  you just have to catch all uses of fork().  i.e. popen() is hell.

-dean
Comment 42 Jeffrey Baker 2007-11-02 08:38:18 PDT
Karl: unfortunately this will break any normal fork that Mozilla does to communicate with itself or a subproc.  See comment #31.  If you make PR_Open set close-on-exec by default, most of the NSPR test suite breaks.
Comment 43 Karl Tomlinson (:karlt) 2007-11-02 16:06:50 PDT
(In reply to comment #31)
> Unfortunately in NSPR the same path is used to spawn an unrelated process
> and to fork your own process.  If you explicitly close all the FDs when NSPR
> forks, half the unit tests fail.

I didn't understand how this applied to a default close-on-exec approach.
Sure this would be a problem if explicitly closing all FDs in the sense of
choice (2) of comment #41.

But for a default close-on-exec approach, PR_SetFDInheritable would be needed
on any FD needed across exec.  (They would still be available after fork
but:)

Should an NSPR process wanting to fork use PR_CreateProcess to exec another
instance of itself?  And is that what Mozilla does?
http://developer.mozilla.org/en/docs/Process_Forking_in_NSPR

I guess PR_SetFDInheritable would be needed in quite a few places then.
And if it can't be done after fork, that could get complicated.
Comment 44 Karl Tomlinson (:karlt) 2007-11-02 16:08:41 PDT
(In reply to comment #41)
> (2) portable:  after fork() use dup2 to set up stdin/out/err then
> getrlimit(RLIMIT_NOFILE) and iterate for (int i = 2; i < N; ++i) { close(i); }

This would be best to watch out for race conditions too.
N could be large, but /proc/PID/fd could be an optimization I guess.

> 
> (3) not portable:  linux (and perhaps others) is slowly acquiring O_CLOEXEC,
> F_DUPFD_CLOEXEC and other such flags so that you can cause fds to be created
> with F_CLOEXEC set from the start.

This seemed ideal to me (and a portable library seemed the best way to decide
whether to use this) but I guess this is only going to help with open, not all
the fd creation functions.

> 
> imho (2) is the best solution.

I'm thinking this might be easiest.

If an external helper process like tenthumbs suggests is used, it wouldn't
need to worry about race conditions.  And once its a separate process, doing a
little work to close file descriptors is not a significant issue.

If we were already using the IPC daemon then that would seem the way to go.
If this or a similar daemon was launched early enough, then it would avoid the
problem of being unable to fork when using more than half the available
virtual memory (http://www.cmiss.org/cm/tracker/227/).  But maybe that's for
the future.

>  you just have to catch all uses of fork(). 
> i.e. popen() is hell.

:-) I'm hoping we are not using popen.
Comment 45 Boris Zbarsky [:bz] 2007-11-02 17:00:32 PDT
> :-) I'm hoping we are not using popen.

In general, we are.  Not for helper apps, though.

Obvious popen() users in the tree:

* Cycle collector debugging
* some camino unarchiver code
* JS engine code (whatever file_open is)
* Mailnews smime code
Comment 46 Mike Shaver (:shaver -- probably not reading bugmail closely) 2007-11-03 07:45:30 PDT
(In reply to comment #45)
> Obvious popen() users in the tree:
> * JS engine code (whatever file_open is)

Unused, unbuilt, unloved jsfile.c.  Nothing we're going to see in any product.
Comment 47 dean gaudet 2007-11-03 09:34:47 PDT
there's also pthread_atfork()...
Comment 48 timeless 2008-02-05 12:15:21 PST
*** Bug 415804 has been marked as a duplicate of this bug. ***
Comment 49 timeless 2008-02-05 12:26:24 PST
*** Bug 414841 has been marked as a duplicate of this bug. ***
Comment 50 Tobias Koenig 2008-02-05 12:56:58 PST
*** Bug 415808 has been marked as a duplicate of this bug. ***
Comment 51 Bob McElrath 2008-07-21 09:43:20 PDT
This bug hits me *every single day*.  I play mp3's (usually internet radio using rhythmbox or songbird) and use firefox extensively, reading lots of pdf documents with xpdf.  After some time, xpdf will block the sound device and I have to go track down which pid is blocking it and kill all the documents I have open (usually I have several).  The choice by ubuntu to use pulseaudio seems to make the situation worse.  I usually find it's the first thing I have to kill, then it's xpdf sessions, or firefox itself.

This may be a deeper bug.  Clearly it works with 2 pid's holding the same file handle, but shows up as more and more pid's hold the filehandle.  Exactly which circumstance leads to the sound device being blocked?  The reported programs which cause this (xpdf, evince, gv) do not read or write the sound device at all.  So why does it get blocked in the first place?  

Closing the file handle may or may not fix the problem depending on the ultimate reason that the sound device gets blocked.  Has anyone actually tested the theory that it's just holding file handles that causes the problem?  (e.g. with a helper script as suggested above?)
Comment 52 matt harrison 2009-01-09 09:32:10 PST
Have also seen this bug on gentoo and ubuntu (32 and 64 bit).  If I open a pdf from firefox (using kpdf), the next day the pdf will be stealing the sound devices and trying to play flash sound from mozilla causes a spike in cpu.  I usually resort to killing firefox and the app stealing the sound device.
Comment 53 Nicos Gollan 2009-04-08 01:40:51 PDT
This bug makes the "open with" functionality potentially dangerous, since it leaks file descriptors concerning hardware access (raw sound devices). Maybe "open with" should be disabled on affected systems to avoid security issues.
Comment 54 Mate Soos 2009-06-03 00:51:51 PDT
This bug is very annoying, as I regularly find Ktorrent (launched from Firefox) stealing away my soundcard (I debug with "fuser"). I am pretty sure many users actually don't realize it's due to Firefox that their sound doesn't work, and merely blame 'Linux' in general. I am convinced that fixing this bug would help many: it would be great to get this fixed after 7 years.
Comment 55 Nicos Gollan 2009-12-11 05:29:40 PST
This bug is embarassing. Of all the applications I'm using, Firefox is the /only/ one that reliably manages to break sound, of all functionality. Sound doesn't work? Find out what application was spawned by FF, terminate it, sound resumes.

It can't be that hard to iterate over and close all file descriptors >= 2.
Comment 56 Lennart Poettering 2010-02-20 16:18:30 PST
(In reply to comment #51)
> This bug hits me *every single day*.  I play mp3's (usually internet radio
> using rhythmbox or songbird) and use firefox extensively, reading lots of pdf
> documents with xpdf.  After some time, xpdf will block the sound device and I
> have to go track down which pid is blocking it and kill all the documents I
> have open (usually I have several).  The choice by ubuntu to use pulseaudio
> seems to make the situation worse.  I usually find it's the first thing I have
> to kill, then it's xpdf sessions, or firefox itself.

I am pretty sure that the PA libraries set O_CLOEXEC on all its fds. What makes you think PA is at fault here?

That said, regardless whether PA is misbehaving here or not, firefox definitely needs to close all open fds after fork()ing and before exec()ing an application. On linux this is best done by iterating through /proc/self/fd. Something like this:
http://git.0pointer.de/?p=pulseaudio.git;a=blob;f=src/pulsecore/core-util.c;h=b64c51e18fb71d2a111b0a0236bc81bc5d849950;hb=HEAD#l2230

That code is fast on Linux and still compatible with non-Linux Unixes, so should be a safe choice.
Comment 57 [:Aleksej] 2010-04-07 12:25:47 PDT
*** Bug 457272 has been marked as a duplicate of this bug. ***
Comment 58 Matthias Versen [:Matti] 2010-08-17 07:03:58 PDT
*** Bug 419489 has been marked as a duplicate of this bug. ***
Comment 59 Jeffrey Baker 2010-09-15 02:17:09 PDT
Just think how fun it will be to have a tenth birthday party for this bug in 2012 when it's still not fixed.  Security and platform parity used to be important features of the Mozilla project but not these days.
Comment 60 Damon Sicore (:damons) 2010-09-28 15:13:16 PDT
Roc, why is this blocking?  It's been around forever and we have a TON of bugs blocking 4.0 at this point.  Something has to give.
Comment 61 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-10-07 19:14:46 PDT
It's blocking because it's a pretty bad bug, we have no other procedure for getting bugs like this fixed, and I was hoping Karl would have time to do it without impacting anything else important.
Comment 62 Robert Sayre 2010-10-07 22:10:59 PDT
Comment 59 is nasty but correct. Sometimes you just have to put your foot down and fix it.
Comment 63 Boris Zbarsky [:bz] 2010-12-14 09:45:00 PST
We do need to fix this, but we're not going to hold Gecko 2 for it.  Moving to the 2.x list, pending Karl actually having time...
Comment 64 dean gaudet 2010-12-15 12:36:51 PST
yeah let's not fix this before it's a decade old.  not much longer to wait!
Comment 65 Philip Chee 2011-04-27 09:44:03 PDT
*** Bug 460936 has been marked as a duplicate of this bug. ***
Comment 66 Rogan Creswick 2011-04-27 10:27:16 PDT
Hey, we only have 13 months before the 10-year anniversary of this bug!

Here's a doodle for scheduling the party:

http://www.doodle.com/9uav5v7igabaxndv

*Maybe* we could get the ticket status updated to a status that actually exists  for the occasion :) "NEW" doesn't seem to be on the list anymore: https://bugzilla.mozilla.org/page.cgi?id=fields.html#status 

I won't go so far as to suggest RESOLVED, but IN_PROGRESS would be *awesome*.
Comment 67 Michael Ventnor 2011-05-10 03:55:24 PDT
So this is the culprit?
http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/unix/uxproces.c#263

How exactly do you trigger this? I thought download code used GIO or GnomeVFS now to spawn processes.

Is there any valid case for wanting to keep the file descriptors open after a fork? I was thinking that, in that function ForkAndExec(), if attr is null then I close the fds. karl, thoughts?
Comment 68 Bob McElrath 2011-05-10 04:12:28 PDT
Some relevant reference and code over at stackoverflow:

http://stackoverflow.com/questions/5713242/linux-fork-file-descriptors-inheritance
Comment 69 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-10 04:58:05 PDT
Michael, I'd love it if you could take this.
Comment 70 Mike Hommey [:glandium] 2011-05-10 05:06:09 PDT
(In reply to comment #67)
> So this is the culprit?
> http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/unix/
> uxproces.c#263
> 
> How exactly do you trigger this? I thought download code used GIO or
> GnomeVFS now to spawn processes.
>
> Is there any valid case for wanting to keep the file descriptors open after
> a fork? I was thinking that, in that function ForkAndExec(), if attr is null
> then I close the fds. karl, thoughts?

My 2 cents:
- The problem is not keeping the file descriptors open after a fork, but after an exec.
- There are a lot of valid reasons to keep file descriptors open after a fork
- There are a few valid reasons to keep file descriptors open after an exec, but I don't think any of these reasons are in the scope of a browser
- Modifying ForkAndExec() behaviour in nspr means a change in behaviour in all applications using NSPR, which may or may not expect file descriptors not to be closed.
- As you pointed out, nowadays GIO and GnomeVFS is used (except for mailcap entries), but I don't think they close file descriptors either.
Comment 71 Michael Ventnor 2011-05-10 05:42:20 PDT
wtc doesn't want the proposed approach as he mentioned in bug 372734. We need to find a way to do this without touching NSPR.

This might possibly mean auditing all PRFileDesc* in mozilla-central and calling PR_SetFDInheritable(..., PR_FALSE) on them....
Comment 72 Michael Ventnor 2011-05-10 06:11:45 PDT
Doing some more code searching, the most important place to put this call might be nsLocalFile::OpenNSPRFileDesc (and possibly OpenANSIFileDesc). Writing that here for my own records.
Comment 73 Karl Tomlinson (:karlt) 2011-05-10 17:57:38 PDT
gnome_vfs_mime_application_launch_with_env calls g_spawn_async without
G_SPAWN_LEAVE_DESCRIPTORS_OPEN so helper apps should be fine with GnomeVFS.

Non-GNOME (and newer GNOME) systems without --enable-gio will use nsIProcess
which uses PR_CreateProcess, thus hitting this bug.

Helper apps should be fine in --enable-gio builds because
g_desktop_app_info_launch_uris also calls g_spawn_async without
G_SPAWN_LEAVE_DESCRIPTORS_OPEN.

For plugin/content processes this is addressed by CloseSuperfluousFds.

Most systems should have GIO these days, so it looks like the appropriate fix
here is to update glib on our build systems so we can --enable-gio.

(I haven't done an audit of other nsIProcess consumers.)
Comment 74 Michael Ventnor 2011-05-10 20:41:04 PDT
Ideally at the same time we can install GTK3 on our build systems and migrate to that asap.
Comment 75 Mike Hommey [:glandium] 2011-10-03 03:03:14 PDT
(In reply to Karl Tomlinson (:karlt) from comment #73)
> Helper apps should be fine in --enable-gio builds because
> g_desktop_app_info_launch_uris also calls g_spawn_async without
> G_SPAWN_LEAVE_DESCRIPTORS_OPEN.

Actually, Non-GNOME systems won't be covered with --enable-gio, because the gio service is in the mozgnome component, which depends on GNOME libs, which will most likely fail to load on non-GNOME systems. And if mozgnome loads anyway, it contains the GnomeVFSService, which would already use gnome_vfs_mime_application_launch. So --enable-gio would change nothing.
Comment 76 Karl Tomlinson (:karlt) 2011-10-03 18:03:29 PDT
(In reply to Mike Hommey [:glandium] from comment #75)
> Actually, Non-GNOME systems won't be covered with --enable-gio, because the
> gio service is in the mozgnome component, which depends on GNOME libs, which
> will most likely fail to load on non-GNOME systems.

We'd have to either --disable-gconf --disable-gnomevfs (as I expect GIO distributions will do), or make mozgnome dynamically check for gconf and gnomevfs if still useful.
Comment 77 Mike Hommey [:glandium] 2011-10-03 22:45:16 PDT
Also note that mozgnome also may depend on libnotify. (mozilla builds don't but distros' do)
Comment 78 Karl Tomlinson (:karlt) 2011-10-04 16:10:20 PDT
Mozilla builds depend on libnotify.so.1.  The ABI and soname has since changed, so that may be a problem on newer distros.  If there's no stable notification library we can use instead, then we could dynamically detect.
Comment 79 Chris Coulson 2011-10-09 13:14:16 PDT
Actually, I was thinking about this last week, but why do we have the gio support in the mozgnome component? gio exists on any system with a gtk version of 2.14 or newer (the version there is just plucked from a brief look through the git history of gtk)

And if we modified the mozgnome component to dynamically check for gconf/gnomevfs, then is there any point to having the separate component for them anyway? (perhaps I'm missing the point of putting them in libmozgnome in the first place though?)
Comment 80 Chris Coulson 2011-10-09 13:20:49 PDT
Ah, I answered my own question - the minimum gtk requirement is 2.10
Comment 81 Mike Hommey [:glandium] 2011-10-14 06:32:32 PDT
Something else: mailcap support uses nsIProcess to start commands. These wouldn't be covered either.
Comment 82 Patrick McManus [:mcmanus] 2016-01-29 13:44:07 PST
*** Bug 372734 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.