Revert setuid sandbox as a "init process" changes

Revert setuid sandbox as a "init process" changes

- 4d93b1f629fda9bd2782d4f7bf8aa12f479cc8f3 (126188)
- 28af78c4ea3cdeaa959ded5c3bf29d707012774a (119746)

This introduced dreaded complexity in something that should be kept simple.

We can fix the zombie issue in Zygote or at the very least in unprivileged
code. It was not fully fixed by those patches anyway (109944).

BUG=125821, 109944
TEST="Run chrome with the setuid sandbox enabled"
NOTRY=true

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138482

[jln (very slow on Chromium), 2012-05-18 19:04:20]

Markus, I'm reverting those changes to the setuid sandbox.

This makes the sandbox way too complex, we need to address this issue in Zygote
(or at the very least in unprivileged code in the sandbox).

Since grand childs dying without being waited for only happens with the
(traditional) seccomp sandbox so far, and in any case is not a major issue, I'm
not trying to make this revert and the upcoming fix atomic.

[Markus (顧孟勤), 2012-05-18 19:11:33]

Please don't revert without actually fixing it. This was an externally
reported bug that affected actual users.

I don't have a problem with us trying to fix it some other way. But we
should never re-introduce a bug that is then visible to users.


Markus

On Fri, May 18, 2012 at 12:04 PM, <jln@chromium.org> wrote:

> Reviewers: Markus (顧孟勤),
>
> Message:
> Markus, I'm reverting those changes to the setuid sandbox.
>
> This makes the sandbox way too complex, we need to address this issue in
> Zygote
> (or at the very least in unprivileged code in the sandbox).
>
> Since grand childs dying without being waited for only happens with the
> (traditional) seccomp sandbox so far, and in any case is not a major
> issue, I'm
> not trying to make this revert and the upcoming fix atomic.
>
> Description:
> Revert setuid sandbox as a "init process" changes
>
> - 4d93b1f629fda9bd2782d4f7bf8aa1**2f479cc8f3 (126188)
> - 28af78c4ea3cdeaa959ded5c3bf29d**707012774a (119746)
>
> This introduced dreaded complexity in something that should be kept simple.
>
> We can fix the zombie issue in Zygote or at the very least in unprivileged
> code. It was not fully fixed by those patches anyway (109944).
>
> BUG=125821,109944
> TEST=
>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10389214/<https://chromiumcodereview...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  D sandbox/linux/suid/init_**process.h
>  D sandbox/linux/suid/init_**process.c
>  M sandbox/linux/suid/sandbox.c
>  M sandbox/sandbox.gyp
>
>
>

[jln (very slow on Chromium), 2012-05-18 19:15:32]

But from what I understand, that bug (109944) is still not fixed anyway.

I would rather roll back this incorrect fix that modified the setuid sandbox in
a scary way and start fresh to fix 109944.

In other words, we have two issues, 125821 and 109944. This fixes 125821 without
affecting 109944.

On 2012/05/18 19:11:33, Markus (顧孟勤) wrote:
> Please don't revert without actually fixing it. This was an externally
> reported bug that affected actual users.
> 
> I don't have a problem with us trying to fix it some other way. But we
> should never re-introduce a bug that is then visible to users.
> 
> 
> Markus
> 
> On Fri, May 18, 2012 at 12:04 PM, <mailto:jln@chromium.org> wrote:
> 
> > Reviewers: Markus (顧孟勤),
> >
> > Message:
> > Markus, I'm reverting those changes to the setuid sandbox.
> >
> > This makes the sandbox way too complex, we need to address this issue in
> > Zygote
> > (or at the very least in unprivileged code in the sandbox).
> >
> > Since grand childs dying without being waited for only happens with the
> > (traditional) seccomp sandbox so far, and in any case is not a major
> > issue, I'm
> > not trying to make this revert and the upcoming fix atomic.
> >
> > Description:
> > Revert setuid sandbox as a "init process" changes
> >
> > - 4d93b1f629fda9bd2782d4f7bf8aa1**2f479cc8f3 (126188)
> > - 28af78c4ea3cdeaa959ded5c3bf29d**707012774a (119746)
> >
> > This introduced dreaded complexity in something that should be kept simple.
> >
> > We can fix the zombie issue in Zygote or at the very least in unprivileged
> > code. It was not fully fixed by those patches anyway (109944).
> >
> > BUG=125821,109944
> > TEST=
> >
> >
> > Please review this at
>
https://chromiumcodereview.**appspot.com/10389214/%3Chttps://chromiumcoderevi...>
> >
> > SVN Base:
>
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
> >
> > Affected files:
> >  D sandbox/linux/suid/init_**process.h
> >  D sandbox/linux/suid/init_**process.c
> >  M sandbox/linux/suid/sandbox.c
> >  M sandbox/sandbox.gyp
> >
> >
> >

[Markus (顧孟勤), 2012-05-18 19:27:01]

125821 is a "theoretical bug" at this point. It doesn't affect any users,
and doesn't actually do anything wrong other than involving extra code
complexity that could potential turn into a problem in the future. So,
while I don't disagree with fixing it, it also doesn't result in a
user-visible change; so, it shouldn't be such a high priority that it
justifies introducing new bugs that actually do affect our users.

On the other hand, 109944 is a single report of an issue that we have not
been able to reproduce and that the user has not been able to provide any
additional information about. For all we know, this could be a user error
(e.g. running a different version of Chrome than what they think they are
running) or it could be completely unrelated.

If I recall correctly, the user who reported 109944 said that he saw a
single zombie process. This very different from the bug that we originally
fixed, and that you are about to reintroduce. That bug resulted in users
seeing lots (sometimes tens of thousands) of zombies. A single zombie is
annoying, but probably doesn't cause much of a performance issue. Tens of
thousands of zombies will result in serious performance degradation. We
really don't want to go there, unless we have no other alternative.

And the alternative is that we first come up with a code change that we
like, and only then do we revert the change that we are not so happy about.
This makes sure there is no window of time where we are actively hurting
our users.


Markus

On Fri, May 18, 2012 at 12:15 PM, <jln@chromium.org> wrote:

> But from what I understand, that bug (109944) is still not fixed anyway.
>
> I would rather roll back this incorrect fix that modified the setuid
> sandbox in
> a scary way and start fresh to fix 109944.
>
> In other words, we have two issues, 125821 and 109944. This fixes 125821
> without
> affecting 109944.
>
>
> On 2012/05/18 19:11:33, Markus (顧孟勤) wrote:
>
>> Please don't revert without actually fixing it. This was an externally
>> reported bug that affected actual users.
>>
>
>  I don't have a problem with us trying to fix it some other way. But we
>> should never re-introduce a bug that is then visible to users.
>>
>
>
>  Markus
>>
>
>  On Fri, May 18, 2012 at 12:04 PM, <mailto:jln@chromium.org> wrote:
>>
>
>  > Reviewers: Markus (顧孟勤),
>> >
>> > Message:
>> > Markus, I'm reverting those changes to the setuid sandbox.
>> >
>> > This makes the sandbox way too complex, we need to address this issue in
>> > Zygote
>> > (or at the very least in unprivileged code in the sandbox).
>> >
>> > Since grand childs dying without being waited for only happens with the
>> > (traditional) seccomp sandbox so far, and in any case is not a major
>> > issue, I'm
>> > not trying to make this revert and the upcoming fix atomic.
>> >
>> > Description:
>> > Revert setuid sandbox as a "init process" changes
>> >
>> > - 4d93b1f629fda9bd2782d4f7bf8aa1****2f479cc8f3 (126188)
>> > - 28af78c4ea3cdeaa959ded5c3bf29d****707012774a (119746)
>>
>> >
>> > This introduced dreaded complexity in something that should be kept
>> simple.
>> >
>> > We can fix the zombie issue in Zygote or at the very least in
>> unprivileged
>> > code. It was not fully fixed by those patches anyway (109944).
>> >
>> > BUG=125821,109944
>> > TEST=
>> >
>> >
>> > Please review this at
>>
>
> https://chromiumcodereview.**a**ppspot.com/10389214/%3Chttps:/**
>
/chromiumcodereview.appspot.**com/10389214/<http://appspot.com/10389214/%3Chttps://chromiumcodereview.appspot.com/10389214/>
> >
>
>> >
>> > SVN Base:
>>
>
>
svn://svn.chromium.org/chrome/****trunk/src<http://svn.chromium.org/chrome/**trunk/src>
>
<http://svn.**chromium.org/chrome/trunk/src<http://svn.chromium.org/chrome/tru...
> >
>
>> >
>> > Affected files:
>> >  D sandbox/linux/suid/init_****process.h
>> >  D sandbox/linux/suid/init_****process.c
>> >  M sandbox/linux/suid/sandbox.c
>> >  M sandbox/sandbox.gyp
>> >
>> >
>> >
>>
>
>
>
https://chromiumcodereview.**appspot.com/10389214/<https://chromiumcodereview...
>

[jln (very slow on Chromium), 2012-05-23 00:09:41]

Markus, now that https://chromiumcodereview.appspot.com/10417019 is ongoing,
would you mind taking another look ?

I would make sure I commit 10417019 before this one.

[Markus (顧孟勤), 2012-05-23 04:55:41]

This looks pretty sane. I assume you did this automatically, so I shouldn't be
overly worried about subtle typos? And I also assume you actually tested this?

If so, LGTM

[jln (very slow on Chromium), 2012-05-23 15:47:04]

On 2012/05/23 04:55:41, Markus (顧孟勤) wrote:
> This looks pretty sane. I assume you did this automatically, so I shouldn't be
> overly worried about subtle typos? And I also assume you actually tested this?
> 
> If so, LGTM

It's tested, not extensively, but enough I hope.

[commit-bot: I haz the power, 2012-05-23 15:48:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/10389214/1

[commit-bot: I haz the power, 2012-05-23 15:50:49]

Change committed as 138482