svn commit: r301572 - in head/lib/libcasper: libcasper services/cap_dns services/cap_grp services/cap_pwd services/cap_random services/cap_sysctl

[Mariusz Zaborski]

On Fri, Jun 10, 2016 at 04:47:28PM +0200, Jilles Tjoelker wrote:
> On Wed, Jun 08, 2016 at 02:03:53AM +0000, Mariusz Zaborski wrote:
> > Author: oshogbo
> > Date: Wed Jun  8 02:03:53 2016
> > New Revision: 301572
> > URL: https://svnweb.freebsd.org/changeset/base/301572
> 
> > Log:
> >   Add flags to the Casper services.
> 
> >   CASPER_SERVICE_STDIO - Casper will not close the first three descriptors (stdin,
> >   		       stdout and stderr) this can be helpful for debugging.
> >   CASPER_SERVICE_FD - Capser will not close all other descriptors, this can
> >   		    be useful for a filesystem service.
> 
> This reminds me that there are some common cases where it is wrong to
> close descriptors you don't know about. The non-POSIX command
>   diff <(cmd1) <(cmd2)
> that compares the outputs of the two commands, when executed with bash
> that was compiled with the full /dev/fd visible (as in poudriere), will
> actually run something like
>   diff /dev/fd/63 /dev/fd/62
> passing two file descriptors to pipes.
> 
> When created by a shell, these pathnames will start with /dev/fd/, but
> people could create symlinks to these special files.
This is why the CASPER_SERVICE_FD was added. :)

> > [snip]
> > +static void
> > +stdnull(void)
> > +{
> > +	int fd;
> > +
> > +	fd = open(_PATH_DEVNULL, O_RDWR);
> > +	if (fd == -1)
> > +		errx(1, "Unable to open %s", _PATH_DEVNULL);
> > +
> > +	if (setsid() == -1)
> > +		errx(1, "Unable to detach from session");
> 
> There is an implicit assumption here that stdnull() is only called from
> a process that was forked off from here, since setsid() will not and
> cannot work when called from a process that is already a session leader.
> 
> If the application is running from a shell, this setsid() will exclude
> the process from most signals, including terminal ^C/^\/^Z, kill % and
> hangups. More generally, this might make it more likely for the process
> to hang around indefinitely after the parent is gone.
I'm not sure but if the process descriptor not solve that?
If we close all process descriptor to the process it should die then, so you
need to kill just the process which is using service.

> > +
> > +	if (dup2(fd, STDIN_FILENO) == -1)
> > +		errx(1, "Unable to cover stdin");
> > +	if (dup2(fd, STDOUT_FILENO) == -1)
> > +		errx(1, "Unable to cover stdout");
> > +	if (dup2(fd, STDERR_FILENO) == -1)
> > +		errx(1, "Unable to cover stderr");
> > +
> > +	close(fd);
> 
> This was not broken by this commit, but fd should not be closed if it is
> equal to STDIN_FILENO, STDOUT_FILENO or STDERR_FILENO.
Yes you are in 100% right. Thanks, I will fix that.

Thanks,
-- 
Mariusz Zaborski
oshogbo//vx		| http://oshogbo.vexillium.org
FreeBSD commiter	| https://freebsd.org
Software developer	| http://wheelsystems.com
If it's not broken, let's fix it till it is!!1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20160610/d4dae620/attachment.sig>