When a SIGHUP signal is sent to syslogd, the configuration is reparsed, leading to new resource acquisition. If syslogd is running in capability mode and a SIGHUP is received, new resources cannot be acquired. To mitigate this issue, libcasper is used to parse the configuration. The libcasper process runs outside of capability mode and is capable of parsing syslogd's configuration and obtaining new resources. These resources are then sent to the syslogd process via nvlist.
Details
Details
- Reviewers
markj - Commits
- rG2567168dc498: syslogd: Refresh configuration using libcasper
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Use logerror() to log errors instead of exiting. This makes debugging significantly easier when something goes wrong during configuration parsing.
| usr.sbin/syslogd/syslogd_cap_config.c | ||
|---|---|---|
| 74 | What's the point of having filed_count as an nvlist element? Doesn't nvlist_get_nvlist_array() return the size of the array in filed_count? | |
| 130 | Per my comment in D41463, we should continue factoring this code out to make it less confusing. In an ideal design, IMO, readconfigfile() would return the nvlist instead of mucking with global variables. That doesn't need to happen in this revision, it's just a comment about the direction I think we should go. | |
Comment Actions
Don't manually add the filed count to the nvlist. The filed count is fetched when getting the nvlist filed array.
Comment Actions
Fix minor mistyping in populate_config(). filed_count should be declared as size_t, not uint64_t.
| usr.sbin/syslogd/syslogd_cap_config.c | ||
|---|---|---|
| 82 | Does the casper service do any validation of the config file path? Doesn't this interface allow syslogd to parse any file as a syslogd configuration file and derive capabilities from it? Most likely we should handle this by making the configuration service take a limit, set by cap_limit_set(). During initialization, syslogd would limit the service to only /etc/syslog.conf (or whatever is specified by -f). An attempt to parse any other file should be rejected by the casper service. | |
| usr.sbin/syslogd/syslogd_cap_config.c | ||
|---|---|---|
| 82 |
I made ConfFile a global variable and did a strcmp() to verify that the passed-in path matched libcasper's original copy. | |
Comment Actions
This broadly looks ok to me. I'm a bit worried about resource leaks in the routines which (de)serialize nvlists. It's quite difficult to account for everything and be confident that we're not leaking descriptors or memory somewhere.
| usr.sbin/syslogd/syslogd_cap_config.c | ||
|---|---|---|
| 56 | In general, I'd like for us to move towards copying structures using nvlist_add_binary(), to try and simplify this tedious serialization code. Of course, special care is needed for pointers and file descriptors, but most of these fields don't belong in those two categories. Once the patch set has landed, we should try and simplify this code further. | |
| 239 | Can we not use nvlist_take_descriptor() here? | |
| usr.sbin/syslogd/syslogd_cap_config.c | ||
|---|---|---|
| 146 | This is maybe a painful request, but I wonder if it's possible to avoid having to serialize addrinfo here at all. syslogd really just wants the socket addr and address family. If we go down this path, it can be done in a separate patch. | |
| usr.sbin/syslogd/syslogd_cap_config.c | ||
|---|---|---|
| 146 | As you pointed out in another comment, nvlist_add_binary() would probably be less tedious since a lot of struct filed members are inline (not pointers or descriptors). I think I originally went with this approach because I didn't like the idea of storing a binary blob with invalid descriptor numbers and pointers; I feel like doing that generally leads to confusion for future contributors that are not completely familiar with which specific members should be xfered outside of the binary blob. | |
| 239 | This function does not modify the state of nvl_filed. Using take_descriptor() would remove (and free) the descriptor from nvl_filed. We just get it and dup(2) it instead | |