Page MenuHomeFreeBSD
Differential D47104

syslogd: Open forwarding socket descriptors
ClosedPublic
Actions

Authored by jfree on Oct 14 2024, 1:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 28, 12:50 AM
Unknown Object (File)
Tue, Nov 26, 5:27 PM
Unknown Object (File)
Tue, Nov 26, 2:48 AM
Unknown Object (File)
Tue, Nov 26, 2:47 AM
Unknown Object (File)
Thu, Nov 21, 12:10 AM
Unknown Object (File)
Sat, Nov 16, 9:42 AM
Unknown Object (File)
Fri, Nov 8, 4:17 AM
Unknown Object (File)
Wed, Nov 6, 11:50 PM
View All 15 Files
Subscribers
imp

Details

Reviewers
markj
Commits
rG4ecbee2760f7: syslogd: Open forwarding socket descriptors
Summary

Previously, when forwarding a message to a remote address, the target's
addrinfo was saved at config-parse-time. When message-deliver-time came,
the message's addrinfo was passed into sendmsg(2) and delivered by the
first available inet socket.

The use of sendmsg(2) is prohibited in Capsicum capability mode, so
sockets are now opened and connected to their remote peers at
config-parse-time when executing outside of the capability sandbox.

These sockets are then used with send(2), allowing forwarding to be
performed inside of the capability sandbox.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60747
Build 57631: arc lint + arc unit

Event Timeline

jfree created this revision.Oct 14 2024, 1:56 AM
Herald added a subscriber: imp. · View Herald TranscriptOct 14 2024, 1:56 AM
jfree requested review of this revision.Oct 14 2024, 1:56 AM
Harbormaster completed remote builds in B59966: Diff 144797.Oct 14 2024, 1:56 AM
jfree added a parent revision: D41477: syslogd: Watch for dead pipe processes.Oct 14 2024, 1:57 AM
markj accepted this revision.Oct 14 2024, 12:45 PM

This looks good to me, thanks. A couple of minor nits.

usr.sbin/syslogd/syslogd.c
367–372

Hopefully f->f_addr_fds == NULL implies f->f_num_addr_fds == 0, in which case there's no need to check this condition explicitly.

3043

It'd be nicer to use calloc() here IMO.

This revision is now accepted and ready to land.Oct 14 2024, 12:45 PM
jfree updated this revision to Diff 145168.EditedOct 19 2024, 6:29 PM
jfree marked 2 inline comments as done.

Address Mark's comments: use calloc() instead of malloc() for
memory allocations. This is done to avoid potential integer overflow
in total allocation size.

This revision now requires review to proceed.Oct 19 2024, 6:29 PM
Harbormaster completed remote builds in B60099: Diff 145168.Oct 19 2024, 6:29 PM
jfree added inline comments.Oct 19 2024, 6:30 PM
usr.sbin/syslogd/syslogd.c
367–372

Yes. f->f_addr_fds == NULL does imply f->f_num_addr_fds == 0

markj accepted this revision.Oct 21 2024, 2:27 PM
markj added inline comments.
usr.sbin/syslogd/syslogd_cap_config.c
218

Same here, it'd be nice to use calloc().

This revision is now accepted and ready to land.Oct 21 2024, 2:27 PM
markj mentioned this in D41402: casper/cap_net: Expose pack/unpack functions.Oct 21 2024, 2:30 PM
jfree updated this revision to Diff 146623.Sun, Nov 17, 7:57 PM
jfree marked an inline comment as done.
  • Use calloc(3) instead of malloc(3) for array allocations.
  • Add missing error checking for a memory allocation
This revision now requires review to proceed.Sun, Nov 17, 7:57 PM
Harbormaster completed remote builds in B60646: Diff 146623.Sun, Nov 17, 7:57 PM
markj added inline comments.Sun, Nov 17, 10:06 PM
usr.sbin/syslogd/syslogd.c
1751

I'd suggest writing sockfd = f->f_addr_fds[0] here.

1770

The new getsockopt() and getpeername() calls are only needed to implement dprintf(), but that's a no-op unless debug printing is enabled. IMO it would be better to avoid this extra work unless Debug is true.

2666
3048
usr.sbin/syslogd/syslogd_cap_config.c
223

Not new with this diff, so can be fixed separately, but: can we use nvlist_take_descriptor_array() instead of duping? Same for the F_FILE case.

jfree updated this revision to Diff 146630.Mon, Nov 18, 2:59 AM
jfree marked 4 inline comments as done.
  • Minor style changes
  • Only call getsockopt() and getpeername() when Debug is non-zero
Harbormaster completed remote builds in B60653: Diff 146630.Mon, Nov 18, 2:59 AM
jfree added inline comments.Mon, Nov 18, 2:59 AM
usr.sbin/syslogd/syslogd_cap_config.c
223

Not new with this diff, so can be fixed separately, but: can we use nvlist_take_descriptor_array() instead of duping? Same for the F_FILE case.

We could. I addressed this in response to your other comment about it.

I use nvlist_get_descriptor_array() here because nvlist_to_filed() and filed_to_nvlist() do not consume their arguments. i.e. the nvlist passed into nvlist_to_filed() is marked const. Using nvlist_take_descriptor_array() would modify that nvlist.

markj added inline comments.Mon, Nov 18, 2:37 PM
usr.sbin/syslogd/syslogd.c
1806

Sorry I didn't catch this before - I believe you can still use sendmsg() (so as to avoid having to loop over the iovec), so long as msghdr.msg_name = NULL. That would be cleaner IMO.

jfree added inline comments.Wed, Nov 20, 6:04 PM
usr.sbin/syslogd/syslogd.c
1806

Sorry I didn't catch this before - I believe you can still use sendmsg() (so as to avoid having to loop over the iovec), so long as msghdr.msg_name = NULL. That would be cleaner IMO.

Ah! I could've sworn I checked syscalls.master and didn't see CAPENABLED on sendmsg() before doing this. I guess I didn't. I will definitely use sendmsg() then.

jfree updated this revision to Diff 146887.Sun, Nov 24, 5:48 PM
jfree marked an inline comment as done.

Use sendmsg(2) to send iovec list.

Harbormaster completed remote builds in B60747: Diff 146887.Sun, Nov 24, 5:48 PM
markj accepted this revision.Sun, Nov 24, 6:40 PM
This revision is now accepted and ready to land.Sun, Nov 24, 6:40 PM
Closed by commit rG4ecbee2760f7: syslogd: Open forwarding socket descriptors (authored by jfree). · Explain WhyWed, Nov 27, 10:28 PM
This revision was automatically updated to reflect the committed changes.
jfree added a commit: rG4ecbee2760f7: syslogd: Open forwarding socket descriptors.

Revision Contents
Changeset List

PathSize
usr.sbin/
syslogd/
3 lines
145 lines
35 lines

Diff 146887

View Options

usr.sbin/syslogd/syslogd.h

Loading...
View Options

usr.sbin/syslogd/syslogd.c

Loading...
View Options

usr.sbin/syslogd/syslogd_cap_config.c

Loading...