Close announced sockets on hangup

[Ticed]

What this changes

so_hangup skipped close() whenever shutdown() failed:

r = shutdown(fd, 2);
if(r >= 0)
	r = close(fd);

An announced socket has no peer, so shutdown() returns ENOTCONN on it and the
descriptor is never closed. The write to the control file reports the error, and
the descriptor leaks — one per hangup, for the life of the emulator.

shutdown is now best-effort and close always runs. Same change in
emu/port/ipif-posix.c, emu/port/ipif6-posix.c and emu/FreeBSD/ipif.c,
which carry the same function. emu/Nt/ipif.c and emu/Nt/ipif6.c go straight
to closesocket() and never had this.

Test

tests/tcp_test.b gains AnnounceHangupReleasesPort: announce
tcp!127.0.0.1!18798, write hangup to the control file, announce the same
address again. It skips cleanly when there is no IP stack.

The test only discriminates on BSD-family hosts, and says so in a comment.
POSIX makes shutdown() on a listening socket ENOTCONN and macOS and FreeBSD
return it; Linux permits it as the idiom for unblocking a thread parked in
accept(), so the close happens there either way and the test passes with or
without the change. Most of the CI runners are Linux, so only the macOS runner
exercises this.

Against unfixed code, on macOS ARM64:

=== RUN   AnnounceHangupReleasesPort
--- FAIL: AnnounceHangupReleasesPort (0.00s)
    /tests/tcp_test.b:/testAnnounceHangupReleasesPort/
    announced socket hangup failed: Socket is not connected

With the fix:

--- PASS: AnnounceHangupReleasesPort (0.00s)
4 passed, 2 skipped

The two skips are the outbound-network tests, skipped offline.

[pdfinn]

Heads-up on merging this one, and it is my doing rather than yours.

This branch predates #560, which stopped tracking compiled bytecode. It has a
.dis file in its diff, so merging hits:

CONFLICT (modify/delete): <path>.dis deleted in HEAD and modified in <branch>.
Version <branch> of <path>.dis left in tree.

Git leaves the file sitting in the working tree, so resolving with git add .
would quietly re-commit bytecode that master deliberately removed. The
resolution is to delete it:

git rm <path>.dis

Or just rebase onto current master, where the file no longer exists and the
conflict does not arise.

Nothing else in the PR is affected — dis/ is a build product now, rebuilt
with:

for d in appl appl/mpeg appl/veltro tests; do (cd $d && mk install); done

hooks/post-merge does that automatically after a pull if you have run
./hooks/install.sh.

Sorry for the friction — five open PRs are in this position because of the
timing.

[pdfinn]

The fix is right and the coverage is complete. Skipping close() because
shutdown() failed is plainly wrong whatever the platform, and making r the
result of close() is also better — the caller now learns whether the teardown
actually succeeded rather than whether the shutdown did.

I checked for other copies: emu/Nt/ipif.c and emu/Nt/ipif6.c both go
straight to closesocket() and never call shutdown, so they never had this.
The three you patched are the whole set.

The bug is BSD-only, and the test cannot fail on Linux

Worth knowing, because it changes what CI tells you.

On Linux, shutdown() on a listening socket succeeds. I checked rather than
assumed:

shutdown(listening_fd, SHUT_RDWR) = 0
=> old code would STILL call close()

Linux permits it deliberately — it is the idiom for unblocking a thread parked
in accept(). POSIX says ENOTCONN, and macOS and FreeBSD do return that,
which is where your leak actually happens.

So on Linux there was never a leak, and AnnounceHangupReleasesPort passes
with or without the C change. I built both and ran it:

unpatched emu:  --- PASS: AnnounceHangupReleasesPort (0.00s)
patched emu:    --- PASS: AnnounceHangupReleasesPort (0.00s)

Three of the CI jobs are Linux, so this test will sit green there forever
regardless of the code. Only the macOS runner exercises it.

The assertion that bites is not the one you wrote

On macOS with the old code, so_hangup returns -1, and devip.c:905 is

if(c->sfd >= 0 && so_hangup(c->sfd, 1) < 0)
    error(...);

so the write of "hangup" itself fails, and your t.fatal("announced socket hangup failed") fires. That is the real detector.

The re-announce assertion underneath it is masked: ipif-posix.c:251 sets
SO_REUSEADDR on announce, so rebinding the same address succeeds even with a
leaked fd still bound. That means the PR description's symptom —

Hanging up an announce and re-announcing the same address fails with "Socket
is not connected"

is not quite the mechanism. The re-announce is fine; it is the hangup that
errors, and the lasting harm is the leaked descriptor rather than the port
being unusable.

None of that is a reason to hold the PR. But a comment in the test saying it
only discriminates on BSD-family hosts would stop someone later "simplifying"
it after watching it pass on Linux — and if you want it to detect the leak
everywhere, counting descriptors around a loop of announce/hangup would do it
platform-independently.

Happy for this to go in once the dis/tests/tcp_test.dis conflict is resolved
by deletion rather than by keeping the branch's copy (see my other comment).

[Ticed]

Rebased onto current master, dis/tests/tcp_test.dis resolved by deletion.

Added the comment you asked for above testAnnounceHangupReleasesPort, saying
the test only discriminates on BSD-family hosts and why, so nobody simplifies it
after watching it pass on Linux.

You were right about the mechanism and the body was wrong. It claimed the
re-announce fails; it does not. The hangup write errors and the descriptor leaks,
one per hangup for the life of the emulator. Body rewritten to say that, with
the BSD-only scope stated in it rather than left for a reader to discover from
CI.

I had the branch reviewed here before pushing. It confirmed the rebase is
lossless — the pre-rebase and post-rebase patches are byte-identical across all
four source files — and re-derived your discrimination result independently by
building both emulators: green with the fix, red without, failing with
announced socket hangup failed: Socket is not connected.

Not done: the descriptor-counting variant you suggested for platform-independent
detection. That is a better test than this one and I would rather offer it
separately than bolt it on here.

[Ticed]

The force-push dismissed your approval, sorry. To make re-reading cheap, here is the exact delta from the commit you approved (2be32d36) to the tip (b3802781):

emu/port/ipif-posix.c    unchanged
emu/port/ipif6-posix.c   unchanged
emu/FreeBSD/ipif.c       unchanged
dis/tests/tcp_test.dis   deleted
tests/tcp_test.b         +5 lines, comment only

The C you reviewed is byte-identical. The five lines are the BSD-only comment you asked for, above testAnnounceHangupReleasesPort. The .dis is gone by rebase, resolved by deletion as you specified.

The PR body also changed, and that is a correction rather than a cosmetic edit — it claimed the re-announce fails, which you pointed out is wrong. It now says the hangup write errors and the descriptor leaks, and states the BSD-only scope so it is not left for a reader to infer from CI.

[Ticed]

Rebased onto current master (df34b02). Force-pushed b380278 -> 594ab72.

git range-diff reports every commit unchanged — same content, same messages,
only the base moved. The five commits that landed underneath are #583, #582,
#566, #578 and #580; none of them touches this branch's files.

Re-verified on macOS after the rebase.

testAnnounceHangupReleasesPort passes. Reverting so_hangup in a scratch
build and rebuilding the emulator turns it red — "announced socket hangup
failed: Socket is not connected" — so it still discriminates on this host rather
than passing for free. Restored, rebuilt, green again. The BSD-only comment is
untouched.

#578 fixed the ClusterFuzzLite link break, so Fuzz should now be green here
rather than red for a reason that was never this branch's.