unix: only treat an empty sun_path as a peer descriptor for connectat(2)
ClosedPublic
Actions

Authored by dteske on Wed, Aug 12, 1:16 AM.

Details

Reviewers

markj
kib
inquire_JohnEricson.me

Group Reviewers

capsicum

Commits

rG3baada83bf2d: unix: only treat an empty sun_path as a peer descriptor for connectat(2)

Summary

connect(2) passes AT_FDCWD to unp_connectat(), so the empty-path
descriptor branch added in 6563dcb6b1f5 turned any sockaddr whose
sun_path begins with a NUL byte into getsock(AT_FDCWD), failing with
EBADF where the pathname lookup historically failed with ENOENT.

Linux abstract namespace names are exactly that: the linuxulator
passes them through with the leading NUL intact, and libxcb tries the
abstract socket first, falling back to the pathname socket only on
ENOENT or ECONNREFUSED. The EBADF made every Linux X11 client fail
at startup with "Missing X server or $DISPLAY".

Restrict the descriptor interpretation to fd != AT_FDCWD, matching
the contract stated in 6563dcb6b1f5's commit message ("Accept an
empty sun_path when fd is not AT_FDCWD"): connect(2) again reaches
the pathname lookup and fails with ENOENT as it always did.

Fixes: 6563dcb6b1f5 ("unix: allow connectat(2) to name the peer socket by descriptor")

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 75528
Build 72411: arc lint + arc unit

Event Timeline

dteske created this revision.Wed, Aug 12, 1:16 AM

Herald added subscribers: olce, glebius, imp. · View Herald TranscriptWed, Aug 12, 1:16 AM

dteske requested review of this revision.Wed, Aug 12, 1:16 AM

Harbormaster completed remote builds in B75528: Diff 183904.Wed, Aug 12, 1:16 AM

This is a fix for regression introduced by D58405

Found while chasing why Linux X11 clients stopped launching on main ("Missing X server or $DISPLAY"): libxcb always tries the Linux abstract socket namespace first — a sun_path with a leading NUL and nonzero namelen — and falls back to the pathname socket only on ENOENT or ECONNREFUSED. Since 6563dcb6b1f5, that sockaddr reaches the empty-path descriptor branch through connect(2)'s AT_FDCWD and fails with EBADF, so the fallback never fires. The linuxulator passes abstract names through with the leading NUL intact, so this hits every Linux X11 client.

This is the minimal path to fixing the regression: it restores connect(2)'s historical ENOENT and leaves every connectat(2) behavior exactly as D58405 shipped it. It also matches the contract already stated there ("Accept an empty sun_path when fd is not AT_FDCWD" — connect(2) "still reject[s] an empty path").

One question I'd leave to you: connectat.2 defines "empty" strictly as namelen == offsetof(struct sockaddr_un, sun_path), so a stricter fix would gate on length rather than *buf == '\0' ... a NUL-leading, nonzero-length path through connectat(2) with a real fd would then also take the pathname lookup rather than the descriptor interpretation. Happy to extend the patch that way if that's the preferred reading.

Sorry about the breakage! I have no authority here, but would you mind writing a test for this? See where the other connectat test are.

This revision is now accepted and ready to land.Wed, Aug 12, 4:18 AM

In D58792#1349062, @inquire_JohnEricson.me wrote:

Sorry about the breakage! I have no authority here, but would you mind writing a test for this? See where the other connectat test are.

Test is simple. Install www/linux-brave from ports. Try to run it in Xorg. Get error: Missing X server or $DISPLAY (despite the fact that DISPLAY is set appropriately).

Oh I didn't mean a real-world test with actual xorg, I meant a small test of connect/connectat itself ensuring we get the correct error code.

See D58406 for the sort of test I'm talking about.

In D58792#1349067, @inquire_JohnEricson.me wrote:

Oh I didn't mean a real-world test with actual xorg, I meant a small test of connect/connectat itself ensuring we get the correct error code.

See D58406 for the sort of test I'm talking about.

Yes, let's please add a regression test.

sys/kern/uipc_usrreq.c
3123	This comment isn't useful. At most, it needs to say, "AT_FDCWD isn't a valid fd, so we don't treat it like it might be one." I'd prefer to just fix the code.

In D58792#1349067, @inquire_JohnEricson.me wrote:

Oh I didn't mean a real-world test with actual xorg, I meant a small test of connect/connectat itself ensuring we get the correct error code.

See D58406 for the sort of test I'm talking about.

Roger that. I'll create an ATF test for this later today

dteske added inline comments.Wed, Aug 12, 1:00 PM

sys/kern/uipc_usrreq.c
3123	Roger. Will reductify or remove (likely just remove which will improve the signal of your original patch without unnecessary clutter)

markj added inline comments.Wed, Aug 12, 1:06 PM

sys/kern/uipc_usrreq.c
3123	Thanks. A test case much more likely prevent the bug from reappearing.

inquire_JohnEricson.me added inline comments.Wed, Aug 12, 1:09 PM

sys/kern/uipc_usrreq.c
3123	I think the Xorg background info comment, while not so useful here, might in fact be good to put on the regression test. I agree that the needs of specific applications would clutter the main implementation too much, but I like it when regression tests have little stories next to them.