asp: wait only in poll() so deferred signals are always reachable - #3275
Conversation
2c3eace to
2bfea07
Compare
andylemin
changed the title
2bfea07 to
b2400c9
Compare
|
@NJRoadfan This one is for you 😉 Could you help test this? 🙏 @rdmark Finally brought ASP up to speed and integrated with IPC 🎉 We should try and stress test this for ASP clients? |
b2400c9 to
4857080
Compare
|
@NJRoadfan @rdmark Now ready for testing (extensively reviewed) - can we request some help from some classic mac users? They should be happy to see these changes, as this offers a lot of gains for our classic user base 🙂 This offers; improved performance, latency, security, stability, and compatibility for ASP. And improved hardening for DSI. Any ASP user running more than a single login, will see notable coherence improvements. |
|
I connected a client and was able to copy files to/from the server. Anything else needed to test here? I don't exactly have a setup handy at the moment where I can have multiple machines access the server. Single user testing shows no regressions. ASP is still noticeably slower as a transport over DSI but no new regressions when I timed a file copy. |
Thanks @NJRoadfan, no there is nothing special to test. These changes are the kind of changes where if there was something wrong, even your simple test would have shown it. Thank you! Thats correct, this does not speed up the ASP protocol itself.. It just resolves various latency issues where ASP/ATP exchanges get stuck/buffered, and not picked up immediately. And dircache was never updated at all, so ASP clients would continue working against a stale cache when other users changed files. This fixes all of that. |
d9f2095 to
2b1332c
Compare
The ASP request loop did its signal work inside the handlers — LOG(),
close_all_vol(), free(), PAM logout, blocking network I/O — which deadlocks or
corrupts the heap when it interrupts the main flow mid-allocation or holding the
syslog lock. Handlers now record and wake, as the DSI transport does.
Deferring that work is only safe if the loop can always be woken, and a
self-pipe can only wake a loop that waits in poll(). ASP makes that harder than
DSI does: a readable ATP socket does not promise a request, because the datagram
may belong to another transaction or another node, and atp_rreq() consumed
datagrams until one matched. The loop was therefore parked in recvfrom() with
the signal and hint pipes unwatched, where no self-pipe write could reach it.
atp_rreq_try() makes a single attempt and reports "nothing yet" separately from
failure; atp_rreq() keeps its blocking contract by looping it, so the callers
outside ASP are unaffected. asp_getrequest() uses the single-attempt form, and
the loop enters it only once poll() has reported the socket readable. A datagram
that yields no request returns to the wait. poll() is now the only place the
loop blocks, and it always waits without a timeout.
SA_RESTART is retained on every handler, as the DSI handlers keep it. Clearing
it would make each interruptible syscall in the child fail with EINTR: a SIGHUP
config reload arriving while atp_sresp() blocks in netddp_sendto() surfaces as
asp_cmdreply() < 0, which the loop treats as a dead client, so a reload would
end healthy sessions. The self-pipe is what makes the wake-up reliable, and it
suffices because nothing waits outside poll().
The loop is split into an inner wait and an outer serve and carries the DSI
loop's [A0]-[I] section labels, so the two transports can be read against each
other. All deferred work is funnelled through one asp_process_deferred_signals()
called at [A0] and [E1], so a reload is acted on in the wake-up that delivered
it. The hint pipe is drained ahead of the request, because a sibling's
invalidation and the command it invalidates can arrive in the same wake-up and
answering first would use the entry the hint retires; process_cache_hints() is
driven by revents rather than run on every wake.
The self-pipe itself moves to libatalk/util/sigpipe.c, using the existing
setnonblock() helper and checking what it calls. Both transports use it, so the
private copy in afp_dsi.c is removed. That also retires its unchecked fcntl()
calls: a failed F_SETFL left the pipe blocking, and sigpipe_drain() runs
unconditionally at the top of every iteration, so such a session would have hung
on its first pass. A half-configured pipe is now closed rather than left in
place, and a second init is a no-op.
ATP fixes:
- one atp_queue_push() is the single entry to the unmatched-receive queue. It
is bounded, evicts the oldest in one pass, and never sacrifices the packet
just received: that is the only one a caller may still be waiting for, and
discarding it lost every request thereafter, because the entries already
queued are the ones nothing can claim.
- a packet with no ATP function bits is discarded at validation. It satisfies
every ((*func & rfunc) == rfunc) test, so it would be handed back as the
response to whichever transaction asked first, and once queued no request
could ever claim it.
- atphd_bitmap is range-checked before it indexes atph_resppkt[] or is used as
a shift count. It is a sequence number straight off the wire, and past the
width of int the shift is undefined: on the usual targets the count is
masked, so 32, 64 and so on alias onto bits the bitmap test accepts.
- a response buffer stored in atph_resppkt[] is no longer freed when the
Send-Transaction-Status resend fails. The slot kept the pointer, so
atp_sreq/atp_rresp/atp_close would push the same buffer onto the pool free
list a second time and hand it to two owners.
ASP fixes:
- every byte read out of cmdbuf goes through unsigned char. It is signed on
the usual ABIs, so a function byte of 0xFF/0xFE/0xFD was returned as one of
the negative result codes, and a session id of 0x80 or above never matched
asp_sid, leaving that session answering nothing.
- a request shorter than the ASP header is rejected before cmdlen, a size_t,
wraps to near its maximum and reaches the command handlers as their input
length.
- afp_asp_close() treats only a failed seteuid restore as fatal. Exiting when
the restore had succeeded skipped close_all_vol() and the PAM logout.
Loop robustness: POLLNVAL/POLLERR on the self-pipe ends the session rather than
spinning on an event nothing consumes, and the sequence and session-id
mismatches any spoofed datagram produces neither clear nor increment the
consecutive-read-error counter, and are counted and logged periodically instead
of one line each.
The ATP queue behaviour is covered by unit tests that fail if the queue reverts
to discarding the arriving packet.
Two small fixes fall to the DSI loop, which shares these seams:
- a command frame carrying no AFP function byte is rejected. Every length
check on both paths was an upper bound, so a zero-length payload reached the
dispatcher, which reads commands[0] to choose the AFP call and would take it
from whatever the previous request left in the buffer, then run it with an
input length of 0. DSI rejects it in dsi_stream_receive() beside its other
frame checks; ASP rejects it in asp_getrequest() for the two functions that
carry a call.
- POLLHUP on the self-pipe ends the session. Nothing consumes it, so poll()
would return immediately for the rest of the session and the loop would spin
with no way to be woken. DSI tested only POLLIN, so it also missed POLLNVAL
and POLLERR.
ATP transaction fixes:
- atp_rsel() reports "keep waiting" only while retries remain. The two early
returns for a response it cannot use skipped the retry-exhausted check at
the end of the function, so atp_rresp() looped forever; once
resend_request() had spent the last retry, `requesting` went false, which
drops the select() timeout and leaves the next receive blocking with nothing
to break it. All three exits now share one guard that fails with ETIMEDOUT
when the transaction is spent.
- the ATP buffer pool can be released. It carved each malloc into ten buffers
and recorded nothing, so the chunk bases were unrecoverable and the pool was
unfreeable by construction. The unit tests now use the real allocator and
hand everything back.
A read error is no longer counted or logged as a stray packet. It fell through
to the switch default, where the stray-packet rate limit meant up to nine
consecutive failures produced no line at all and the tenth reported no errno.
Each is now logged with its cause, and the dead errno == EINTR retry is gone:
SA_RESTART makes it unreachable, and it could only ever have bypassed the bound
it sat above.
Unit tests cover the DSI zero-length rejection and the ATP queue eviction; both
go red if the guard or the eviction policy is reverted.
2b1332c to
ebb17cf
Compare
|
@rdmark ~90 of them are the imported AFP testsuite (test/testsuite/FP*.c, T2_*.c, spectest.c, afpclient.c, …), We will need to create a separate PR and do a git history audit (largely didg/Lahm/Boehme/Markstedt work). |
|
📊 Performance DashboardCommit: 🔥 Spectest (AFP 3.4) - FlameGraphNetatalk Code-time: 3.0% · Runtime: 62s · Stacks: 1093 🔥 Click the preview to open the interactive flamegraph (zoom + search). 🔝 Top 10 leaf functions
📈 Speedtest (AFP 3.4) - PerfGraphPeak Read: 6746 MB/s (-6.8% vs hist avg 7241.9 MB/s; min 6309 / max 9526 over 30 PRs) 🔝 Throughputs per operation (vs. historical average)
⏱️ Lantest (AFP 3.4) - LatencyGraphAvg total runtime: 2302 ms (-45.0% vs hist avg 4185.6 ms; min 2122 / max 5208 over 30 PRs) 🐢 All operations (avg runtime, in test order, vs. historical average)
Run baseline: median op-test delta -37.5%, MAD 7.9%. Adj Δ% shifts each delta by the median; standouts ≥5% in bold. A large MAD means the run did not move uniformly — read the adjusted column with caution. |
|
@andylemin it's valuable to see analytics about source files lacking copyright notices! my concern is primarily adding notices to newly created files -- historical files that exist in the repo have been vetted by downstream packagers already (Debian and Fedora in particular) so there's little risk they'll flag any new copyright or licensing concerns for those it is true that as we refactor older files, historical code may move into new files, which is indeed a time when we should clarify authorship and add copyright notices where missing! |
3 participants
The ASP transport never received the loop work the DSI transport has had since February. Its request loop was still
while ((reply = asp_getrequest(asp)))— a bare blocking read, nopoll(), signal work done inside the handlers, and a sibling dircache hint pipe that the child was handed but never read.This PR brings ASP to the same footing as DSI:
poll()as the single blocking point with a self-pipe wake, async-signal-safe handlers, and IPC cache hints actually consumed. It also repairs an unbounded ATP receive queue, a packet class that matched every request, twocmdbufsign-extension faults, asize_tunderflow reaching the command handlers, an out-of-bounds store driven by one wire byte, and a pooled-buffer double free.It adds the first unit tests the AppleTalk transport has ever had. Two of the seams it touches are shared, so a pair of minor fixes fall to the DSI loop as well — see below.
What this brings ASP that DSI already had
poll(-1)loop that watches itasp_getsess()), then never read —process_cache_hints()was called only fromafp_dsi.c[A0]–[I]poll loopLOG(),close_all_vol(),free(), PAM logout and blocking network I/O in signal contextIPC cache hints are the headline.
asp_getsess()builds the hint pipe, registers the write end with the parent and stores the read end inasp->asp_hint_fd;073379dfthen wired it toobj->hint_fdin the child. But nothing in the ASP path ever drained it — onmain,process_cache_hints()has exactly two call sites, both inafp_dsi.c. So an ASP session's dircache was never invalidated by a sibling's activity: it served stale entries for the life of the session, while the parent filled the pipe and then discarded hints once it was full. ASP now fully participates in cross-process cache coherency.What ASP deliberately does not gain: #2808's idle worker. ASP has no
iw_*integration (DSI has fifteen call sites) because the rfork cache tier it maintains is a TCP-path optimisation of no benefit over AppleTalk, sodircache_init()skips that allocation for ASP sessions.[A1]below is ASP's own substitute for the invalid-entry release DSI does underiw_is_active().Latency and stability follow from the same change rather than being separate work: the loop now waits in
poll()instead of a blocking read, so hints and signals are serviced at the moment they arrive rather than whenever the next client request happens to turn up — which, for an idle client, was never.Root cause
ASP is harder to drive from
poll()than DSI, and the reason is the protocol, not the code quality. DSI reads a stream: oncepoll()reports readable,dsi_stream_receive()has data and cannot block — its own comment says exactly that. ATP is a datagram protocol, and a readable socket does not promise a request: the datagram may be a response to another transaction, a release for a timed-out one, or traffic from a different node.atp_rreq()therefore looped, consuming datagrams until one matched:Each
atp_rsel()performs one blockingrecvfrom(). So the loop's resting place was inside that read. Anything that wants to reach an idle ASP child — a signal, a cache hint — has nowhere to arrive, because the child is not watching any descriptor. That is the whole of it, and both the stale-dircache behaviour and the signal-context handlers are consequences: with nopoll()to return to, there was no safe place to defer handler work to, and no point at which the hint pipe could be read.SA_RESTARTis retained on every handler, as DSI retains it. Clearing it would make each interruptible syscall in the child fail withEINTR: a SIGHUP reload arriving whileatp_sresp()blocks innetddp_sendto()surfaces asasp_cmdreply() < 0, which the loop reads as a dead client, so a reload would end healthy sessions. The self-pipe is what makes the wake-up reliable, and it suffices precisely because nothing now waits outsidepoll().Revision note, since this branch was previously pushed: an earlier revision of this PR deferred the handler work but tried to bridge the blocking read with
shutdown(asp_atp_fd, SHUT_RDWR)from the die handler. That cannot work — the ATP socket issocket(AF_APPLETALK, SOCK_DGRAM, 0)(libatalk/netddp/netddp_open.c) and dgramshutdown()is unsupported there, with the return value discarded — and where it does take effect it is worse, becauseatp_recv_atp()treats onlyrecvlen < 0as an error, so a permanently-0-returningrecvfrom()spins. That approach, and theasp_atp_fdit needed, are gone; the read itself no longer blocks.How this code got here (2000 → 2011 → 2024)
Not anyone's mistake. This is the 2000 original, and it missed the hardening DSI received because for most of the intervening time it was not in the tree at all.
31843674— rufustfirefly, 2000-07-25 (initial revision). Every defect below is present here:while ((reply = asp_getrequest(asp)))as the entire request loop,atp_rreq()'s consume-until-match loop,asp->cmdlen = atpb.atp_rreqdlen - 4,asp->cmdbuf[1] != asp->asp_sid, andatph_resppkt[resp_hdr.atphd_bitmap]indexed by a raw wire byte. All were reasonable then: thecmdbuffault needs a session id above 127, hence 128 concurrent sessions; the queue needs a peer willing to send unmatchable datagrams.be96d276— Frank Lahm, 2011-02-24 ("Remove all Appletalk stuff"). AppleTalk leaves the tree. From here DSI is hardened alone.983e15c9— Daniel Markstedt, 2024-07-24 ("Reintroduce AppleTalk modules in libatalk"). The 2000 code returns essentially unchanged, into a tree whose DSI loop had moved on.cc0c751e/a2fa3170(Enumerate uses cache, AD support in cache and inter-process cache sync #2733, 2026-02),3c424057(dircache: add idle worker thread for background cache maintenance #2808, 2026-03),2ea63298(afpd: make signal handlers async-signal-safe to fix crash on macOS #2860, 2026-04). The dircache hint pipe, the idle worker and the async-signal-safe self-pipe loop all land inafp_dsi.c. ASP, back for under two years, gets the hint plumbing via073379dfbut no consumer.Attribution note:
git blamecredits thecmdlen,cmdbufandatph_resppktlines to983e15c9and to200f3999(the 2025 astyle reformat), because both rewrote every line in the file. Neither introduced anything — the logic is verbatim 2000, confirmed against the31843674blob.Coverage matrix — what could reach an ASP session
Each event the loop must answer. "Idle" means a connected client not currently issuing a command, i.e. parked in
recvfrom()— the common case.Legend: ✓ = handled promptly and safely · ⚠ = handled, but from signal context, where
LOG()can deadlock on the syslog lock andfree()/close_all_vol()can corrupt the heap if they interrupt mid-allocation · ~ = delayed until the next client request, unbounded for an idle client · ✗ = never.mainasp_attention()does blocking network I/O,afp_asp_close()runsclose_all_vol()+ PAM logout +free()asp_attention(),setitimer(),sigaction()from a handlerSERVERTEXT)readmessage()+asp_attention()from a handleratp_close()atphd_bitmapFix
atp_rreq_try()makes exactly one attempt and reports "nothing yet" (0) separately from failure (-1).atp_rreq()keeps its existing blocking contract by looping it, so its six callers outside ASP —bin/pap,etc/papd(two sites),contrib/timelord,contrib/a2boot,contrib/macipgwandasp_getsess()— are untouched.asp_getrequest()uses the single-attempt form and returnsASP_NOREQUESTwhen the datagram was not a request for this session.The loop enters the read only once
poll()has reported the socket readable, andpoll()always waits without a timeout. EveryASP_NOREQUESThas consumed exactly one datagram, so the socket is no longer readable and the nextpoll()blocks — there is no spin.Structurally it is now DSI's loop: an inner wait and an outer serve, carrying the same
[A0]–[I]section labels so the two transports read side by side. DSI's[A](stream data already buffered) and[B](disconnected or dying) have no ASP counterpart — no stream buffer, no reconnect state machine — and the code says so, to save the next reader hunting.[A1]is ASP's own: releasing what the hints pruned, which DSI does after each command gated oniw_is_active(), but ASP does in the wait so an idle session still sheds other clients' churn while receiving nothing but hints.flowchart TD A0["<b>[A0]</b> drain self-pipe · act on deferred signals<br/><i>die · timedown · reload · server message</i>"] A1["<b>[A1]</b> release hint-pruned dircache entries"] C["<b>[C]</b> build pollfd set<br/><i>ATP socket · hint pipe · self-pipe</i>"] D{{"<b>[D]</b> BLOCK IN POLL — timeout −1<br/><i>the only place this loop waits</i>"}} E{"<b>[E]</b> poll error?"} E1{"<b>[E1]</b> self-pipe<br/>healthy?"} F["<b>[F]</b> apply cache hints<br/><i>ahead of dispatch</i>"] G{"<b>[G]</b> ATP fd<br/>valid?"} H{"<b>[H]</b> socket<br/>readable?"} I["<b>[I]</b> hints or signals only"] R["<b>asp_getrequest</b><br/>one datagram · cannot block"] S["dispatch AFP command"] X(["end session"]) A0 --> A1 --> C --> D --> E E -->|"EINTR"| A0 E -->|"fatal"| X E -->|"ok"| E1 E1 -->|"POLLNVAL / POLLERR"| X E1 -->|"POLLIN — drain + act"| F E1 -->|"quiet"| F F --> G G -->|"POLLNVAL"| X G -->|"ok"| H H -->|"no"| I --> A0 H -->|"yes"| R R -->|"ASP_NOREQUEST<br/>stray or replayed"| A0 R -->|"command"| S --> A0 classDef blockNode fill:#1f6feb,stroke:#1158c7,color:#ffffff classDef endNode fill:#a40e26,stroke:#82071e,color:#ffffff classDef serveNode fill:#1a7f37,stroke:#116329,color:#ffffff class D blockNode class X endNode class R,S serveNodeHow each element maps to a defect
Baseline is
main.mainrecvfrom(), never inpoll()atp_rreq_try(); read only after[H]LOG()deadlocks on the syslog lock,free()/close_all_vol()corrupt the heap mid-allocation[A0]/[E1][F], ahead of[H]atp_close()atp_queue_push(), bounded, evicting the oldest in a single pass — never the arriving packet, which is the only one a caller may still be waiting for((*func & rfunc) == rfunc)test, so it is returned as the response to whichever transaction asks firstatphd_bitmapunboundedatph_resppkt[8]out of bounds; the shift is undefined pastint, and on the usual targets the count is masked, so 32, 64, … alias onto bits the bitmap test acceptsATP_MAXRESPatp_free_buf()on a Send-Transaction-Status resend failureatph_resppkt[]slot keeps the pointer, so the buffer reaches the pool free list twice and has two ownersasp->cmdlen = atp_rreqdlen - 4,cmdlenasize_t~SIZE_MAXas theibuflenhanded to the command handlersASP_HDRSIZcmdbuf[1] != asp_sid,charvsuint8_tmax connections = 200unsigned charon everycmdbufreadcmdbuf[0]returned ascharafp_asp_close()exits whenever euid ≠ login userexit()sat outside the failed-seteuidbranch, so a successful restore still skippedclose_all_vol()and the PAM logoutMinor DSI loop fixes
Three of the defects live in seams both transports share, so they are fixed on both sides rather than only on the one this PR is named after. All three are small and none changes the DSI loop's structure.
dsi_stream_receive()was an upper bound, and both dispatch sites readcommands[0]with nocmdlen >= 1guardPOLLHUPon the self-pipe ends the session[E1]tested onlyPOLLIN, so it missedPOLLHUP,POLLNVALandPOLLERRpoll()returns immediately for the rest of the session and the loop spins with no way to be wokenafp_dsi.chad its own copy whose fourfcntl()calls were uncheckedF_SETFLleft the pipe blocking, andsigpipe_drain()runs unconditionally at the top of every iteration — that session would have hung on its first passThe migration also deletes ~45 lines of duplicated machinery, which is what the "still two copies" follow-up in the previous revision of this description was about. There is now one implementation, hardened: a half-configured pipe is closed rather than left in place, and a second init is a no-op.
Behaviour changes
asp_process_deferred_signals()called at both[A0]and[E1], as DSI does.atp_rreq()'s contract is unchanged;atp_rreq_try()is additive.process_cache_hints()runs onrevents, so stray traffic does not cost a read and a parse each.libatalk/util/sigpipe.c, using the existingsetnonblock()helper and checking what it calls.afp_dsi.cstill has its own copy (see follow-ups).Testing
Container build,
-Dwith-appletalk=true -Dwith-tests=true,-Wall -Wextra: clean, 198/198 targets.Warnings are a delta against
mainmeasured per file, not an absolute count:libatalk/atp/atp_rsel.ccarries two pre-existing-Wsign-comparewarnings on thenetddp_sendtoreturn comparisons.mainbuilt with the same flags emits the identical two, at the same source expressions. Delta zero; no other changed file warns. For anyone reproducing this:with-appletalkdefaults to false, so a default build compiles none of these files and reports no warnings for them.Unit tests — the first the AppleTalk transport has ever had
test/afpd/subtests_atp.cis the first unit test coverage for ASP/ATP in the project's history. Onmain, searchingtest/foratp_,asp_orappletalkmatches zero files — the transport had none, while DSI has nine distinct unit tests in the same harness. That gap is a large part of why the 2000-era defects above survived reintroduction unnoticed, and it is why this PR starts closing it rather than only fixing the code.afpdtest: 81 ok, 9 skip, 0 failures.Both new cases are regression tests in the strict sense — reverting
atp_queue_push()to discarding the arriving packet turns them red, which was verified rather than assumed:The ATP buffer pool is now releasable, which is what lets these tests use the
real allocator. It carved each
mallocinto ten buffers and recorded nothing, sothe chunk bases were unrecoverable and the pool was unfreeable by construction —
valgrind reported the base as possibly lost, which is in its default error set,
and CI runs
meson test --wrapper='valgrind --leak-check=full --error-exitcode=1'.atp_bufs_release()frees the chunks, so the tests exercise the same ownershipthe transport does and leave nothing of their own on the pool's free list.
Spectests
Green, but as regression assurance only: spectest runs over TCP/DSI and does not exercise ASP at all, so it covers none of this branch's own behaviour. Stated rather than implied.
Not in this PR (follow-ups)
cmdlenbound, thecmdbufsignedness and theatphd_bitmaprange check. Each needs a live ASP session or a handle mid-transaction, which the afpd unit harness cannot set up. They are verified by reading.poll()rather than in a read, so a future change can silently reintroduce the blocking read that makes deferred signals and hints unreachable. This is the PR's central property and its least protected one.contrib/macipgwfills the sameatph_queuethrough its own unbounded pusher and then callsatp_rsel()on that handle. It is unaffected by the eviction policy, but it should go throughatp_queue_push()rather than keeping a second implementation of the same queue.afpdtestis not run bymeson testin the Alpine configuration, andtest/afpd/test.shonly generatestest.conf, so the binary must be run separately; the image needsutil-linuxforuuidgen. The unit results above were obtained that way.