Add routine to close file descriptors on exec

Add routine to close file descriptors on exec
for linux and mac. See BUG 6598.

[jeremy, 2009-01-26 19:24:41]

Very nice :)

http://codereview.chromium.org/18801/diff/1/2
File base/process_util.h (right):

http://codereview.chromium.org/18801/diff/1/2#newcode76
Line 76: int SetAllFDsToCloseOnExec();
Should return a bool.

http://codereview.chromium.org/18801/diff/1/3
File base/process_util_linux.cc (right):

http://codereview.chromium.org/18801/diff/1/3#newcode47
Line 47: SetAllFDsToCloseOnExec();
Check return value and abort on failure.

http://codereview.chromium.org/18801/diff/1/4
File base/process_util_mac.mm (right):

http://codereview.chromium.org/18801/diff/1/4#newcode34
Line 34: SetAllFDsToCloseOnExec();
Check return value.

http://codereview.chromium.org/18801/diff/1/5
File base/process_util_posix.cc (right):

http://codereview.chromium.org/18801/diff/1/5#newcode18
Line 18: #include <stdlib.h>
#includes should be sorted alphabetically.

http://codereview.chromium.org/18801/diff/1/5#newcode73
Line 73: // on the supported platforms.
I'm not sure your comment is correct, if support 64 bit architectures then
sizeof(int) will be 8.

The reason for limiting things in this way is so the brute force FD closing
didn't loop for too long.

IMHO if you are no longer looping over all possible numbers from 3..the max open
FD #, you can safely remove this cast and just return rlimit.rlim_cur directly.

http://codereview.chromium.org/18801/diff/1/5#newcode84
Line 84: int SetAllFDsToCloseOnExec() {
The convention we've been using is to return success failure as a bool rather
than an int.

http://codereview.chromium.org/18801/diff/1/5#newcode94
Line 94: while ((ent = readdir(dir))) {
No need for nested parenthesis.

http://codereview.chromium.org/18801/diff/1/5#newcode95
Line 95: // Skip . and .. entries.
if ent is NULL, return false.

http://codereview.chromium.org/18801/diff/1/5#newcode104
Line 104: fcntl(i, F_SETFD, flags | FD_CLOEXEC);
Check return value of fcntl.

http://codereview.chromium.org/18801/diff/1/5#newcode106
Line 106: return 0;
closedir();
It'd be much safer to create a scoped type (using the unfortunately named
scoped_malloc) that automatically calls closedir when it goes out of scope.

See line 344 of http://codereview.chromium.org/18530/diff/226/229?context=50 for
an example.

[Mark Mentovai, 2009-01-27 05:22:23]

http://codereview.chromium.org/18801/diff/1/2
File base/process_util.h (right):

http://codereview.chromium.org/18801/diff/1/2#newcode72
Line 72: int GetMaxFilesOpenInProcess();
Does anyone call this anymore?  If not, we should get rid of it.

http://codereview.chromium.org/18801/diff/1/5
File base/process_util_posix.cc (right):

http://codereview.chromium.org/18801/diff/1/5#newcode18
Line 18: #include <stdlib.h>
Jeremy wrote:
> #includes should be sorted alphabetically.

Note that C system headers should precede C++ ones, so this should end in
#include <limits>.  Also, there should be a blank line between the C and C++
includes so that it's obvious that they're different sections.

http://codereview.chromium.org/18801/diff/1/5#newcode84
Line 84: int SetAllFDsToCloseOnExec() {
I think this function should return void.  Nobody checks its return value and
it's not likely anyone will.  All that anyone would do is log a message and then
try to do whatever they were going to do anyway.  Maybe instead of returning 0
or false or whatever, unexpected conditions can just have a DLOG.

http://codereview.chromium.org/18801/diff/1/5#newcode88
Line 88: #if defined(OS_MACOSX)
Use #elif instead of #endif #if.

I prefer this:

#if defined(OS_LINUX)
  const char fd_dir[] = "/proc/self/fd";
#elif defined(OS_MACOSX)
  const char fd_dir[] = "/dev/fd";
#endif
  DIR* dir = opendir(fd_dir);

http://codereview.chromium.org/18801/diff/1/5#newcode94
Line 94: while ((ent = readdir(dir))) {
Jeremy wrote:
> No need for nested parenthesis.

The extra () avoids a gcc warning.  If you write:

while (ent = readdir(dir))

gcc assumes you may have meant == and generates a warning.  The canonical way to
assert that you really mean assignment is to use ((expr)).

http://codereview.chromium.org/18801/diff/1/5#newcode95
Line 95: // Skip . and .. entries.
Jeremy wrote:
> if ent is NULL, return false.

Huh?

http://codereview.chromium.org/18801/diff/1/5#newcode99
Line 99: // We don't close stdin, stdout or stderr.
Do we ever close stdin, stdout, or stderr elsewhere?  If so, we need to be
careful here: their FDs may be reused for something else and we would want to
set FD_CLOEXEC.

http://codereview.chromium.org/18801/diff/1/5#newcode106
Line 106: return 0;
Jeremy wrote:
> closedir();
> It'd be much safer to create a scoped type

Strongly agree.  You can just introduce an anonymous AutoDirCloser here in this
file too.

http://codereview.chromium.org/18801/diff/1/5#newcode106
Line 106: return 0;
Really give up?  I don't think so, I think we should keep on going through the
FDs.  Assume another thread closed an FD after we got the directory entry but
before we called fcntl.  That shouldn't ruin the party, should it?

[Craig Schlenter, 2009-01-27 05:58:09]

Thanks for the comments everyone ... I'll probably only get to fixing this stuff
up tomorrow as I have $DAYJOB issues to tackle but I'll quickly comment on some
issues:

http://en.wikipedia.org/wiki/64-bit says that most of the targets should be LP64
i.e. ints are 32 bits even on the 64 bit platforms and thus I think the comment
change I made is correct. The getrlimit rlim_t versus int mismatch is really
because rlim_t was bumped up to 64 bits to handle some of the other resources
that can be requested AFAIK. On my machine (and I suspect most distributions),
the file limit will probably be about 1024 unless someone messed with ulimit.

GetMaxFilesOpenInProcess is still called in the unit test but it may be possible
to kill it off or shove it into the unit test and avoid bloating up base with it
...

Thank you!

[Craig Schlenter, 2009-01-28 07:21:51]

On 2009/01/27 05:22:23, Mark Mentovai wrote:
> http://codereview.chromium.org/18801/diff/1/2
> File base/process_util.h (right):

I am returning bool rather than int now as Jeremy suggested although the callers
still ignore the return.
 
> http://codereview.chromium.org/18801/diff/1/2#newcode72
> Line 72: int GetMaxFilesOpenInProcess();
> Does anyone call this anymore?  If not, we should get rid of it.

I've moved it into the unit test as nothing else uses it. The unit test uses it
to loop over the FDs and close them to see if any open FDs were leaked into the
child. I kept the new comment though.

> http://codereview.chromium.org/18801/diff/1/5
> File base/process_util_posix.cc (right):
> 
> http://codereview.chromium.org/18801/diff/1/5#newcode18
> Line 18: #include <stdlib.h>
> Jeremy wrote:
> > #includes should be sorted alphabetically.
> 
> Note that C system headers should precede C++ ones, so this should end in
> #include <limits>.  Also, there should be a blank line between the C and C++
> includes so that it's obvious that they're different sections.

I've sorted the headers and added a blank line etc.

> http://codereview.chromium.org/18801/diff/1/5#newcode84
> Line 84: int SetAllFDsToCloseOnExec() {
> I think this function should return void.  Nobody checks its return value and
> it's not likely anyone will.  All that anyone would do is log a message and
then
> try to do whatever they were going to do anyway.  Maybe instead of returning 0
> or false or whatever, unexpected conditions can just have a DLOG.

I have a DLOG now for the two possible errors. The function still returns a bool
though in case a caller wants to take some action based on the failure. Current
callers ignore the return value.

> http://codereview.chromium.org/18801/diff/1/5#newcode88
> Line 88: #if defined(OS_MACOSX)
> Use #elif instead of #endif #if.
> 
> I prefer this:
> 
> #if defined(OS_LINUX)
>   const char fd_dir[] = "/proc/self/fd";
> #elif defined(OS_MACOSX)
>   const char fd_dir[] = "/dev/fd";
> #endif
>   DIR* dir = opendir(fd_dir);

Done but with a scoped dircloser thingy too ...

> http://codereview.chromium.org/18801/diff/1/5#newcode94
> Line 94: while ((ent = readdir(dir))) {
> Jeremy wrote:
> > No need for nested parenthesis.
> 
> The extra () avoids a gcc warning.  If you write:
> 
> while (ent = readdir(dir))
> 
> gcc assumes you may have meant == and generates a warning.  The canonical way
to
> assert that you really mean assignment is to use ((expr)).
> 
> http://codereview.chromium.org/18801/diff/1/5#newcode95
> Line 95: // Skip . and .. entries.
> Jeremy wrote:
> > if ent is NULL, return false.
> 
> Huh?

unchanged.

> http://codereview.chromium.org/18801/diff/1/5#newcode99
> Line 99: // We don't close stdin, stdout or stderr.
> Do we ever close stdin, stdout, or stderr elsewhere?  If so, we need to be
> careful here: their FDs may be reused for something else and we would want to
> set FD_CLOEXEC.

Agreed ... I'll do some grepping to see if I spot anything closing stderr etc.

> http://codereview.chromium.org/18801/diff/1/5#newcode106
> Line 106: return 0;
> Jeremy wrote:
> > closedir();
> > It'd be much safer to create a scoped type
> 
> Strongly agree.  You can just introduce an anonymous AutoDirCloser here in
this
> file too.

I've created the scoped type. It feels like a bit of an overkill to me as I did
fix the logic in the code so that if you do stick a closedir before the last
return it won't leak. Let me know if you want a patch that just does the
closedir.

> http://codereview.chromium.org/18801/diff/1/5#newcode106
> Line 106: return 0;
> Really give up?  I don't think so, I think we should keep on going through the
> FDs.  Assume another thread closed an FD after we got the directory entry but
> before we called fcntl.  That shouldn't ruin the party, should it?

I keep looping now even if there is an error on one FD.

Thank you!

[Mark Mentovai, 2009-01-28 14:29:34]

Looks good for the most part.  A few more minor nits, there's really nothing of
functional significance.  Nice job.

http://codereview.chromium.org/18801/diff/401/604
File base/process_util_posix.cc (right):

http://codereview.chromium.org/18801/diff/401/604#newcode77
Line 77: bool SetAllFDsToCloseOnExec() {
I still don't agree with having this return a type, but I'm not going to fight
over it any more.

I think that if someone ever does want a caller to use the status of this
function, they can go in and put the return type in themselves.

Some of the rest of the comments here only apply if we're really going to keep
the return value.

http://codereview.chromium.org/18801/diff/401/604#newcode87
Line 87: return 0;
If you want the return type to be bool, you should be dealing in true and false,
not 0 and 1.

http://codereview.chromium.org/18801/diff/401/604#newcode88
Line 88: }
Here - after this line - is a good spot for a blank line.  I like to put blank
lines after early returns to say "OK, done checking for that weird case, let's
move on to something else now."

http://codereview.chromium.org/18801/diff/401/604#newcode95
Line 95: int i =  atoi(ent->d_name);
Why the extra space between the = and atoi?

http://codereview.chromium.org/18801/diff/401/604#newcode99
Line 99: int flags = fcntl(i, F_GETFD);
The line before this one is a ripe candidate for a blank line.  We're done
checking the exclusion cases, and are moving on to the real meat.  Blank lines
between logical sections help keep things readable.

http://codereview.chromium.org/18801/diff/401/604#newcode105
Line 105: return ret;
And again, a blank line before this return would be nice.  "Done with that
medium-sized loop, let's do something else now" (incidentally, LEAVE!)

http://codereview.chromium.org/18801/diff/401/605
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/18801/diff/401/605#newcode146
Line 146: struct rlimit rlimit;
I would not name an instantiation the same as its type, or any type for that
matter.  (The problem probably came from the code you're swiping from the other
file, but since you're moving it, you get to fix it.)

http://codereview.chromium.org/18801/diff/401/605#newcode152
Line 152: // We do this since FD #s are ints which are all 32 bits
This sentence looks weird because you're only using 3/4 of the 80 columns you're
allowed.

[Craig Schlenter, 2009-01-29 05:58:06]

All fixed I hope. I've only tested this on linux 'cos my mac is PPC.

http://codereview.chromium.org/18801/diff/401/604
File base/process_util_posix.cc (right):

http://codereview.chromium.org/18801/diff/401/604#newcode77
Line 77: bool SetAllFDsToCloseOnExec() {
On 2009/01/28 14:29:34, Mark Mentovai wrote:
> I still don't agree with having this return a type, but I'm not going to fight
> over it any more.
> 
> I think that if someone ever does want a caller to use the status of this
> function, they can go in and put the return type in themselves.
> 
> Some of the rest of the comments here only apply if we're really going to keep
> the return value.

I've made it void now.

http://codereview.chromium.org/18801/diff/401/604#newcode87
Line 87: return 0;
On 2009/01/28 14:29:34, Mark Mentovai wrote:
> If you want the return type to be bool, you should be dealing in true and
false,
> not 0 and 1.

Changed into return since function is void now.

http://codereview.chromium.org/18801/diff/401/604#newcode88
Line 88: }
On 2009/01/28 14:29:34, Mark Mentovai wrote:
> Here - after this line - is a good spot for a blank line.  I like to put blank
> lines after early returns to say "OK, done checking for that weird case, let's
> move on to something else now."

Done.

http://codereview.chromium.org/18801/diff/401/604#newcode95
Line 95: int i =  atoi(ent->d_name);
On 2009/01/28 14:29:34, Mark Mentovai wrote:
> Why the extra space between the = and atoi?

Fixed. Either too little or too much coffee involved when I typed that.

http://codereview.chromium.org/18801/diff/401/604#newcode99
Line 99: int flags = fcntl(i, F_GETFD);
On 2009/01/28 14:29:34, Mark Mentovai wrote:
> The line before this one is a ripe candidate for a blank line.  We're done
> checking the exclusion cases, and are moving on to the real meat.  Blank lines
> between logical sections help keep things readable.

Done.

http://codereview.chromium.org/18801/diff/401/604#newcode105
Line 105: return ret;
On 2009/01/28 14:29:34, Mark Mentovai wrote:
> And again, a blank line before this return would be nice.  "Done with that
> medium-sized loop, let's do something else now" (incidentally, LEAVE!)

The return is gone completely since the function is void.

http://codereview.chromium.org/18801/diff/401/605
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/18801/diff/401/605#newcode146
Line 146: struct rlimit rlimit;
On 2009/01/28 14:29:34, Mark Mentovai wrote:
> I would not name an instantiation the same as its type, or any type for that
> matter.  (The problem probably came from the code you're swiping from the
other
> file, but since you're moving it, you get to fix it.)

Called it rlim now which matches the man page.

http://codereview.chromium.org/18801/diff/401/605#newcode152
Line 152: // We do this since FD #s are ints which are all 32 bits
On 2009/01/28 14:29:34, Mark Mentovai wrote:
> This sentence looks weird because you're only using 3/4 of the 80 columns
you're
> allowed.

Merged both comments onto 1 line. I think it looks better. I also added the
function description comment to the top of the function that I forgot to move
originally.

[Mark Mentovai, 2009-01-29 15:01:58]

LGTM

Craig, have you already done the CLA dance?

[Craig Schlenter, 2009-01-29 15:37:43]

On 2009/01/29 15:01:58, Mark Mentovai wrote:
> LGTM
> 
> Craig, have you already done the CLA dance?

Yes, thank you.