Allow stdio access to be revoked from casper services

[jah]

Summary

r325740 moved dhclient to the cap_syslog service, while r341692
changed cap_syslog to inherit stdio descriptors from its parent process.
The broke the ability to capture dhclient's foreground output, as the
cap_syslog service now holds the original stdio descriptors open after
dhclient daemonizes. Since syslog(3) only requires stdio access for the
LOG_PERROR case (which dhclient doesn't use), instead revert r341692
and in the LOG_PERROR case supply the parent's stderr descriptor as
an argument within the implementation of cap_openlog().

PR: 234514

[jah]

Other than reverting r341692, this is the simplest thing that works. But I'm a complete libcasper n00b, so I'm not sure if this is the ideal approach.
Should we instead have something finer-grained (maybe the ability to revoke an individual fd from the service), and/or something like the ability to specify whether stdio/fds should be inherited through the call to cap_service_open() instead of being baked into the service declaration?

[oshogbo]

lib/libcasper/libcasper/service.c

324 ↗

(On Diff #53268)

Why we should kill whole process if we are unavailable to execute your request?

sbin/dhclient/dhclient.c

2459 ↗

(On Diff #53268)

I was thinking that syslog need to have those descriptors to log stuf.

[jah]

lib/libcasper/libcasper/service.c

324 ↗

(On Diff #53268)

Good point.

sbin/dhclient/dhclient.c

2459 ↗

(On Diff #53268)

Only if LOG_PERROR is passed to openlog(). So it was never really an issue for dhclient, which doesn't use LOG_PERROR.

[oshogbo]

sbin/dhclient/dhclient.c

2459 ↗

(On Diff #53268)

So maybe close the descriptors in the service if the LOG_PERROR is not passed?

[jah]

sbin/dhclient/dhclient.c

2459 ↗

(On Diff #53268)

That would work here, but it would break other possible use cases:

1. any process that first calls cap_openlog() without LOG_PERROR but then later calls it with LOG_PERROR on the same service

2. any process that wants log messages to go to stderr while it's in the foreground, but later daemonizes or does the equivalent of daemon[fd]. It could fix that by re-opening the log without LOG_PERROR, but that's awkward.

[markj]

Sorry for the delay in looking at this.

This approach should work, but I'm not crazy about it: every cap_syslog consumer needs to consider whether this problem affects them, and add this extra API call if so. Another quick solution would be to revert r341692 and change savecore(8) to not use cap_syslog: it is not a daemon and is quite unlikely to be affected by the problem (reconnecting to syslogd upon an error) which motivates cap_syslog in the first place. All it really needs to do is call openlog(3) before entering capability mode, which it already does.

A more robust solution might be to revert r341692 and instead change cap_openlog(LOG_PERROR) to pass the caller's STDERR_FILENO descriptor to the cap_syslog service. The service can dup() the received descriptor onto STDERR_FILENO, and then I believe cap_syslog(3) will do the right thing. Some care would be needed to ensure that the cap_syslog process doesn't allocate STDERR_FILENO for some other purpose.

[jah]

In D18989#406289, @markj wrote:

Sorry for the delay in looking at this.

This approach should work, but I'm not crazy about it: every cap_syslog consumer needs to consider whether this problem affects them, and add this extra API call if so. Another quick solution would be to revert r341692 and change savecore(8) to not use cap_syslog: it is not a daemon and is quite unlikely to be affected by the problem (reconnecting to syslogd upon an error) which motivates cap_syslog in the first place. All it really needs to do is call openlog(3) before entering capability mode, which it already does.

A more robust solution might be to revert r341692 and instead change cap_openlog(LOG_PERROR) to pass the caller's STDERR_FILENO descriptor to the cap_syslog service. The service can dup() the received descriptor onto STDERR_FILENO, and then I believe cap_syslog(3) will do the right thing. Some care would be needed to ensure that the cap_syslog process doesn't allocate STDERR_FILENO for some other purpose.

I've prototyped your second suggestion, and it seems like it works correctly: if LOG_PERROR is specified, logging to stderr works but does not hold a daemonizing parent's stderr hostage.
It's also pretty cool; I didn't realize nvlist had a clean way to wrap fds.

I do have one concern with that approach, namely that it's specific to cap_syslog. One of the reasons I started with cap_close_stdio() was that it could be applied to other services that might run into this problem in the future. Do you guys see any added value in an approach like that, or would you prefer that it always be handled in a service-specific way?

[markj]

In D18989#406689, @jah wrote:

I do have one concern with that approach, namely that it's specific to cap_syslog. One of the reasons I started with cap_close_stdio() was that it could be applied to other services that might run into this problem in the future. Do you guys see any added value in an approach like that, or would you prefer that it always be handled in a service-specific way?

I do think cap_close_stdio() would be worth having, but I don't find this particular example particularly compelling... cap_syslog only needs the standard streams if LOG_PERROR is specified, and in this case it's not. We don't have many examples of casper services which request STDIO, so I'm a bit concerned about generalizing from too few examples. That said, I don't object to this patch and am ok with it going in if @oshogbo is ok with it.

BTW, @oshogbo, why does cap_fileargs request the STDIO descriptors? I can't see how they're used.

lib/libcasper/libcasper/service.c

323 ↗

(On Diff #53268)

Just a side comment: it bugs me that libcasper's internal command namespace may conflict with commands defined by application-specific casper services.

[jah]

In D18989#406823, @markj wrote:

In D18989#406689, @jah wrote:

I do have one concern with that approach, namely that it's specific to cap_syslog. One of the reasons I started with cap_close_stdio() was that it could be applied to other services that might run into this problem in the future. Do you guys see any added value in an approach like that, or would you prefer that it always be handled in a service-specific way?

I do think cap_close_stdio() would be worth having, but I don't find this particular example particularly compelling... cap_syslog only needs the standard streams if LOG_PERROR is specified, and in this case it's not. We don't have many examples of casper services which request STDIO, so I'm a bit concerned about generalizing from too few examples. That said, I don't object to this patch and am ok with it going in if @oshogbo is ok with it.

BTW, @oshogbo, why does cap_fileargs request the STDIO descriptors? I can't see how they're used.

How about we go forward with your stderr-passing idea for now, and implement something like cap_close_stdio() if/when it ends up being needed? Since passing stderr keeps things completely within cap_syslog, it also has the near-term advantage of not needing a shlib minor version bump or a manpage edit.

[markj]

In D18989#407047, @jah wrote:

In D18989#406823, @markj wrote:

In D18989#406689, @jah wrote:

I do have one concern with that approach, namely that it's specific to cap_syslog. One of the reasons I started with cap_close_stdio() was that it could be applied to other services that might run into this problem in the future. Do you guys see any added value in an approach like that, or would you prefer that it always be handled in a service-specific way?

I do think cap_close_stdio() would be worth having, but I don't find this particular example particularly compelling... cap_syslog only needs the standard streams if LOG_PERROR is specified, and in this case it's not. We don't have many examples of casper services which request STDIO, so I'm a bit concerned about generalizing from too few examples. That said, I don't object to this patch and am ok with it going in if @oshogbo is ok with it.

BTW, @oshogbo, why does cap_fileargs request the STDIO descriptors? I can't see how they're used.

How about we go forward with your stderr-passing idea for now, and implement something like cap_close_stdio() if/when it ends up being needed? Since passing stderr keeps things completely within cap_syslog, it also has the near-term advantage of not needing a shlib minor version bump or a manpage edit.

That sounds good to me, thanks.

[jah]

In D18989#407424, @markj wrote:

In D18989#407047, @jah wrote:

In D18989#406823, @markj wrote:

In D18989#406689, @jah wrote:

I do have one concern with that approach, namely that it's specific to cap_syslog. One of the reasons I started with cap_close_stdio() was that it could be applied to other services that might run into this problem in the future. Do you guys see any added value in an approach like that, or would you prefer that it always be handled in a service-specific way?

I do think cap_close_stdio() would be worth having, but I don't find this particular example particularly compelling... cap_syslog only needs the standard streams if LOG_PERROR is specified, and in this case it's not. We don't have many examples of casper services which request STDIO, so I'm a bit concerned about generalizing from too few examples. That said, I don't object to this patch and am ok with it going in if @oshogbo is ok with it.

BTW, @oshogbo, why does cap_fileargs request the STDIO descriptors? I can't see how they're used.

How about we go forward with your stderr-passing idea for now, and implement something like cap_close_stdio() if/when it ends up being needed? Since passing stderr keeps things completely within cap_syslog, it also has the near-term advantage of not needing a shlib minor version bump or a manpage edit.

That sounds good to me, thanks.

I turns out I did some of my testing against the wrong copy of libcasper.so; passing stderr on LOG_PERROR actually *does* hold the parent's stderr stream hostage across daemon[fd]. That makes sense, given that passing fds as SCM_RIGHTS behaves like dup() in the way the underlying kernel objects are managed. I don't think it should change the plan here, but seemed worth noting that processes using LOG_PERROR could still have the same issue as dhclient today.

[jah]

Remove cap_close_stdio() and pass stderr within cap_syslog

[jah]

Simplify openlog() control flow, only dup existing stderr if LOG_PERROR
isn't already in use.

[jah]

Avoid reassigning stderr if the dup of the existing descriptor failed

[markj]

In D18989#407594, @jah wrote:

I turns out I did some of my testing against the wrong copy of libcasper.so; passing stderr on LOG_PERROR actually *does* hold the parent's stderr stream hostage across daemon[fd]. That makes sense, given that passing fds as SCM_RIGHTS behaves like dup() in the way the underlying kernel objects are managed. I don't think it should change the plan here, but seemed worth noting that processes using LOG_PERROR could still have the same issue as dhclient today.

Indeed, I would have expected the problem to persist if LOG_PERROR is used.

Having thought about this further, I now believe that the "real" solution would be to have the cap_syslog only handle maintaining the connection to syslogd. That is, in the regular case cap_syslog(3) should write directly to a socket provided by the service. Not only does this handle the LOG_PERROR problem, it avoids the substantial overhead of sending log messages through multiple sockets. This is however quite a lot of work to implement and probably requires refactoring the libc syslog() implementation. I think this is patch is a fine solution until that work is done.

lib/libcasper/services/cap_syslog/cap_syslog.c

164

I think dnvlist_take_descriptor() would be more appropriate. The _get flavour dup()s the copy in the nvlist itself and returns that, so _take avoids an extra system call.

168

I'd suggest casting the return value of dup() and dup2() to void to indicate that we're intentionally ignoring the return values.

[jah]

lib/libcasper/services/cap_syslog/cap_syslog.c

164

I thought nvlist_add_descriptor() was the only one that does any dup'ing?
The manpage at least indicates that [d]nvlist_get_descriptor() returns an immutable descriptor still owned by the nvlist (which should be fine here since we're just going to dup2() to stderr anyway). With dnvlist_take_descriptor(), we'd also need to close() the returned fd.

[markj]

lib/libcasper/services/cap_syslog/cap_syslog.c

164

Sorry, you're right.

[jah]

Designate ignored return values

[oshogbo]

lib/libcasper/services/cap_syslog/cap_syslog.c

91

This is incorrect according to style 9.

170

Dose it work?
While changing those descriptors you may change internal Casper descriptors. This is why you should have the CASPER_SERVICE_STDIO to reserve those descriptors.

[oshogbo]

@markj The cap_fileargs have the STDIO because people do very strange hacks. For example wc /dev/fd/0 or open('/dev/fd/0') or open('/dev/stdio'). We also don't close any descriptors because people do stuff like wc <(ls) which is translated to wc /dev/fd/3.

[oshogbo]

lib/libcasper/services/cap_syslog/cap_syslog.c

170

Oh sorry I forgot that Casper reserve those for you. Doing /dev/null on them. Sorry LGTM.