[PATCH] setsid: don't fork

[PATCH] setsid: don't fork

[Phillip Susi]

2.24 added a switch to setsid to wait for the child to exit
and return its exit status.  I believe this was the wrong way
to fix the underlying bug since the behavior of the program
still differs depending on whether it is run as the group leader
or not, and requires a switch to get the correct behavior.
Instead of this --wait switch, just make sure the parent
process is the one that execs the new program rather than
the child process.

Signed-off-by: Phillip Susi <psusi@ubuntu.com>
---
 sys-utils/setsid.1 |  3 ---
 sys-utils/setsid.c | 23 +++++++++--------------
 2 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/sys-utils/setsid.1 b/sys-utils/setsid.1
index da8d648..ab1b20c 100644
--- a/sys-utils/setsid.1
+++ b/sys-utils/setsid.1
@@ -16,9 +16,6 @@ runs a program in a new session.
 \fB\-c\fP, \fB\-\-ctty\fP
 Set the controlling terminal to the current one.
 .TP
-\fB\-w\fP, \fB\-\-wait\fP
-Wait the execution of the program to end, and return the exit value of
-the child as return value of the
 .BR setsid .
 .SH "SEE ALSO"
 .BR setsid (2)
diff --git a/sys-utils/setsid.c b/sys-utils/setsid.c
index 782de82..9810138 100644
--- a/sys-utils/setsid.c
+++ b/sys-utils/setsid.c
@@ -47,12 +47,10 @@ int main(int argc, char **argv)
 {
 	int ch;
 	int ctty = 0;
-	pid_t pid;
-	int status = 0;
+	pid_t pid = 0;
 
 	static const struct option longopts[] = {
 		{"ctty", no_argument, NULL, 'c'},
-		{"wait", no_argument, NULL, 'w'},
 		{"version", no_argument, NULL, 'V'},
 		{"help", no_argument, NULL, 'h'},
 		{NULL, 0, NULL, 0}
@@ -71,9 +69,6 @@ int main(int argc, char **argv)
 		case 'c':
 			ctty=1;
 			break;
-		case 'w':
-			status = 1;
-			break;
 		case 'h':
 			usage(stdout);
 		default:
@@ -90,22 +85,22 @@ int main(int argc, char **argv)
 			err(EXIT_FAILURE, _("fork"));
 		case 0:
 			/* child */
+			sleep(5);
 			break;
 		default:
 			/* parent */
-			if (!status)
-				return EXIT_SUCCESS;
-			if (wait(&status) != pid)
-				err(EXIT_FAILURE, "wait");
-			if (WIFEXITED(status))
-				return WEXITSTATUS(status);
-			err(status, _("child %d did not exit normally"), pid);
+			if (setpgid(pid, 0) == -1)
+				err(EXIT_FAILURE, _("setpgid failed"));
+			if (setpgid(0, pid) == -1)
+				err(EXIT_FAILURE, _("setpgid failed"));
+
 		}
 	}
 	if (setsid() < 0)
 		/* cannot happen */
 		err(EXIT_FAILURE, _("setsid failed"));
-
+	if (pid)
+		kill(pid, SIGTERM);
 	if (ctty) {
 		if (ioctl(STDIN_FILENO, TIOCSCTTY, 1))
 			err(EXIT_FAILURE, _("failed to set the controlling terminal"));
-- 
1.8.3.2

Re: [PATCH] setsid: don't fork

[Karel Zak]

On Thu, Nov 14, 2013 at 02:23:13PM -0500, Phillip Susi wrote:
> 2.24 added a switch to setsid to wait for the child to exit
> and return its exit status.  I believe this was the wrong way
> to fix the underlying bug since the behavior of the program
> still differs depending on whether it is run as the group leader
> or not, and requires a switch to get the correct behavior.

Yep, it was our goal to not change the default behaviour that exists
for years.

> Instead of this --wait switch, just make sure the parent
> process is the one that execs the new program rather than
> the child process.

>  		case 0:
>  			/* child */
> +			sleep(5);

 pause() ?

>  			break;
>  		default:
>  			/* parent */
> -			if (!status)
> -				return EXIT_SUCCESS;
> -			if (wait(&status) != pid)
> -				err(EXIT_FAILURE, "wait");
> -			if (WIFEXITED(status))
> -				return WEXITSTATUS(status);
> -			err(status, _("child %d did not exit normally"), pid);
> +			if (setpgid(pid, 0) == -1)
> +				err(EXIT_FAILURE, _("setpgid failed"));
> +			if (setpgid(0, pid) == -1)
> +				err(EXIT_FAILURE, _("setpgid failed"));
> +
>  		}
>  	}
>  	if (setsid() < 0)
>  		/* cannot happen */
>  		err(EXIT_FAILURE, _("setsid failed"));
> -
> +	if (pid)
> +		kill(pid, SIGTERM);

It does not seem too elegant :-) If you really want to change the
default behaviour than it would be better to make --wait default
and exec() in child, then you don't need setpgid(), kill() and
sleep/pause() at all.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] setsid: don't fork

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/19/2013 8:24 AM, Karel Zak wrote:
> Yep, it was our goal to not change the default behaviour that
> exists for years.

It isn't really a default preference but a straight up bug since the
behavior depends on how the shell invokes it.  You only get the no
wait behavior when it is a process group leader.  When invoked not as
a group leader, then you get the wait behavior whether you asked for
it or not.  So if you really want no wait to be the default behavior
then you need to fork whether or not you're a group leader and the
policy decision chooses whether to wait for the child or not.

> It does not seem too elegant :-) If you really want to change the 
> default behaviour than it would be better to make --wait default 
> and exec() in child, then you don't need setpgid(), kill() and 
> sleep/pause() at all.

What's not elegant about it?  I think it's cleaner than keeping around
a useless intermediary process that just waits for the child and
returns its exit status.  I'm still not sure why the kernel requires
you to not be a group leader but this seems like a better workaround
than forking off a lost child.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSi31MAAoJEJrBOlT6nu75Hd4H/2zwoBVn/wFbKfbTCMKxXg/i
80+PwDicDX3hnsU4RgSfi7AvqWJkiMBabSYg6aPE8f1HpKCxRDhdOg3wGP2a3Xj6
wZJRMLmVfxVW+bC0IuWU9unnsGmLF35OGfUMM1oNc0f3pdMPtMG80nV3FTwWints
cP4YoAaHa3DC+yV0e7hxtLDqJVwx6xlRVu5B9eo/uOk/GCnJ4Hv0Zj+4Ff/FF6LV
07dbSqt6glXLc1IIuja19qxpJL2xT/tQ0+723zNK3JiDdM6zSQ+0IGTRZw+/HhD+
kjtWQWxXSCRGj/bHcotJpwJ8qmNW7/gTGEpZk7lUNAEMH2OzNN9+2uIA7PD7hxk=
=UgK/
-----END PGP SIGNATURE-----

Re: [PATCH] setsid: don't fork

[Karel Zak]

On Tue, Nov 19, 2013 at 10:01:32AM -0500, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 11/19/2013 8:24 AM, Karel Zak wrote:
> > Yep, it was our goal to not change the default behaviour that
> > exists for years.
> 
> It isn't really a default preference but a straight up bug since the
> behavior depends on how the shell invokes it.  You only get the no
> wait behavior when it is a process group leader.  When invoked not as
> a group leader, then you get the wait behavior whether you asked for
> it or not.  So if you really want no wait to be the default behavior

 but why you don't want --wait behavior? It does not change anything,
 but fix the problem with group leader. 
 
 IMHO we can make it (--wait) the default (you almost convinced me
 that the default behaviour should be fixed).

> then you need to fork whether or not you're a group leader and the
> policy decision chooses whether to wait for the child or not.
> 
> > It does not seem too elegant :-) If you really want to change the 
> > default behaviour than it would be better to make --wait default 
> > and exec() in child, then you don't need setpgid(), kill() and 
> > sleep/pause() at all.
> 
> What's not elegant about it? 

 fork for sleep() and then kill it

> I think it's cleaner than keeping around
> a useless intermediary process that just waits for the child and
> returns its exit status. 
 
 it also fixes the problem with group leader as child process is never
 group leader (you have to call setpgid() to be leader).

 I'm also not sure if remove --wait is the best way... what about
 people who already use it? (yep, it's release few weeks ago, but...)

> I'm still not sure why the kernel requires
> you to not be a group leader but this seems like a better workaround
> than forking off a lost child.

 If I good understand the man page then setsid() caller is session and
 group leader of the *new* session and group. It's probably unwanted
 to make the original group orphaned (without leader), because
 processes in orphaned group get SIGCONT from kernel...

 (BTW, SIGCONT is probably what will happen in your version after
  kill(pid) as the sleeping child is group leader, right?)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] setsid: don't fork

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/19/2013 1:05 PM, Karel Zak wrote:
> but why you don't want --wait behavior? It does not change
> anything, but fix the problem with group leader.

I *do* want that behavior, I just think it was supposed to have been
that way all along and which behavior you get should NOT depend on
whether it is a group leader or not.

> IMHO we can make it (--wait) the default (you almost convinced me 
> that the default behaviour should be fixed).

Why even have the option then?  Does anyone actually want the non wait
behavior?  If they have to add a switch to get it, they may as well
just have the shell background the job.  Simpler and cleaner than a
switch.

> fork for sleep() and then kill it

Why is that worse than fork then exec?  And I thought swapping the
group leader so the parent process can call setsid() was pretty clever ;)

> I'm also not sure if remove --wait is the best way... what about 
> people who already use it? (yep, it's release few weeks ago,
> but...)

The longer it stays in, the more people will use it.  Best to get rid
of it now if it wasn't such a great idea, otherwise it will be there
forever.

> If I good understand the man page then setsid() caller is session
> and group leader of the *new* session and group. It's probably
> unwanted to make the original group orphaned (without leader),
> because processes in orphaned group get SIGCONT from kernel...
> 
> (BTW, SIGCONT is probably what will happen in your version after 
> kill(pid) as the sleeping child is group leader, right?)

I think that happens the old way too: as soon as setsid exits after
successfully forking the child, then the group is orphaned.  In other
words, both setpgrp() and exit() remove this process from being the
group leader.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSi8DXAAoJEJrBOlT6nu75N+AH/1qyxjdJ2h+nJgN6BhIQ5WfN
SaEbSmVPGFKEFKS3ltVnH+aGfS9WJiQO5DitNewrqOlek3VSvwG3T9WZRmDPjg0M
gSaTPQMVFDIAcMcaUt71AXBEMxHLZUI5rEDLwXraaDT9ISGSGjCNRpfTrwSCwiHY
jUrulunvS0z2WkqyJbHjQZK19Z0waOEvUO7KH8UK/BWqZ2ybCHvCmC1D/uq3K09A
1dmavup13NNbxF7mG80QqdWfm9nN54N1sQ8iPUrzrQyW/6b29aSfUr74JMKbbSzt
Rodimqio/sYpf7iSbMDA4+Gmswh//IANOf9235h8s5LDb1EghSdVzl5ktu+2aLU=
=ZK8O
-----END PGP SIGNATURE-----