Switch to using vfork() instead of fork() when we can.

Switch to using vfork() instead of fork() when we can.

This means we can avoid creating new page tables, but that we share our memory
mappings / stack with the parent.  This is a bit more fragile, but should be
workable.  This saves us some work since we are just going to exec().

This also removes some sandbox unsetting code, since we shouldn't be spawning
processing under the sandbox anyway.

BUG=19863

[Mark Mentovai, 2009-08-20 20:33:26]

Gnarly.  Scary.  I love it.  LGTM.

[agl, 2009-08-20 20:35:16]

LGTM

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

http://codereview.chromium.org/173141/diff/1/2#newcode232
Line 232: bool use_vfork = (environ.size() == 0);
const

http://codereview.chromium.org/173141/diff/1/2#newcode232
Line 232: bool use_vfork = (environ.size() == 0);
Comment about why we can't modify the environment here.

[Amanda Walker, 2009-08-20 20:37:37]

Drive-by: technically, closing file descriptors between vfork() and
execvp() is illegal.  How much of a performance difference does this
actually make?

On Thu, Aug 20, 2009 at 4:26 PM, <deanm@chromium.org> wrote:
>
> Reviewers: agl, Mark Mentovai,
>
> Description:
> Switch to using vfork() instead of fork() when we can.
>
> This means we can avoid creating new page tables, but that we share our
> memory
> mappings / stack with the parent. =C2=A0This is a bit more fragile, but
> should be
> workable. =C2=A0This saves us some work since we are just going to exec()=
.
>
> This also removes some sandbox unsetting code, since we shouldn't be
> spawning
> processing under the sandbox anyway.
>
> BUG=3D19863
>
> Please review this at http://codereview.chromium.org/173141
>
> Affected files:
> =C2=A0M base/process_util_posix.cc
>
>
> Index: base/process_util_posix.cc
> diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc
> index
> ecb49371e3de32a942706f198c74760ecf8b05ad..d81776f189499cd4ddabe58779be5b8=
7f0838dce
> 100644
> --- a/base/process_util_posix.cc
> +++ b/base/process_util_posix.cc
> @@ -224,25 +224,40 @@ bool LaunchApp(const std::vector<std::string>& argv=
,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0const environment_=
vector& environ,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0const file_handle_=
mapping_vector& fds_to_remap,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0bool wait, Process=
Handle* process_handle) {
> - =C2=A0pid_t pid =3D fork();
> + =C2=A0// We call vfork() for additional performance (avoids touching th=
e page
> + =C2=A0// tables). =C2=A0This makes things a bit more dangerous since th=
e child and
> + =C2=A0// parent share the same address space and stack. =C2=A0Try to do=
 most of our
> + =C2=A0// operations before the fork, and hope that everything we do hav=
e to do
> + =C2=A0// will be ok...
> + =C2=A0bool use_vfork =3D (environ.size() =3D=3D 0);
> +
> + =C2=A0InjectiveMultimap fd_shuffle;
> + =C2=A0for (file_handle_mapping_vector::const_iterator
> + =C2=A0 =C2=A0 =C2=A0it =3D fds_to_remap.begin(); it !=3D fds_to_remap.e=
nd(); ++it) {
> + =C2=A0 =C2=A0fd_shuffle.push_back(InjectionArc(it->first, it->second, f=
alse));
> + =C2=A0}
> +
> + =C2=A0scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
> + =C2=A0for (size_t i =3D 0; i < argv.size(); i++)
> + =C2=A0 =C2=A0argv_cstr[i] =3D const_cast<char*>(argv[i].c_str());
> + =C2=A0argv_cstr[argv.size()] =3D NULL;
> +
> + =C2=A0pid_t pid =3D use_vfork ? vfork() : fork();
> =C2=A0 if (pid < 0)
> =C2=A0 =C2=A0 return false;
>
> =C2=A0 if (pid =3D=3D 0) {
> =C2=A0 =C2=A0 // Child process
> - =C2=A0 =C2=A0InjectiveMultimap fd_shuffle;
> - =C2=A0 =C2=A0for (file_handle_mapping_vector::const_iterator
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0it =3D fds_to_remap.begin(); it !=3D fds_to_=
remap.end(); ++it) {
> - =C2=A0 =C2=A0 =C2=A0fd_shuffle.push_back(InjectionArc(it->first, it->se=
cond, false));
> - =C2=A0 =C2=A0}
>
> - =C2=A0 =C2=A0for (environment_vector::const_iterator it =3D environ.beg=
in();
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 it !=3D environ.end(); ++it) {
> - =C2=A0 =C2=A0 =C2=A0if (it->first) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if (it->second) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0setenv(it->first, it->second, 1);
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsetenv(it->first);
> + =C2=A0 =C2=A0if (!use_vfork) {
> + =C2=A0 =C2=A0 =C2=A0for (environment_vector::const_iterator it =3D envi=
ron.begin();
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 it !=3D environ.end(); ++it) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (it->first) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (it->second) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0setenv(it->first, it->second, =
1);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsetenv(it->first);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> =C2=A0 =C2=A0 =C2=A0 }
> =C2=A0 =C2=A0 }
> @@ -255,17 +270,8 @@ bool LaunchApp(const std::vector<std::string>& argv,
> =C2=A0 =C2=A0 if (!ShuffleFileDescriptors(fd_shuffle))
> =C2=A0 =C2=A0 =C2=A0 _exit(127);
>
> - =C2=A0 =C2=A0// If we are using the SUID sandbox, it sets a magic envir=
onment
> variable
> - =C2=A0 =C2=A0// ("SBX_D"), so we remove that variable from the environm=
ent here on
> the
> - =C2=A0 =C2=A0// off chance that it's already set.
> - =C2=A0 =C2=A0unsetenv("SBX_D");
> -
> =C2=A0 =C2=A0 CloseSuperfluousFds(fd_shuffle);
>
> - =C2=A0 =C2=A0scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
> - =C2=A0 =C2=A0for (size_t i =3D 0; i < argv.size(); i++)
> - =C2=A0 =C2=A0 =C2=A0argv_cstr[i] =3D const_cast<char*>(argv[i].c_str())=
;
> - =C2=A0 =C2=A0argv_cstr[argv.size()] =3D NULL;
> =C2=A0 =C2=A0 execvp(argv_cstr[0], argv_cstr.get());
> =C2=A0 =C2=A0 LOG(ERROR) << "LaunchApp: exec failed!, argv_cstr[0] " << a=
rgv_cstr[0]
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 << ", errno " << errno;
>
>
>



--=20
"Portability is generally the result of advance planning rather than trench
warfare involving #ifdef" -- Henry Spencer (1992)