Bug in interaction with pid namespaces

Bug in interaction with pid namespaces

[Chirantan Ekbote]

Hi,

I think I've found an issue with the way zsh interacts with pid namespaces [1].
Specifically the problem is that when zsh is launched immediately following the
creation of a new pid namespace it doesn't take ownership of the process group,
causing things like SIGINT to be sent to the process that spawned zsh rather
than zsh itself.  This can be minimally reproduced by running:

    unshare -p -f --mount-proc zsh -l

and then running

    ps o pid,pgid,comm

inside the newly created shell.  The expected result is that the pgid for zsh
should be the same as its pid, indicating that zsh has become the leader of a
new process group.  Instead I see that zsh has pgid 0 and a different pid
(usually 1).  If you then press ctrl-c, zsh will exit rather than just show the
prompt again.  I think this can be fixed with the following patch:

diff --git a/Src/jobs.c b/Src/jobs.c
index a668b07..8c4254a 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -2734,7 +2734,7 @@ acquire_pgrp(void)
     long ttpgrp;
     sigset_t blockset, oldset;

-    if ((mypgrp = GETPGRP()) > 0) {
+    if ((mypgrp = GETPGRP()) >= 0) {
        long lastpgrp = mypgrp;
        sigemptyset(&blockset);
        sigaddset(&blockset, SIGTTIN);

but this brings up another problem.  When zsh exits it tries to restore the
original process group using setpgrp(0, origpgrp).  This is an issue when
origpgrp is 0 because setpgrp(0, 0) actually means "set the process group of the
calling process to be the same as its pid", which is not the intention of the
call at all.

What's the proper way to fix this?

Thanks,
Chirantan



[1] http://lwn.net/Articles/531419/

Re: Bug in interaction with pid namespaces

[Bart Schaefer]

On Dec 16,  3:07pm, Chirantan Ekbote wrote:
}
} I think I've found an issue with the way zsh interacts with pid
} namespaces [1]. Specifically the problem is that when zsh is launched
} immediately following the creation of a new pid namespace it doesn't
} take ownership of the process group, causing things like SIGINT to be
} sent to the process that spawned zsh rather than zsh itself.

Considering that pid namespaces are a very recent invention compared to
zsh, this is hardly surprising.  It was never meant to be used as a
replacement for init.

} The expected result is that the pgid for zsh should be the same as its
} pid, indicating that zsh has become the leader of a new process group.
} Instead I see that zsh has pgid 0 and a different pid (usually 1).

How often is it not 1 ?  The first process in the namespace should always
get pid 1, if I'm reading correctly.  Nevertheless ...

} I think this can be fixed with the following patch:
} 
} -    if ((mypgrp = GETPGRP()) > 0) {
} +    if ((mypgrp = GETPGRP()) >= 0) {

I don't see any reason not to make that change ...

} but this brings up another problem. When zsh exits it tries to restore
} the original process group using setpgrp(0, origpgrp). This is an
} issue when origpgrp is 0 because setpgrp(0, 0) actually means "set the
} process group of the calling process to be the same as its pid"

I'm not familiar enough with pid namespaces to answer authoritatively,
but my understanding is that when the first process in the namespace
exits, the entire namespace ceases to exist -- sy why does this matter
at all?  The pgrp should already be the same as the pid at that point,
and the whole point of the namespace is that processes inside the
namespace can't directly reference a pid or pgrp outside the namespace,
so setpgrp(0,0) should be unnecessary but a no-op.

What's the actual problem that's left over after zsh has exited?

Re: Bug in interaction with pid namespaces

[Bart Schaefer]

On Dec 16,  3:07pm, Chirantan Ekbote wrote:
}
} I think I've found an issue with the way zsh interacts with pid
} namespaces [1]. Specifically the problem is that when zsh is launched
} immediately following the creation of a new pid namespace it doesn't
} take ownership of the process group

There seem to be a few other potential problems with this using zsh as
the init process in a namespace; for example:

- the suspend command should never stop the shell (even with -f)

- the shell should refuse normal exit when NOHUP is set and there are
  any jobs still running (because as I understand it, it's not possible
  for any processes in the namespace to survive after the init exits)

How far down this path do we want to go?

Re: Bug in interaction with pid namespaces

[Chirantan Ekbote]

On Tue, Dec 16, 2014 at 10:27 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Dec 16,  3:07pm, Chirantan Ekbote wrote:
> }
> } I think I've found an issue with the way zsh interacts with pid
> } namespaces [1]. Specifically the problem is that when zsh is launched
> } immediately following the creation of a new pid namespace it doesn't
> } take ownership of the process group, causing things like SIGINT to be
> } sent to the process that spawned zsh rather than zsh itself.
>
> Considering that pid namespaces are a very recent invention compared to
> zsh, this is hardly surprising.  It was never meant to be used as a
> replacement for init.
>

We're not trying to use zsh as a replacement for init.  I first
noticed this issue because the chrome os build system recently
switched to using pid namespaces in the build chroot.  I only
mentioned the unshare method because I figured you wouldn't want to
set up the entire chrome os build system just to reproduce this
problem ;-)  So the fact that it ends up as pid 1 is a bit of a red
herring here.  In the actual workflow where this is an issue for me,
the pid is somewhere in the 240 to 270 range and zsh is only meant to
behave like a regular shell.

> } The expected result is that the pgid for zsh should be the same as its
> } pid, indicating that zsh has become the leader of a new process group.
> } Instead I see that zsh has pgid 0 and a different pid (usually 1).
>
> How often is it not 1 ?  The first process in the namespace should always
> get pid 1, if I'm reading correctly.  Nevertheless ...
>
> } I think this can be fixed with the following patch:
> }
> } -    if ((mypgrp = GETPGRP()) > 0) {
> } +    if ((mypgrp = GETPGRP()) >= 0) {
>
> I don't see any reason not to make that change ...
>

Excellent.  Should I re-send it as a proper patch?

> } but this brings up another problem. When zsh exits it tries to restore
> } the original process group using setpgrp(0, origpgrp). This is an
> } issue when origpgrp is 0 because setpgrp(0, 0) actually means "set the
> } process group of the calling process to be the same as its pid"
>
> I'm not familiar enough with pid namespaces to answer authoritatively,
> but my understanding is that when the first process in the namespace
> exits, the entire namespace ceases to exist -- sy why does this matter
> at all?  The pgrp should already be the same as the pid at that point,
> and the whole point of the namespace is that processes inside the
> namespace can't directly reference a pid or pgrp outside the namespace,
> so setpgrp(0,0) should be unnecessary but a no-op.
>
> What's the actual problem that's left over after zsh has exited?

So the problem is that on exit zsh gives a warning:

    zsh: can't set tty pgrp: no such process

which I thought was due to the call to setpgrp() but is actually in
the call to attachtty().  I'm having a trouble parsing that function
to figure out why that warning is happening though.

Re: Bug in interaction with pid namespaces

[Bart Schaefer]

On Dec 16, 11:50pm, Chirantan Ekbote wrote:
}
} We're not trying to use zsh as a replacement for init. I first noticed
} this issue because the chrome os build system recently switched to
} using pid namespaces in the build chroot. [...] In the actual workflow
} where this is an issue for me, the pid is somewhere in the 240 to 270
} range and zsh is only meant to behave like a regular shell.

OK, so the issue is that zsh is started as a (possibly indirect) child
of whatever was given "leadership" of the namespace, but at the time
zsh starts GETPGRP() == 0.

That sounds like a bug in one of the ancestor processes, then, because
shouldn't whatever *that* is, have become a group leader, or assigned
the process group to the fork() pid before execve() of zsh?

} > } -    if ((mypgrp = GETPGRP()) > 0) {
} > } +    if ((mypgrp = GETPGRP()) >= 0) {
} >
} > I don't see any reason not to make that change ...
} >
} 
} Excellent.  Should I re-send it as a proper patch?

No need, I already applied it and pushed it to the repository.

} So the problem is that on exit zsh gives a warning:
} 
}     zsh: can't set tty pgrp: no such process
} 
} which I thought was due to the call to setpgrp() but is actually in
} the call to attachtty().

Aha.  That means that one of

	tcsetpgrp(SHTTY, pgrp)
or
	arg = &pgrp
	ioctl(SHTTY, TIOCSPGRP, &arg)

is returning -1 and errno == ESRCH.  Which makes sense if pgrp == 0.

But that also means that zsh is being invoked as an interactive shell,
because attachtty() is a no-op otherwise.  I can't imagine why the
chrome os build would need to run an interactive shell.

However, I suggest the following.  The mypgrp = origpgrp assignment is
questionable because the pgrp won't actually have been changed, but it
may help elsewhere in tests that mypgrp != GETPGRP().  Shell exit is
not the only place where release_pgrp() is called.

diff --git a/Src/jobs.c b/Src/jobs.c
index 8c4254a..c6e1bce 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -2779,8 +2779,11 @@ void
 release_pgrp(void)
 {
     if (origpgrp != mypgrp) {
-	attachtty(origpgrp);
-	setpgrp(0, origpgrp);
+	/* in linux pid namespaces, origpgrp may never have been set */
+	if (origpgrp) {
+	    attachtty(origpgrp);
+	    setpgrp(0, origpgrp);
+	}
 	mypgrp = origpgrp;
     }
 }

Re: Bug in interaction with pid namespaces

[Chirantan Ekbote]

On Wed, Dec 17, 2014 at 8:54 AM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Dec 16, 11:50pm, Chirantan Ekbote wrote:
> }
> } We're not trying to use zsh as a replacement for init. I first noticed
> } this issue because the chrome os build system recently switched to
> } using pid namespaces in the build chroot. [...] In the actual workflow
> } where this is an issue for me, the pid is somewhere in the 240 to 270
> } range and zsh is only meant to behave like a regular shell.
>
> OK, so the issue is that zsh is started as a (possibly indirect) child
> of whatever was given "leadership" of the namespace, but at the time
> zsh starts GETPGRP() == 0.
>
> That sounds like a bug in one of the ancestor processes, then, because
> shouldn't whatever *that* is, have become a group leader, or assigned
> the process group to the fork() pid before execve() of zsh?
>

It quite possibly is a bug in the ancestor process.  I've also brought
this up with our build team and I think they may make the change you
suggest.

>
> } So the problem is that on exit zsh gives a warning:
> }
> }     zsh: can't set tty pgrp: no such process
> }
> } which I thought was due to the call to setpgrp() but is actually in
> } the call to attachtty().
>
> Aha.  That means that one of
>
>         tcsetpgrp(SHTTY, pgrp)
> or
>         arg = &pgrp
>         ioctl(SHTTY, TIOCSPGRP, &arg)
>
> is returning -1 and errno == ESRCH.  Which makes sense if pgrp == 0.
>
> But that also means that zsh is being invoked as an interactive shell,
> because attachtty() is a no-op otherwise.  I can't imagine why the
> chrome os build would need to run an interactive shell.
>

I won't bore you with the details of the build process because they
would bore you.  The short version is to start working on chrome os we
run a command (cros_sdk) that downloads updates, sets up the
environment, mounts various directories inside a chroot, and at the
end runs

    exec chroot "${chroot_dir}" sudo -u ${username} -i

It is at this point that zsh is launched (since it is my login shell)
and from there, I can run commands to build different components or an
entire image of chrome os.

> However, I suggest the following.  The mypgrp = origpgrp assignment is
> questionable because the pgrp won't actually have been changed, but it
> may help elsewhere in tests that mypgrp != GETPGRP().  Shell exit is
> not the only place where release_pgrp() is called.
>
> diff --git a/Src/jobs.c b/Src/jobs.c
> index 8c4254a..c6e1bce 100644
> --- a/Src/jobs.c
> +++ b/Src/jobs.c
> @@ -2779,8 +2779,11 @@ void
>  release_pgrp(void)
>  {
>      if (origpgrp != mypgrp) {
> -       attachtty(origpgrp);
> -       setpgrp(0, origpgrp);
> +       /* in linux pid namespaces, origpgrp may never have been set */
> +       if (origpgrp) {
> +           attachtty(origpgrp);
> +           setpgrp(0, origpgrp);
> +       }
>         mypgrp = origpgrp;
>      }
>  }

This works for me.  Thanks very much for your help :-)