POSIX: don't allocate memory after forking.

POSIX: don't allocate memory after forking.

Previously we would allocate memory in the child process. However, the
allocation might have happened while the malloc lock was held,
resulting in a deadlock.

This patch removes allocation from the child but probably makes Mac's
startup time slower until a Mac person can implement
dir_reader_posix.h.

TEST=Unittest for new code
BUG=36678

[Paweł Hajdan Jr., 2010-03-08 21:22:28]

Drive-by with some test-related comments.

http://codereview.chromium.org/672003/diff/9/1011
File base/dir_reader_posix_unittest.cc (right):

http://codereview.chromium.org/672003/diff/9/1011#newcode17
base/dir_reader_posix_unittest.cc:17: namespace {
nit: Why not put everything in the anonymous namespace? Putting just the typedef
in it doesn't give us anything, as typedefs don't have external linkage anyway.

http://codereview.chromium.org/672003/diff/9/1011#newcode29
base/dir_reader_posix_unittest.cc:29: CHECK(dir);
How about making this ASSERT_TRUE to avoid crashing the entire base_unittests
binary in case of failure?

http://codereview.chromium.org/672003/diff/9/1011#newcode32
base/dir_reader_posix_unittest.cc:32: CHECK(prev_wd >= 0);
Similarly here and also later.

http://codereview.chromium.org/672003/diff/9/1011#newcode54
base/dir_reader_posix_unittest.cc:54: for (; reader.Next(); ) {
Please put some SCOPED_TRACE here so we know the current state in case of
assertion failure. A possible candidate for the message would be reader.name().

[agl, 2010-03-08 21:42:23]

http://codereview.chromium.org/672003/diff/9/1011
File base/dir_reader_posix_unittest.cc (right):

http://codereview.chromium.org/672003/diff/9/1011#newcode17
base/dir_reader_posix_unittest.cc:17: namespace {
On 2010/03/08 21:22:28, Paweł Hajdan Jr. wrote:
> nit: Why not put everything in the anonymous namespace? Putting just the
typedef
> in it doesn't give us anything, as typedefs don't have external linkage
anyway.

I'm just working from the boilerplate here but you're right. The namespace might
as well cover everything.

http://codereview.chromium.org/672003/diff/9/1011#newcode29
base/dir_reader_posix_unittest.cc:29: CHECK(dir);
On 2010/03/08 21:22:28, Paweł Hajdan Jr. wrote:
> How about making this ASSERT_TRUE to avoid crashing the entire base_unittests
> binary in case of failure?

The things that are asserts are things that could reasonably fail. The things
that are CHECKs are things that mean your computer is on fire. I only check them
at all out of habit. It's not worth the extra code to return if they fail.

http://codereview.chromium.org/672003/diff/9/1011#newcode54
base/dir_reader_posix_unittest.cc:54: for (; reader.Next(); ) {
On 2010/03/08 21:22:28, Paweł Hajdan Jr. wrote:
> Please put some SCOPED_TRACE here so we know the current state in case of
> assertion failure. A possible candidate for the message would be
reader.name().

Done.

[willchan no longer on Chromium, 2010-03-09 00:32:40]

Should there also be a base.gypi change to add the new dir_reader_*.h files?

http://codereview.chromium.org/672003/diff/11/13
File base/dir_reader_linux.h (right):

http://codereview.chromium.org/672003/diff/11/13#newcode5
base/dir_reader_linux.h:5: #ifndef BASE_DIR_READER_LINUX_H
Your copyright dates are off and the ifndef guard is missing a trailing
underscore after the H.

http://codereview.chromium.org/672003/diff/11/13#newcode10
base/dir_reader_linux.h:10: #include <unistd.h>
You have unistd.h twice here.

http://codereview.chromium.org/672003/diff/11/13#newcode31
base/dir_reader_linux.h:31: DirReaderPosix(const char* directory_path)
explicit

http://codereview.chromium.org/672003/diff/11/13#newcode35
base/dir_reader_linux.h:35: }
Maybe you should print an error if open() fails.

http://codereview.chromium.org/672003/diff/11/13#newcode39
base/dir_reader_linux.h:39: close(fd_);
technically it's a minor bug if you don't check the return value of close.  not
a big deal, but I generally log an ERROR if it fails.

http://codereview.chromium.org/672003/diff/11/13#newcode60
base/dir_reader_linux.h:60: DCHECK(false) << "getdents64 returned an error: " <<
errno;
DPLOG(FATAL) would probably be better here.  However, the LOG implementation
will actually use memory, so this code might cause us to crash as we attempt to
write the LOG message and crash thereafter.  Not a big deal since we were going
to crash anyway, but it's a shame to potentially lose the error message.  Maybe
you should add a RAW_DCHECK based on the RAW_CHECK in base/logging.h.  Not a big
deal either way though.

http://codereview.chromium.org/672003/diff/11/13#newcode89
base/dir_reader_linux.h:89: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/672003/diff/11/14
File base/dir_reader_posix.h (right):

http://codereview.chromium.org/672003/diff/11/14#newcode5
base/dir_reader_posix.h:5: #ifndef BASE_DIR_READER_POSIX_H
Your copyright date is off again and so is the include guard.

http://codereview.chromium.org/672003/diff/11/14#newcode13
base/dir_reader_posix.h:13: #include "base/dir_reader_linux.h"
I think I prefer the solution we use in net for the various platform specific
implementations.  Please refer to net/socket/tcp_client_socket.h.  It uses the
OS definitions to choose which header to include and which typedef to use.  It
seems confusing in your other file that dir_reader_linux.h defines a class
called DirReaderPosix and I expect it to be confusing when the Mac version comes
along too.

http://codereview.chromium.org/672003/diff/11/15
File base/dir_reader_posix_unittest.cc (right):

http://codereview.chromium.org/672003/diff/11/15#newcode1
base/dir_reader_posix_unittest.cc:1: // Copyright (c) 2009 The Chromium Authors.
All rights reserved.
Copyright is old.

http://codereview.chromium.org/672003/diff/11/15#newcode6
base/dir_reader_posix_unittest.cc:6: #include "base/dir_reader_posix.h"
This header should come first in the file.  If you don't take my suggestion to
use a dir_reader.h that forwards to the appropriate header and does the
appropriate typedef, then you should move the build_config.h include to
dir_reader_posix.h

http://codereview.chromium.org/672003/diff/11/15#newcode18
base/dir_reader_posix_unittest.cc:18: typedef testing::Test
DirReaderPosixUnittest;
I'm not sure I understand the need for this.

http://codereview.chromium.org/672003/diff/11/15#newcode31
base/dir_reader_posix_unittest.cc:31: CHECK(prev_wd >= 0);
CHECK_GE

http://codereview.chromium.org/672003/diff/11/15#newcode37
base/dir_reader_posix_unittest.cc:37: snprintf(buf, sizeof(buf), "%d", i);
you should include stdio.h for snprintf()

http://codereview.chromium.org/672003/diff/11/15#newcode54
base/dir_reader_posix_unittest.cc:54: if (strcmp(reader.name(), ".") == 0) {
you should include string.h for strcmp

http://codereview.chromium.org/672003/diff/11/15#newcode70
base/dir_reader_posix_unittest.cc:70: EXPECT_EQ(seen.count(value), 0u);
the order should be expected, actual for gtest's output to read correctly.

http://codereview.chromium.org/672003/diff/11/15#newcode77
base/dir_reader_posix_unittest.cc:77: unlink(buf);
PCHECK?

http://codereview.chromium.org/672003/diff/11/15#newcode80
base/dir_reader_posix_unittest.cc:80: rmdir(dir);
PCHECK?

http://codereview.chromium.org/672003/diff/11/15#newcode87
base/dir_reader_posix_unittest.cc:87: EXPECT_EQ(seen.size(), kNumFiles);
reverse the order of parameters

http://codereview.chromium.org/672003/diff/11/16
File base/file_descriptor_shuffle.cc (right):

http://codereview.chromium.org/672003/diff/11/16#newcode16
base/file_descriptor_shuffle.cc:16: (InjectiveMultimap* m, InjectionDelegate*
delegate) {
opening paren should go on prev line

http://codereview.chromium.org/672003/diff/11/16#newcode40
base/file_descriptor_shuffle.cc:40: extra_fds[next_extra_fd++] = temp_fd;
So, this function silently fails if there are too many file descriptors?  That
sounds problematic.  Maybe we should RAW_LOG if we hit this condition?

http://codereview.chromium.org/672003/diff/11/16#newcode71
base/file_descriptor_shuffle.cc:71: bool PerformInjectiveMultimap(const
InjectiveMultimap &m_in,
the & should go next to the type, since you do that for *.  they should be
consistent.

http://codereview.chromium.org/672003/diff/11/18
File base/process_util_posix.cc (right):

http://codereview.chromium.org/672003/diff/11/18#newcode272
base/process_util_posix.cc:272: // TODO(agl): Remove this function. It's
fundamentally broken for multithreaded
Can't we just delete this function now? I don't see where it's used.

http://codereview.chromium.org/672003/diff/11/18#newcode417
base/process_util_posix.cc:417: #endif
This probably works, although I'd be somewhat concerned about collateral damage,
removing execute perms on that page might cover some non-malloc code too.  But
hey, this is just for debugging anyway.

http://codereview.chromium.org/672003/diff/11/18#newcode436
base/process_util_posix.cc:436: setenv(it->first.c_str(), it->second.c_str(),
1);
setenv() and unsetenv() both use locks.  setenv() can call realloc() and
malloc().

http://codereview.chromium.org/672003/diff/11/18#newcode461
base/process_util_posix.cc:461: PLOG(ERROR) << "LaunchApp: execvp(" <<
argv_cstr[0] << ") failed";
Maybe this should be RAW_LOG too.

[Paweł Hajdan Jr., 2010-03-09 06:37:28]

The bits I commented in the drive-by LGTM

[agl, 2010-03-09 18:56:12]

PTAL

http://codereview.chromium.org/672003/diff/11/13
File base/dir_reader_linux.h (right):

http://codereview.chromium.org/672003/diff/11/13#newcode5
base/dir_reader_linux.h:5: #ifndef BASE_DIR_READER_LINUX_H
On 2010/03/09 00:32:40, willchan wrote:
> Your copyright dates are off and the ifndef guard is missing a trailing
> underscore after the H.

Done.

http://codereview.chromium.org/672003/diff/11/13#newcode10
base/dir_reader_linux.h:10: #include <unistd.h>
On 2010/03/09 00:32:40, willchan wrote:
> You have unistd.h twice here.

Done.

http://codereview.chromium.org/672003/diff/11/13#newcode31
base/dir_reader_linux.h:31: DirReaderPosix(const char* directory_path)
On 2010/03/09 00:32:40, willchan wrote:
> explicit

Done.

http://codereview.chromium.org/672003/diff/11/13#newcode35
base/dir_reader_linux.h:35: }
On 2010/03/09 00:32:40, willchan wrote:
> Maybe you should print an error if open() fails.

In this case it's up to the caller to deal with the error.

http://codereview.chromium.org/672003/diff/11/13#newcode39
base/dir_reader_linux.h:39: close(fd_);
On 2010/03/09 00:32:40, willchan wrote:
> technically it's a minor bug if you don't check the return value of close. 
not
> a big deal, but I generally log an ERROR if it fails.

Done.

http://codereview.chromium.org/672003/diff/11/13#newcode60
base/dir_reader_linux.h:60: DCHECK(false) << "getdents64 returned an error: " <<
errno;
On 2010/03/09 00:32:40, willchan wrote:
> DPLOG(FATAL) would probably be better here.  However, the LOG implementation
> will actually use memory, so this code might cause us to crash as we attempt
to
> write the LOG message and crash thereafter.  Not a big deal since we were
going
> to crash anyway, but it's a shame to potentially lose the error message. 
Maybe
> you should add a RAW_DCHECK based on the RAW_CHECK in base/logging.h.  Not a
big
> deal either way though.

Done.

http://codereview.chromium.org/672003/diff/11/13#newcode89
base/dir_reader_linux.h:89: };
On 2010/03/09 00:32:40, willchan wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/672003/diff/11/14
File base/dir_reader_posix.h (right):

http://codereview.chromium.org/672003/diff/11/14#newcode5
base/dir_reader_posix.h:5: #ifndef BASE_DIR_READER_POSIX_H
On 2010/03/09 00:32:40, willchan wrote:
> Your copyright date is off again and so is the include guard.

Done.

http://codereview.chromium.org/672003/diff/11/14#newcode13
base/dir_reader_posix.h:13: #include "base/dir_reader_linux.h"
On 2010/03/09 00:32:40, willchan wrote:
> I think I prefer the solution we use in net for the various platform specific
> implementations.  Please refer to net/socket/tcp_client_socket.h.  It uses the
> OS definitions to choose which header to include and which typedef to use.  It
> seems confusing in your other file that dir_reader_linux.h defines a class
> called DirReaderPosix and I expect it to be confusing when the Mac version
comes
> along too.

Done.

http://codereview.chromium.org/672003/diff/11/15
File base/dir_reader_posix_unittest.cc (right):

http://codereview.chromium.org/672003/diff/11/15#newcode1
base/dir_reader_posix_unittest.cc:1: // Copyright (c) 2009 The Chromium Authors.
All rights reserved.
On 2010/03/09 00:32:40, willchan wrote:
> Copyright is old.

Done.

http://codereview.chromium.org/672003/diff/11/15#newcode6
base/dir_reader_posix_unittest.cc:6: #include "base/dir_reader_posix.h"
On 2010/03/09 00:32:40, willchan wrote:
> This header should come first in the file.  If you don't take my suggestion to
> use a dir_reader.h that forwards to the appropriate header and does the
> appropriate typedef, then you should move the build_config.h include to
> dir_reader_posix.h

Done.

http://codereview.chromium.org/672003/diff/11/15#newcode18
base/dir_reader_posix_unittest.cc:18: typedef testing::Test
DirReaderPosixUnittest;
On 2010/03/09 00:32:40, willchan wrote:
> I'm not sure I understand the need for this.

Just following the boilerplate.

http://codereview.chromium.org/672003/diff/11/15#newcode31
base/dir_reader_posix_unittest.cc:31: CHECK(prev_wd >= 0);
On 2010/03/09 00:32:40, willchan wrote:
> CHECK_GE

Done.

http://codereview.chromium.org/672003/diff/11/15#newcode37
base/dir_reader_posix_unittest.cc:37: snprintf(buf, sizeof(buf), "%d", i);
On 2010/03/09 00:32:40, willchan wrote:
> you should include stdio.h for snprintf()

Done.

http://codereview.chromium.org/672003/diff/11/15#newcode54
base/dir_reader_posix_unittest.cc:54: if (strcmp(reader.name(), ".") == 0) {
On 2010/03/09 00:32:40, willchan wrote:
> you should include string.h for strcmp

Done.

http://codereview.chromium.org/672003/diff/11/15#newcode70
base/dir_reader_posix_unittest.cc:70: EXPECT_EQ(seen.count(value), 0u);
On 2010/03/09 00:32:40, willchan wrote:
> the order should be expected, actual for gtest's output to read correctly.

Done.

http://codereview.chromium.org/672003/diff/11/15#newcode77
base/dir_reader_posix_unittest.cc:77: unlink(buf);
On 2010/03/09 00:32:40, willchan wrote:
> PCHECK?

Done.

http://codereview.chromium.org/672003/diff/11/15#newcode80
base/dir_reader_posix_unittest.cc:80: rmdir(dir);
On 2010/03/09 00:32:40, willchan wrote:
> PCHECK?

Done.

http://codereview.chromium.org/672003/diff/11/15#newcode87
base/dir_reader_posix_unittest.cc:87: EXPECT_EQ(seen.size(), kNumFiles);
On 2010/03/09 00:32:40, willchan wrote:
> reverse the order of parameters

Done.

http://codereview.chromium.org/672003/diff/11/16
File base/file_descriptor_shuffle.cc (right):

http://codereview.chromium.org/672003/diff/11/16#newcode16
base/file_descriptor_shuffle.cc:16: (InjectiveMultimap* m, InjectionDelegate*
delegate) {
On 2010/03/09 00:32:40, willchan wrote:
> opening paren should go on prev line

Done.

http://codereview.chromium.org/672003/diff/11/16#newcode40
base/file_descriptor_shuffle.cc:40: extra_fds[next_extra_fd++] = temp_fd;
On 2010/03/09 00:32:40, willchan wrote:
> So, this function silently fails if there are too many file descriptors?  That
> sounds problematic.  Maybe we should RAW_LOG if we hit this condition?

I didn't want to allocate but, you're right, RAW_LOG will do.

http://codereview.chromium.org/672003/diff/11/16#newcode71
base/file_descriptor_shuffle.cc:71: bool PerformInjectiveMultimap(const
InjectiveMultimap &m_in,
On 2010/03/09 00:32:40, willchan wrote:
> the & should go next to the type, since you do that for *.  they should be
> consistent.

Done.

http://codereview.chromium.org/672003/diff/11/18
File base/process_util_posix.cc (right):

http://codereview.chromium.org/672003/diff/11/18#newcode272
base/process_util_posix.cc:272: // TODO(agl): Remove this function. It's
fundamentally broken for multithreaded
On 2010/03/09 00:32:40, willchan wrote:
> Can't we just delete this function now? I don't see where it's used.

Done.

http://codereview.chromium.org/672003/diff/11/18#newcode417
base/process_util_posix.cc:417: #endif
On 2010/03/09 00:32:40, willchan wrote:
> This probably works, although I'd be somewhat concerned about collateral
damage,
> removing execute perms on that page might cover some non-malloc code too.  But
> hey, this is just for debugging anyway.

Have added PROT_EXEC for the hell of it.

http://codereview.chromium.org/672003/diff/11/18#newcode436
base/process_util_posix.cc:436: setenv(it->first.c_str(), it->second.c_str(),
1);
On 2010/03/09 00:32:40, willchan wrote:
> setenv() and unsetenv() both use locks.  setenv() can call realloc() and
> malloc().

Oh tickles. You're right. See lump of new code in next patch.

http://codereview.chromium.org/672003/diff/11/18#newcode461
base/process_util_posix.cc:461: PLOG(ERROR) << "LaunchApp: execvp(" <<
argv_cstr[0] << ") failed";
On 2010/03/09 00:32:40, willchan wrote:
> Maybe this should be RAW_LOG too.

Done.

[willchan no longer on Chromium, 2010-03-09 20:18:22]

Am I looking at an old version?  I don't see how AlterEnvironment affects the
child process.

http://codereview.chromium.org/672003/diff/11001/12003
File base/dir_reader_fallback.h (right):

http://codereview.chromium.org/672003/diff/11001/12003#newcode30
base/dir_reader_fallback.h:30: #endif  // !BASE_DIR_READER_FALLBACK_H_
You shouldn't have the ! there.  Same with your other header files.

http://codereview.chromium.org/672003/diff/11001/12004
File base/dir_reader_linux.h (right):

http://codereview.chromium.org/672003/diff/11001/12004#newcode38
base/dir_reader_linux.h:38: if (close(fd_))
Do we need HANDL_EINTR here?

http://codereview.chromium.org/672003/diff/11001/12006
File base/dir_reader_posix_unittest.cc (right):

http://codereview.chromium.org/672003/diff/11001/12006#newcode22
base/dir_reader_posix_unittest.cc:22: static const unsigned kNumFiles = 100;
Minor nit: static isn't really necessary since it should probably go into an
intermediate anyway.  Whatever.

http://codereview.chromium.org/672003/diff/11001/12006#newcode77
base/dir_reader_posix_unittest.cc:77: snprintf(buf, sizeof(buf), "%d", i);
arraysize(buf)

http://codereview.chromium.org/672003/diff/11001/12010
File base/process_util_posix.cc (right):

http://codereview.chromium.org/672003/diff/11001/12010#newcode204
base/process_util_posix.cc:204: DLOG(ERROR) << "getrlimit(RLIMIT_NOFILE) failed:
" << errno;
This will allocate memory.

http://codereview.chromium.org/672003/diff/11001/12010#newcode265
base/process_util_posix.cc:265: DPCHECK(ret == 0);
This will allocate memory if ret != 0, although it'll crash too, but we might
lose the error message.

http://codereview.chromium.org/672003/diff/11001/12010#newcode348
base/process_util_posix.cc:348: static char** AlterEnvironment(const
environment_vector& changes) {
I haven't really read this yet.  I am a bit confused about its use in
LaunchApp().

http://codereview.chromium.org/672003/diff/11001/12010#newcode364
base/process_util_posix.cc:364: if (j->first.size() == keylen &&
memcmp(j->first.data(), pair, keylen) == 0) {
80 columns

http://codereview.chromium.org/672003/diff/11001/12010#newcode444
base/process_util_posix.cc:444: scoped_array<char>
environ_buffer(new_environ[0]);
I think I'm missing something.  Where does environ buffer get used?

http://codereview.chromium.org/672003/diff/11001/12010#newcode487
base/process_util_posix.cc:487: environ = new_environ.get();
Is there a purpose to this environ variable?  How does this compile?  I don't
see the definition for environ now.

http://codereview.chromium.org/672003/diff/11001/12010#newcode502
base/process_util_posix.cc:502: unsetenv("SBX_D");
This call acquires/releases libc locks.

[agl, 2010-03-09 20:32:03]

http://codereview.chromium.org/672003/diff/11001/12003
File base/dir_reader_fallback.h (right):

http://codereview.chromium.org/672003/diff/11001/12003#newcode30
base/dir_reader_fallback.h:30: #endif  // !BASE_DIR_READER_FALLBACK_H_
On 2010/03/09 20:18:22, willchan wrote:
> You shouldn't have the ! there.  Same with your other header files.

Done.

http://codereview.chromium.org/672003/diff/11001/12004
File base/dir_reader_linux.h (right):

http://codereview.chromium.org/672003/diff/11001/12004#newcode38
base/dir_reader_linux.h:38: if (close(fd_))
On 2010/03/09 20:18:22, willchan wrote:
> Do we need HANDL_EINTR here?

Done.

http://codereview.chromium.org/672003/diff/11001/12010
File base/process_util_posix.cc (right):

http://codereview.chromium.org/672003/diff/11001/12010#newcode204
base/process_util_posix.cc:204: DLOG(ERROR) << "getrlimit(RLIMIT_NOFILE) failed:
" << errno;
On 2010/03/09 20:18:22, willchan wrote:
> This will allocate memory.

This is ok / below my threshold for caring. It works 99% of the time and only
fails if we've really screwed up (basically if the pointer is invalid)

http://codereview.chromium.org/672003/diff/11001/12010#newcode265
base/process_util_posix.cc:265: DPCHECK(ret == 0);
On 2010/03/09 20:18:22, willchan wrote:
> This will allocate memory if ret != 0, although it'll crash too, but we might
> lose the error message.

Again, works 99% of the time and is only there to catch a programming error.

http://codereview.chromium.org/672003/diff/11001/12010#newcode364
base/process_util_posix.cc:364: if (j->first.size() == keylen &&
memcmp(j->first.data(), pair, keylen) == 0) {
On 2010/03/09 20:18:22, willchan wrote:
> 80 columns

Done.

http://codereview.chromium.org/672003/diff/11001/12010#newcode364
base/process_util_posix.cc:364: if (j->first.size() == keylen &&
memcmp(j->first.data(), pair, keylen) == 0) {
On 2010/03/09 20:18:22, willchan wrote:
> 80 columns

Done.

http://codereview.chromium.org/672003/diff/11001/12010#newcode444
base/process_util_posix.cc:444: scoped_array<char>
environ_buffer(new_environ[0]);
On 2010/03/09 20:18:22, willchan wrote:
> I think I'm missing something.  Where does environ buffer get used?

The global environ pointer is set in the child, below.

http://codereview.chromium.org/672003/diff/11001/12010#newcode487
base/process_util_posix.cc:487: environ = new_environ.get();
On 2010/03/09 20:18:22, willchan wrote:
> Is there a purpose to this environ variable?  How does this compile?  I don't
> see the definition for environ now.

It's defined by POSIX to the global environment.

http://codereview.chromium.org/672003/diff/11001/12010#newcode502
base/process_util_posix.cc:502: unsetenv("SBX_D");
On 2010/03/09 20:18:22, willchan wrote:
> This call acquires/releases libc locks.

Good catch, thanks.

[willchan no longer on Chromium, 2010-03-09 20:39:28]

http://codereview.chromium.org/672003/diff/11001/12010
File base/process_util_posix.cc (right):

http://codereview.chromium.org/672003/diff/11001/12010#newcode487
base/process_util_posix.cc:487: environ = new_environ.get();
On 2010/03/09 20:32:03, agl wrote:
> On 2010/03/09 20:18:22, willchan wrote:
> > Is there a purpose to this environ variable?  How does this compile?  I
don't
> > see the definition for environ now.
> 
> It's defined by POSIX to the global environment.

Ah, I missed that.  I saw that it was a parameter before, so I thought it was
leftover from that.

So, is AlterEnvironment() safe?  We're modifying a global variable without
acquiring/releasing locks, which is what setenv() and unsetenv() do.  Also,
AlterEnvironment() is called prior to fork().  Is there something that is
undoing setting the environment?  And is modifying the parent's environment
reasonable?  What if another thread is using getenv()?

I was thinking that we might want to use execve() instead and write a new
frontend for it that does the same path search that execvp() does.