Page MenuHomeFreeBSD

Wakup applications with shutdown on listen sockets (stable/11 version)
ClosedPublic

Authored by jtl on Apr 9 2018, 6:44 PM.

Details

Summary

rS285910 attempted to make shutdown() be POSIX compliant by returning ENOTCONN when shutdown() is called on unconnected sockets. This change was slightly modified by rS316874, which returns ENOTCONN in the case of an unconnected datagram socket, but still runs the shutdown code for the socket. This specifically supports the case where the user-space code is using the shutdown() call to wakeup another thread blocked on the socket.

In PR 227259, a user is reporting that they have code which is using shutdown() to wakup another thread blocked on a stream listen socket. This code is failing, while it used to work on FreeBSD 10 and still works on Linux.

Without expressing an opinion on whether the underlying code *should* be written in this way, it seems reasonable to add another exception to support something users are actually doing and which used to work on FreeBSD 10 and still works on Linux. And, it seems like it should be acceptable to POSIX, as we still return ENOTCONN.

(*NOTE*: This will need to be a direct commit to stable/11. The corresponding review for HEAD is in D15019.)

Test Plan

I ran the program provided by the user. This restores the previous behavior shown in the PR (see comment 1):

root@freebsd:/tmp # ./acc_test | grep "chk OK"
0:	socket(AF_INET, block)	... lskt  accept  shutdown  chk OK, ret code: 53 - Software caused connection abort
1:	socket(AF_INET, block)	... rskt  accept  shutdown  chk OK, ret code: 53 - Software caused connection abort
4:	socket(AF_INET, block)	... lskt  poll  shutdown  chk OK
5:	socket(AF_INET, block)	... rskt  poll  shutdown  chk OK
12:	socket(AF_INET, block)	... lskt  poll  close  chk OK
13:	socket(AF_INET, block)	... rskt  poll  close  chk OK
16:	socket(AF_INET, block)	... lskt  shutdown  accept  chk OK, ret code: 53 - Software caused connection abort
20:	socket(AF_INET, block)	... lskt  shutdown  poll  chk OK
36:	socket(AF_INET, nblock)	... lskt  poll  shutdown  chk OK
37:	socket(AF_INET, nblock)	... rskt  poll  shutdown  chk OK
42:	socket(AF_INET, nblock)	... lskt  shutdown  poll  chk OK

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

jtl created this revision.Apr 9 2018, 6:44 PM
jtl added a comment.Apr 20 2018, 1:46 PM

*ping*

If we're going to get this into 11.2, it would be nice if someone reviewed this soonish.

ed accepted this revision.Apr 20 2018, 1:53 PM
This revision is now accepted and ready to land.Apr 20 2018, 1:53 PM

Presumably we need to agree on and commit D15019 first, and commit this as a MFC in spirit after the change in HEAD.

jtl added a comment.Apr 20 2018, 6:44 PM

Presumably we need to agree on and commit D15019 first, and commit this as a MFC in spirit after the change in HEAD.

Normally. But, in this case, I feel like it is a "maybe". Because the code in head is so different, I don't think there is a reason to hold this one up while we discuss the exact way to fix this in head. If we agree it *should* be fixed in head, I think it seems reasonable to fix this in 11 in time to hit 11.2. (Granted, we still have time for 11.2. But, I'd hate to see this miss 11.2 simply because we were discussing how to fix this in head.)

In D15021#319145, @jtl wrote:

Normally. But, in this case, I feel like it is a "maybe". Because the code in head is so different, I don't think there is a reason to hold this one up while we discuss the exact way to fix this in head. If we agree it *should* be fixed in head, I think it seems reasonable to fix this in 11 in time to hit 11.2. (Granted, we still have time for 11.2. But, I'd hate to see this miss 11.2 simply because we were discussing how to fix this in head.)

I suppose that's fair - it's conceivable we could decide the new behaviour is as expected in HEAD, but that the patch should be committed to stable/11 to avoid such a functional change in a stable branch.

jtl added a comment.May 10 2018, 7:04 PM

Is everyone OK with this going in? Or, to ask it differently, is anyone not OK? The 11.2 release is underway, so it would be good to get this in soon, if we agree to fix it in stable/11.

jtl added a comment.May 11 2018, 1:19 PM

Given the fact that users appear to depend on this (we have two PRs on it), this appears to have been an unintended change in stable/11, and it seems to violate POLA by making this change on a stable branch, I intend to commit this today unless someone objects.

Whether (and how) we fix this in head needs to be resolved by the time stable/12 branches.

This revision was automatically updated to reflect the committed changes.