Otherwise, if the timer event source is enabled, a new query may be
invoked by the already removed service.
This also renames dns_remove_service() to dnssd_discovered_service_detach(),
and drops unused DnssdDiscoveredService.query.
When a DnsServiceBrowser object is detached from the manager,
all queries, event sources, and then the reference to Manager should be
dropped. Otherwise, spurious query may be triggered for already
disconnected varlink.
Cover the size derivation and the packing decisions that the integration
test cannot reach: its veth MTU exceeds every record TEST-89 publishes,
so neither the oversized-record fallback nor the drop of a record beyond
the RFC 6762 section 17 ceiling is ever exercised there.
The cases pin the two size bounds for both families (including the
fallback for an MTU not known yet and the jumbo-frame cap), the split
at max_size, the lone oversized record in a fragmented packet, the
drop of a record that fits nowhere, the skip of a record that cannot be
encoded at all, that
answer-item flags reach the wire (a goodbye must keep TTL 0 and its
cache-flush bit), and that name compression stays within a packet --
the property the split exists for, since every DNS-SD name shares a
suffix and a compression pointer may not cross a packet boundary.
dns_scope_announce() put the entire zone -- for DNS-SD every PTR/SRV/TXT
record of every published service -- into a single reply packet. With a
few hundred published services that grows far beyond the interface MTU,
and beyond what RFC 6762 section 17 permits: a packet larger than the
MTU is sent using fragments, and such a packet "MUST NOT contain more
than one resource record".
Pack the announcement into as many MTU-sized packets as needed instead.
A single record that does not fit the MTU by itself is emitted alone in
an oversized packet, relying on IP fragmentation, which the same section
explicitly allows for that case -- bounded by its hard 9000-byte
ceiling, headers included, which also caps jumbo-frame links. While the
interface MTU is not known yet, packets are cut to the datagram size
every host of the family must be able to accept -- RFC 791 section
3.1's 576 octets for IPv4, whole or reassembled, and RFC 8200 section
5's 1280-byte minimum link MTU for IPv6, which no link fragments --
rather than to that ceiling, so an announcement is not packed for
deliberate fragmentation merely because the MTU was not read yet.
A record that cannot be encoded at any size -- a character string past
255 bytes, say -- is skipped like one too large for the ceiling, rather
than failing the whole announcement: dns_packet_append_rr() rolls the
packet back on every failure, so its neighbours are unaffected either
way. The packetizer returns how many it dropped and the scope warns on
it, since nothing else would distinguish an announcement that carried
everything from one that did not -- and for a withdrawal that matters:
the caller drops the records from the zone either way, so one skipped
here is never withdrawn at all and peers keep it for its full TTL.
The packing itself lives in resolved-mdns.c as mdns_announcement_packetize(),
taking an answer and the two size bounds and returning finalized packets,
so it can be tested without a scope, a link or a socket.
Emission is best effort per packet -- a failed send no longer withholds
the announcement's remaining packets -- and each pass logs the packet and
record counts it actually put on the wire. Each packet takes its own
token from the scope's multicast rate limit: that limit bounds datagrams
on the wire, and an announcement that would exhaust its burst is the
very flood it exists to cap.
dns_scope_send_goodbye(), the other unsolicited-response emitter, goes
through the same packetizer: a withdrawal of many records had the same
oversized-packet problem, and a single record failing to append
abandoned the whole withdrawal. Its return value follows the emitter's
now, so a withdrawal split across packets reports an error when any one
of them fails to go out.
So far we didn't limit the number of jumps when decompressing a DNS
packet. This can lead to some extra work with specially crafted packets
that send a staircase of thousands of descending compression pointers,
which can then be amplified by referencing such staircase from thousands
of names.
Given that legitimate DNS packets need only a handful of jumps in the
compression chains, let's limit the number of jumps per a single name to
256 (the same limit as, for example, Unbound currently has).
Reported by opensec-intelligence (OpenSec).
dns_packet_append_name() appends a name's labels and then the root byte
that terminates it. A failure on that last byte returned directly,
leaving both the labels on the packet and the compression entries this
call had added -- entries naming offsets the caller then truncates away.
Take the same rollback path as every other failure in the function.
Independent of the map invariant fixed before it: this is reachable
whether or not any offset exceeded the pointer range.
The existing case only covered a fully-uncompressed re-occurrence. Every
DNS-SD name shares an in-range suffix, so also pin that a name first
seen beyond the pointer range still compresses from its mapped suffix --
one label plus a two-byte pointer per occurrence, map unchanged, and the
pointer addressing the suffix's prior occurrence rather than any
in-range octet.
A padding helper puts the next append at a chosen offset, for the two
sides of the boundary: at 0x3FFF the name is still mapped and re-occurs
as a pointer whose wire bytes are checked independently of the parser;
one past it, it is not mapped.
dns_packet_append_name() added every name it appends to the packet's
compression map, including names written at offsets >= 0x4000 that an
RFC 1035 compression pointer (14 bits) can never reference. Such
entries are not just useless -- when the same name occurs again, the
lookup finds it, the pointer cannot be emitted, the code falls through
to appending the labels again, and the re-insertion into the map then
fails the whole packet build with hashmap_ensure_put() returning
-EEXIST.
This is easily triggered with mDNS: an unsolicited DNS-SD announcement
covering a zone with a few hundred services exceeds 16KiB, and
dns_scope_announce() then fails with "Failed to build reply packet:
File exists".
This lifts the failure only: dns_scope_announce() still builds the whole
zone into one packet and hands it to the socket as a single datagram,
fragmented as the link requires. Splitting announcements across
MTU-sized packets, as RFC 6762 section 17 asks, is done separately.
Only insert a name into the map if its offset is pointer-expressible,
making "mapped implies referenceable" a map invariant. Re-occurrences of
out-of-range names are emitted as labels again -- the only encoding RFC
1035 section 4.1.4 permits in that case -- still compressed from the
first suffix that is mapped in range.
The lookup restates that range as control flow rather than asserting it.
Under -Db_ndebug=true, which CI builds, assert() is
__builtin_unreachable(), so an out-of-range entry would put 0xC000 | n
truncated to 14 bits on the wire -- a pointer to an arbitrary earlier
offset -- where falling through emits the labels instead. This layer is
shared with unicast DNS, DoT and LLMNR.
The pointer's tag bits and its offset bound are now named once, in
dns-def.h next to the other wire-format constants rather than in
dns-packet.h, since the format is RFC 1035's and not this packet
abstraction's -- dhcp-message.c open-codes the same two bits to parse
compressed names out of DHCP options. Compile-time checks pin that the
two partition the 16-bit field and that the tag is its top two bits,
which is what lets the reader take it from the first octet alone.
The offset's other bound becomes control flow along with it: it was an
assert(), which under -Db_ndebug=true is no check at all, for the bound
whose violation puts a pointer to the wrong bytes on the wire. It is
also tightened to the start of the name being appended rather than the
end of the packet, which is the "prior occurrence" RFC 1035 section
4.1.4 allows a pointer to address and what dns_packet_read_name()
enforces on the way in.
Add regression tests, including one through dns_packet_append_rr() --
the path the reported failure came from, where the name append is nested
inside the rdlength backpatch and two further rollbacks -- and one for
the reader's label-type dispatch, which the renamed constants touch.
manager_recv() sizes its buffer with next_datagram_size_fd(), which peeks at
the datagram with MSG_PEEK|MSG_TRUNC. That peek makes the kernel verify the UDP
checksum - which is the whole point of using it over FIONREAD - and hence also
drop the datagram when the checksum is bad. The datagram that woke the event
source can thus be gone by the time we peek at it, and the peek returns EAGAIN.
manager_recv() returned that error, while the recvmsg_safe() below, which can
fail the same way, already treats it as "no packet". The callers on the mDNS,
LLMNR and stub listening sockets pass a negative return on to the event loop,
which disables the event source, so a single datagram with a bad checksum -
something anyone on the link can send - stops resolved from receiving on that
socket for good. on_dns_packet() completes the transaction with -EAGAIN instead
of retrying it.
So return 0 on a transient error here too, like the other users of the helper
on an event source do. Disconnect errors are left as they are: they show up
here from ICMP errors on the connected UDP socket, and on_dns_packet() handles
them.
mdns_maintenance_query_complete() takes a reference on
query->dnsservice_request and never drops it, so every successful mDNS
maintenance query leaks one reference to the discovered service.
The leak alone would be benign, but the service owns the one-shot event
source that drives its maintenance schedule, and it only borrows a plain
pointer to its service browser. Once the extra reference is outstanding,
dns_service_browser_free() -> dns_remove_service() only drops the list's
reference, so dns_service_free() never runs, the maintenance timer is
never disabled, and the service outlives the browser it points at. The
next time the timer fires, mdns_maintenance_query() dereferences the
freed DnsServiceBrowser to reach the manager's event loop and to issue
the query.
Reaching it only takes a browse subscription whose record lives long
enough for one maintenance query to be answered, followed by the
subscription going away.
Make the reference a _cleanup_ one, the way the sibling
mdns_browse_service_query_complete() below already handles the reference
it takes on the service browser.
Since the D-Bus DNS-SD API was introduced in c3036641f0 ("resolved:
implement D-Bus API for DNS-SD", v236), RegisterService() has set up an
sd_bus_track object so that a registered service is destroyed when the
client that registered it disconnects from the bus — there's even a
handler logging "Client of active request vanished, destroying DNS-SD
service."
However, the track object was never stored anywhere: it is declared as
a _cleanup_(sd_bus_track_unrefp) local in bus_method_register_service()
and hence destroyed again when the method handler returns, taking the
tracking down with it. The vanish handler was thus dead code, and
services registered via D-Bus silently persisted after their client
disconnected — unowned, impossible to re-register (the identifier
stays taken, failing with DNSSD_SERVICE_EXISTS), and advertised until
systemd-resolved itself exited.
Store the track object in DnssdRegisteredService, pinning it for the
lifetime of the service, so that client tracking works as originally
intended. Route the vanish path through the full unregistration logic
(goodbye packets + removal from the per-link mDNS zones), since freeing
the service alone would leave its RRs published in the zones.
Note for fire-and-forget clients (e.g. one-shot busctl/gdbus calls):
these were never reliable — the registration silently vanished on any
resolved restart — and now behave as documented for the corresponding
Avahi API as well: the bus connection must be kept open for as long as
the service is to remain registered. Document that in the man page and
NEWS, and point to *.dnssd files for registrations independent of a
client's lifetime.
dns_scope_announce(scope, /* goodbye= */ true) is not scoped to a
specific service: it walks the whole per-link mDNS zone and flags every
established RR as goodbye (TTL 0). Unregistering a single DNS-SD
service via the UnregisterService() D-Bus call hence withdrew the
host's own A/AAAA records and every other published service on the
link from remote caches, with nothing re-announcing them afterwards.
Introduce dns_scope_send_goodbye(), which sends an unsolicited response
for a caller-provided RR set, and let the DNS-SD unregistration path
send goodbye only for the RRs of the service actually being removed,
mirroring the flag logic of dns_scope_announce().
The goodbye answer is built per scope, and only includes records
actually established in the respective zone (or under re-verification,
in which case they are still served from it): a per-service goodbye
must take care not to withdraw records that are still being probed for
uniqueness, were withdrawn again after a conflict (in which case the
name belongs to the neighbor), or were never published in the scope in
the first place (e.g. on MulticastDNS=resolve links, which have an mDNS
scope but do not publish DNS-SD services).
This becomes more important with the next commit, which promotes
unregistration from an explicit method call to an automatic consequence
of the registering client disconnecting from the bus.
Move the logic that sends goodbye messages, removes a registered DNS-SD
service's RRs from all mDNS zones and frees the service out of
bus_dnssd_method_unregister() into a new helper. While at it, add the
nowadays customary parameter comments and ", ignoring" log message
suffix to the moved code. No functional change.
This is preparation for unregistering services when the D-Bus client
that registered them disconnects: that path needs the full teardown as
well, not just dnssd_registered_service_free(), which would leave the
service's RRs published in the per-link mDNS zones.
Like the command-level CommandDescription.argspec, a verb's argspec is now
a nulstr, so that a verb may declare multiple alternative usage lines. The
verbs help table renders one synopsis line per nulstr entry in the
synopsis cell.
All verb argument specifications must gain an explicit NUL suffix. This
is a lot of churn, but I think this needs to be done. For now, there'll
be only one user of the new functionality (i.e. a verb with more than
one argspec), but I think we'll eventually want to use this in other
places too.
dns_cache_lookup() computes each answer item's expiry ('until', also the
basis of the SD_RESOLVED_CLAMP_TTL TTL patch) as "30s from now" whenever
the caller did not pass SD_RESOLVED_NO_STALE — keyed on the lookup mode,
not on whether the entry being returned is actually stale. That misfires
in both directions:
* A fresh entry hit by a serve-stale retry (second transaction attempt
with StaleRetentionSec= set) has its TTL reported as 30s instead of
its real remaining TTL.
* A lookup that passes neither SD_RESOLVED_NO_STALE nor
SD_RESOLVED_CLAMP_TTL — as the mDNS browse path does for clients
that set no flags — leaves 'current' at 0, so every answer item's
expiry becomes the constant absolute time 30s after boot,
permanently in the past. The RFC 6762 section 5.2 cache maintenance
schedule that resolved-dns-browse-services.c derives from
item->until then degenerates to its already-expired fallback.
Key the rewrite on the entry's actual staleness instead: an entry whose
TTL has lapsed keeps reporting "30s from now", so clients don't hold on
to stale data for long, and a fresh entry reports its real expiry. For
lookups with SD_RESOLVED_NO_STALE set nothing changes: a stale entry
already makes the whole chain miss, so every item returned in that mode
is fresh.
This converts systemd-homed, -hostnamed, -importd, -localed, -logind,
-machined, -networkd, -portebled, -resolved, -sysupdated, -timedated,
and -timesyncd.
The service name and description move from service_parse_argv() arguments
into a COMMAND() description that each daemon now defines. The
service_parse_argv() macro captures the calling binary's verb section,
where that COMMAND is found, while the options are still defined and
parsed in libshared, in the new "service" option namespace that each
COMMAND references. (The named namespace makes sure that options defined
by other translation units in libshared can never leak into the
services' parsers.)
The supported features are declared via option group filtering: each
COMMAND lists "Options" plus "Bus introspection" and/or "Runtime scope".
Unsupported options are now rejected by the parser as unknown options
(and are not shown in --help), replacing the runtime "This service does
not support …" errors.
The help output changes slightly: the "Bus introspection" and "Runtime
scope" option groups now get their own headings, and some descriptions
were reworded to the imperative form used elsewhere.
This time, various commands with non-trivial requirements are handled.
So the series is shorter, but has a pattern of feature additions and
then those new features being used. The way that --help output is
formatted is changed a bit in various places. Those changes are
described in individual commits.
22 'static int help()' functions left after this.
We already have io.systemd.service.SetLogLevel. Add a getter so that
CLI tools with `log-level` subcommands can be more easily implemented
with varlink.
Bind this method to all services that already bind io.systemd.service.SetLogLevel.
Serving stale cache data on a retry is a resilience feature for unicast
DNS: when the configured server does not respond, an expired answer beats
no answer. The gate in dns_transaction_go() that switches the cache lookup
into serve-stale mode from the second attempt on only checks
StaleRetentionSec=, not the scope's protocol, so it applies to mDNS and
LLMNR scopes as well. There it is wrong twice over:
* With StaleRetentionSec= set, a query for a peer that has left the
network misses the cache on the first attempt (the record's TTL has
expired), gets no reply, and is then answered from the expired but
retained record on the retry — for as long as the retention window
lasts. On the link-local protocols a retry means that no peer
answered, not that some server is unreachable, and RFC 6762 treats
TTL expiry as a presence signal: a host whose records lapse is gone.
* Independently of whether anything stale is cached, a cache hit made
in serve-stale mode is reported with its TTL clamped to 30s (see
dns_cache_lookup()) and counted in the served-stale statistics. mDNS
transactions are retried at 1s, 2s, 4s, ... whenever nobody answers,
so a record announced or learnt from another transaction in between
is handed to the client with a 30s TTL instead of its real remaining
TTL, and logged as a "Serve Stale response" although it never was
stale.
Hence only enter serve-stale mode on unicast DNS scopes. This is the
lookup-side counterpart of the cache-side change that stops link-local
records from being retained past their TTL at all.
In practice I think this will ever only be used with ansi_highlight_red(),
but it's cleaner to add this generic extension rather than e.g. a boolean
highlight_footer_red.
The options is passed through via command_print_help_full(), not through
the COMMAND structure, which means that commands with 'help' verb are not
covered by this. The red footer highligting is used for legacy commands,
which don't have verbs, so I think this is OK. Making it generic and
covering 'help' verbs would require more refactoring, and I think it
doesn't make sense to add it until it's actually needed.
Almost all programs are simple and have only a single command. When
printing options, let's just use the first COMMAND block found by default.
This means we can drop the explicit program-name argument and reduce
copy&paste boilerplate.
StaleRetentionSec= is a resilience feature for unicast DNS: when the
configured servers are unreachable, answering from an expired cache
entry beats answering nothing at all. That rationale does not carry
over to the link-local protocols, where there is no configured server
that could be unreachable -- peers simply come and go, and a record's
TTL is the only thing bounding how long a departed peer is still
answered for.
For mDNS the mismatch goes further. RFC 6762 treats record expiry as a
presence signal -- a host or service whose records lapse is to be
considered gone (section 10.1 goodbye packets even force this early by
publishing TTL=0), and the section 5.2 cache maintenance scheme exists
precisely so that expiry happens on time. Retaining expired mDNS
records for StaleRetentionSec= contradicts that: vanished peers linger
in the cache for the whole retention window, are served to clients as
if still present, and DNS-SD browsers do not see their services
disappear when the records' TTL lapses.
Beyond lingering entries, retention also swallows goodbyes outright, as
noted in review of the goodbye re-arm fix [1]: dns_cache_prune() and
dns_cache_expiry_in_one_second() key on the retention-extended "until",
so with StaleRetentionSec= set a goodbye's TTL=1 rewrite lands a full
retention window out -- the one-second prune of RFC 6762 section 10.1
then drops nothing, nothing re-arms, and the withdrawn service stays
listed for the whole window.
LLMNR has no goodbye mechanism, so only the lingering half applies to
it, but it does apply: dns_transaction_cache_answer() passes the
retention through for LLMNR replies just like for unicast DNS, and the
stale-serving gate in the transaction code does not look at the
protocol, so a departed LLMNR peer keeps being answered from cache for
the whole window as well.
Zero the retention centrally in dns_cache_put() for everything but
unicast DNS, which covers both put paths: transaction replies and the
mDNS packets cached directly by on_mdns_packet(). Add tests pinning
both directions, including the goodbye rewrite of an existing mDNS
entry.
[1] https://github.com/systemd/systemd/pull/43259#discussion_r3809488076
Add varlink version of FlushCaches.
Add this to io.systemd.Resolve.Monitor, since that interface already
handles many cache and manager related things.
The 'changed' variable is either unused on error or unconditionally
overwritten on success, so coverity warns about it. Drop the unused
assignment.
CID#1685266
Follow-up for a572dd2f60
BrowseServices(ifindex=0) may observe the same service on multiple
links. Treat the link index as part of the discovered service
identity so add and remove events remain link-scoped.
Use the per-service ifindex for RFC 6762 maintenance queries.
Share the service identity predicate between add and remove handling,
and propagate RR comparison errors instead of reporting removals.
Extend coverage for scoped-browser ifindex fallback, goodbye TTL
handling, and monotonic expiry updates.
So far there's only one multi-line abstract. But as pointed out in the
review, it's silly to handle footer but not the abstract.
help_abstract() is not reused because it only works with one-line
abstracts. (It always prints an empty line to separate paragraphs.)
So the formatting is done anew in print_wrapped(). I think this is OK,
since it's all very simple. After everything has been converted to
COMMAND() and its helpers, help_abstract() and the sister functions will
become unused and can be dropped then.
The footer is now wrapped to min($COLUMNS, 80) when it is printed, so
CommandDescription footers do not need to be hard-wrapped in the source,
and make use of the available terminal width. Newlines embedded in the
text are preserved and each line is wrapped independently, so explicit
line breaks can still be used where needed, e.g. before the example
command line in varlinkctl.
The text is wrapped at max of 80 columns to avoid very wrong lines on
wide terminals. Such lines are hard to read.
A new helper is added for this. We already have format_strv_width in
format-table, but it requires a strv, so we'd need to conver the data,
and returns a formatted string, which we'd need to print ourselves. So
overall, it seems better to add a dedicated helper.
Before this series, many help() functions opened the pager. This got lost
in the translation. Starting the pager requires access to arg_pager_flags.
While we could pipe this through in case when command_print_help() is
called directly, but this hard in the case when verb_help_auto() or
VERB_COMMON_HELP_AUTO/VERB_COMMON_HELP_AUTO_HIDDEN are used. Let's instead
extend the (const static) CommandDescription structure with a pointer to
the arg_pager_flags variable. This restores the pager functionality in a
uniform fashion with a one-line change in the individual binaries.
Previously, the systemd-resolve compat alias was described in the same
CommandDescription structure and resolvectl. But they have different
option sets, so this doesn't really work. The alias is deprecated, so
describe it as such in the abstract, but give it a separate entry.
$ build/resolvectl --introspect-cli
{
"mediaType": "application/vnd.io.systemd.cli-introspection-0",
"commands": [
{
"names": [ "resolvconf" ],
"abstract": [
"Register DNS server and domain configuration with systemd-resolved."
],
...
},
{
"names": [ "resolvectl" ],
"abstract": [
"Send control commands to the network name resolution manager, or\nresolve domain names, IPv4 and IPv6 addresses, DNS records, and services."
],
...
},
{
"names": [ "systemd-resolve" ],
"abstract": [
"Send control commands to the network name resolution manager, or\nresolve domain names, IPv4 and IPv6 addresses, DNS records, and services.\n\nThis command is deprecated. Use resolvectl.1 instead."
],
...
}
]
}
The resolvconf compatibility parser ignored errors from
nameserver, domain, and search lines. If one of those lines was
malformed, resolvectl could still send the partially parsed
configuration to resolved and overwrite the previous link state.
Reproducer:
IF=enp1s0
TMP="$(mktemp -d)"
ln -s "$(command -v resolvectl)" "$TMP/resolvconf"
sudo resolvectl dns "$IF" 1.1.1.1
sudo resolvectl domain "$IF" keep.example
resolvectl domain "$IF"
printf 'nameserver 1.1.1.1\nsearch "unterminated\n' |
sudo "$TMP/resolvconf" -a "$IF" || true
resolvectl domain "$IF"
Before:
The malformed search line was ignored and the previous domain
was cleared:
Link 2 (enp1s0): keep.example
Link 2 (enp1s0):
After:
The malformed input fails before updating resolved, so
keep.example remains configured.
Follow-up for: 097f130731
Yu Watanabe
Christian Glombek
Frantisek Sumsal
Armaan Sandhu
Stefan Agner
Zbigniew Jędrzejewski-Szmek
felix h
Nick Rosbrook
dongshengyuan
Luca Boccassi