Page MenuHomeFreeBSD

unix: allow listening on an unbound socket, and binding after listen
AcceptedPublic

Authored by inquire_JohnEricson.me on Wed, Aug 5, 11:36 PM.

Details

Reviewers
markj
glebius
Summary

uipc_listen() refused a socket that had not been bound, with
EDESTADDRREQ. That made sense while a pathname was the only way to
name a peer: an unbound listener could never be reached, so allowing it
would only have created sockets nothing could connect to. Now that
connectat(2) can name a peer socket by descriptor, an unbound listener
*is* reachable, and the restriction only stands in the way. It also left
stream sockets oddly stricter than datagram ones, which could already
reach an unbound peer.

Dropping the check additionally permits bind(2) after listen(2):
uipc_bindat() already allows this, as it only rejects re-binding a
socket that has a name. That ordering closes a window listeners
otherwise have to leave open. Today the socket file must exist before
the socket may listen, so a client connecting in between is refused;
binding afterwards publishes the name only once the socket is ready to
accept.

unix_seqpacket_test:listen_unbound asserted the old behaviour, and is
inverted accordingly.

Signed-off-by: John Ericson <John.Ericson@Obsidian.Systems>
Assisted-by: Claude Code (Claude Opus 4.8 and Fable 5)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
No Test Coverage
Build Status
Buildable 75405
Build 72288: arc lint + arc unit

Event Timeline

Looks like your Claude edited too much in the tests. Why do we need to adjust all the helpers? Why do we need changes in tests that do not change their behavior?

tests/sys/kern/unix_connectat.c
82–83

I guess this comment forced LLM to rewrite so much :)

@glebius Oh haha, that was not Claude's idea but mine! It didn't do it that way originally, but then I explicitly asked it to.

I was only binding in many of those tests before because I had no choice without this change (which, as a user-facing behavior change, I intentionally wanted to keep separate). Now that we have it, though, I think it is easier to just not bother binding in the test that don't need it (i.e. the ones that are just going to directly pass the socket to connectat).

I'd suggest just do the new sequence in new bind_after_listen test and leave mklistener() as is and rest of the test that use mklistener as is. Then the diff will be much smaller it it will be clearly seen how little (or how much) has changed behavior wise.

Keep tests more additive as @glebius suggested for clearer diff. Also, test another more situations that I thought of.

tests/sys/kern/unix_connectat.c
267–273

Such socket reuse is not specified by the standard and we probably don't want to encourage people to write programs relying on that. I'm not against this test, but let's put in a comment that relying on this behavior in real world scenarios is not recommended.

The change definitely lacks a new test case that would do connectat() on a listening and not bound socket.

tests/sys/kern/unix_connectat.c
267–273

Suggested text:

Note: this case is here just to document current behavior and catch if it changes in the future.  Such socket reuse is not covered by specification and is discouraged to be utilized in real world programs.
tests/sys/kern/unix_connectat.c
267–273

Definitely happy to mention that in the comment. I would even be happy to try prohibit this if you prefer. I thought of including the test less as an *endorsement* than as an *admission* that such things are now possible.

Add new test and warning about discouraged functionality as requested

Thanks! Let's wait for Mark to review.

This revision is now accepted and ready to land.Thu, Aug 6, 1:01 AM
inquire_JohnEricson.me marked 2 inline comments as done and an inline comment as not done.Thu, Aug 6, 1:08 AM

Thank you! Unless I am missing something, I cannot push to land accepted changes anyways, so yes I'll be waiting for @markj regardless :).