Page MenuHomeFreeBSD

Allow stdio access to be revoked from casper services
ClosedPublic

Authored by jah on Jan 27 2019, 6:59 AM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jah created this revision.Jan 27 2019, 6:59 AM
jah added a comment.Jan 27 2019, 7:03 AM

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?

jah edited the summary of this revision. (Show Details)Jan 27 2019, 7:04 AM
oshogbo added inline comments.Jan 27 2019, 1:04 PM
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 added inline comments.Jan 27 2019, 7:01 PM
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 added inline comments.Jan 27 2019, 7:03 PM
sbin/dhclient/dhclient.c
2459 ↗(On Diff #53268)

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

jah added inline comments.Jan 27 2019, 7:25 PM
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
  1. 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 added a comment.Jan 29 2019, 4:09 AM

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 added a comment.Jan 30 2019, 7:16 AM

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 added a comment.Jan 30 2019, 6:59 PM
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 added a comment.Jan 31 2019, 5:47 AM
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 added a comment.Feb 1 2019, 6:06 PM
In D18989#407047, @jah 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 added a comment.Feb 2 2019, 6:18 PM
In D18989#407047, @jah 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 updated this revision to Diff 53543.Feb 2 2019, 7:03 PM

Remove cap_close_stdio() and pass stderr within cap_syslog

jah edited the summary of this revision. (Show Details)Feb 2 2019, 7:08 PM
jah updated this revision to Diff 53548.Feb 2 2019, 8:19 PM

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

jah updated this revision to Diff 53554.Feb 3 2019, 7:45 AM

Avoid reassigning stderr if the dup of the existing descriptor failed

markj added a comment.Feb 3 2019, 3:08 PM
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 ↗(On Diff #53554)

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 ↗(On Diff #53554)

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

jah added inline comments.Feb 3 2019, 5:53 PM
lib/libcasper/services/cap_syslog/cap_syslog.c
164 ↗(On Diff #53554)

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 added inline comments.Feb 3 2019, 5:56 PM
lib/libcasper/services/cap_syslog/cap_syslog.c
164 ↗(On Diff #53554)

Sorry, you're right.

jah updated this revision to Diff 53563.Feb 3 2019, 6:16 PM

Designate ignored return values

markj accepted this revision.Feb 3 2019, 7:36 PM
This revision is now accepted and ready to land.Feb 3 2019, 7:36 PM
oshogbo requested changes to this revision.Feb 5 2019, 8:33 AM
oshogbo added inline comments.
lib/libcasper/services/cap_syslog/cap_syslog.c
91 ↗(On Diff #53563)

This is incorrect according to style 9.

170 ↗(On Diff #53563)

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.

This revision now requires changes to proceed.Feb 5 2019, 8:33 AM

@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 accepted this revision.Feb 5 2019, 8:39 AM
oshogbo added inline comments.
lib/libcasper/services/cap_syslog/cap_syslog.c
170 ↗(On Diff #53563)

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

This revision is now accepted and ready to land.Feb 5 2019, 8:39 AM
This revision was automatically updated to reflect the committed changes.