I think quoting is more useful than not quoting. Without, arguments with
whitespace cannot be split correctly.
Unlike in coredump, "normal" quoting is used in those two cases. This output is
mostly for informational purposes, so the more readable quoting seems apropriate.
Before we wouldn't use any quoting, making it impossible to figure how the
command line was split into arguments. We could use "normal" quotes, but this
has the disadvantage that the commandline *looks* like it could be pasted into
the terminal and executed, but this is not true: various non-printable
characters cannot be expressed in this quoting style. (This is not visible in
this example). Thus, "POSIX quotes" are used, which should allow any command
line to be expressed acurrately and pasted directly into a shell prompt to
reexecute.
I wonder if we should another field in the coredump entry that simply shows the
original cmdline with embedded NULs, in the original /proc/*/cmdline
format. This would allow clients to format the data as they see fit. But I
think we'd want to keep the serialized form anyway, for backwards compatibility.
basic/process-util: add mode where posix shell escape is used for quoting
The new flag is not used, except in tests, so no functional change yet.
This way, the command as shown can be copied-and-pasted into the shell
in more cases. For simple cases, shell quoting with "" is enough. But
$'' is needed when there are control characters in the command.
Significant time was spent in the getpid() measurement code, which is not very
important. So let's optimize this a bit by running the slower version less
times, and only running both tests a lesser amount of times unless slow tests
are enabled.
This gives the better accuracy then before in slow mode, and still reasonable
accuracy in fast mode without a noticable slowdown.
test-process-util: add more debug logging but hide most of it by default
It makes little sense to always print the stuff that is fully deterministic
and verified by asserts. It can be opted-in with $SYSTEMD_LOG_LEVEL when
developing the tests or debugging a failure.
Since the new functionality is controlled by an option, this causes no change
in output yet, except tests.
The login in the old branch of !(flags & PROCESS_CMDLINE_QUOTE) is essentially
unmodified. But there is an important difference in behaviour: instead of
unconditionally reading the whole virtual file, we now read only 'max_columns'
bytes. This makes out code to write process lists quite a bit more efficient
when there are processes with long command lines.
basic/escape: allow truncation mode where "…" is always appended
So far we would append "…" or "..." when the string was wider than the specified
output width. But let's add a mode where the caller knows that the string being
passed is already truncated.
The condition for jumping back in utf8_escape_non_printable_full() was
off-by-one. But we only jumped to that label after doing a check with a
stronger condition, so I think it didn't matter. Now it matters because we'd
output the forced ellipsis one column too early.
basic/escape: escape control characters, but not utf-8, in shell quoting
The comment in the code said that so far this didn't matter, but I want to use
shell quoting in more places where this will make a difference. So control
characters are now escaped. Normal utf-8 characters are passed through, it
is 2021 after all and pretty much everyone is (or should be) using utf-8.
While touching the code, change 'char *r' → 'char *buf', in line with modern
style.
basic/escape: always escape newlines in shell_escape()
shell_escape() is mostly used for mount paths and similar, where we assume
no newlines are present in the string. But if any were ever present, we
should escape them. So let's simplify the code by making this unconditional.
Flagsify EscapeStyle and make ESCAPE_BACKSLASH_ONELINE implicit
I want to tweak behaviour further, and that'll be easier when "style"
is converted to a bitfield.
Some callers used ESCAPE_BACKSLASH_ONELINE, and others not. But the
ones that didn't, simply didn't care, because the argument was assumed to
be one-line anyway (e.g. a service name). In environment-generator, this
could make a difference. But I think it's better to escape the newlines
there too. So newlines are now always escaped, to simplify the code and
the test matrix.
basic/fileio: fix reading of not-too-small virtual files
This code is trying to do two things: when reading a file with working
st.st_size, detect when the file size changes between the fstat() and our
allocation of the buffer based on the returned size, and the subsequent read().
When reading a file without st.st_size, read up to READ_FULL_BYTES_MAX.
But this second scenario was partially broken: we'd start with size = 4095, and
double the size up to three times, i.e. up to 32767. But we want to read up to
READ_FULL_BYTES_MAX.
So let's listentangle the two cases a bit: if a file returns non-zero st._size,
proceed as before. But if we don't know the size, let's immediately allocate
the buffer of maximum size of READ_FULL_BYTES_MAX. I think that allocating 4MB
and 1MB is going to take pretty much the same time as long as the memory is not
written to, so by allocating 1MB, 2MB, and 4MB, we wouldn't really be saving
anything internally, but wasting time on repeated reads, if the file is long
enough.
Also, don't do the seek if we know we're going to return an error immediately
after.
This should fix reading of any files in /proc, which all have size == 0. In
particular, various files read by coredump might be larger than 32767.
What about /sys? The file there return a fake value, usually 4096. So we'll
allocate a small buffer and read that.
Anita Zhang [Fri, 26 Mar 2021 10:01:38 +0000 (03:01 -0700)]
oomd: make it more clear when a kill happens
Improve the logging to only print if systemd-oomd killed something. And
also print which cgroup was targeted.
Demote general swap above/pressure above messages to debug.
№1 0x00007fb5f13ae87f in selabel_lookup_raw (rec=<optimized out>, con=con@entry=0x7fffef307380, key=key@entry=0x55f616ac4750 "/run/user/1000/systemd/units/invocation:systemd-tmpfiles-clean.service", type=type@entry=40960) at label.c:256
lr = <optimized out>
'rec' is passed through as is to selabel_lookup_common().
№2 0x00007fb5f1561b2d in selinux_create_file_prepare_abspath (abspath=0x55f616ac4750 "/run/user/1000/systemd/units/invocation:systemd-tmpfiles-clean.service", mode=40960) at ../src/basic/selinux-util.c:368
filecon = 0x0
r = <optimized out>
__PRETTY_FUNCTION__ = "selinux_create_file_prepare_abspath"
__func__ = "selinux_create_file_prepare_abspath"
№3 0x00007fb5f1561ec3 in mac_selinux_create_file_prepare (path=<optimized out>, mode=40960) at ../src/basic/selinux-util.c:431
r = 0
abspath = 0x55f616ac4750 "/run/user/1000/systemd/units/invocation:systemd-tmpfiles-clean.service"
__PRETTY_FUNCTION__ = "mac_selinux_create_file_prepare"
We checked label_hnd != NULL, but then we apparently called
avc_netlink_check_nb(), which reset label_hnd. Yay for global state!
№4 0x00007fb5f1549950 in symlink_atomic_label (from=0x55f6169d8b50 "69a8dcf7a7ac46b29306f2fddbed3edc", to=0x55f616ab8380 "/run/user/1000/systemd/units/invocation:systemd-tmpfiles-clean.service") at ../src/basic/label.c:55
r = <optimized out>
__PRETTY_FUNCTION__ = "symlink_atomic_label"
In the logs:
Mar 29 14:48:44 fedorapad.home systemd[1974]: selinux: avc: received policyload notice (seqno=2)
Mar 29 14:48:44 fedorapad.home systemd[1974]: Failed to initialize SELinux labeling handle: No such file or directory
Mar 29 14:48:44 fedorapad.home systemd[1974]: selinux: avc: received policyload notice (seqno=3)
Mar 29 14:48:44 fedorapad.home systemd[1974]: selinux: avc: received setenforce notice (enforcing=0)
Christian Hesse [Sun, 28 Mar 2021 11:00:49 +0000 (13:00 +0200)]
units: make locale directory writable for systemd-localed
With 8f20232fcb52dbe6255f3df6101fc057af90bcfa systemd-localed supports
generating locales when required. This fails if the locale directory is
read-only, so make it writable.
Fangrui Song [Mon, 29 Mar 2021 06:35:06 +0000 (23:35 -0700)]
sd-bus: set retain attribute on BUS_ERROR_MAP_ELF_REGISTER
LLD 13 and GNU ld 2.37 support -z start-stop-gc which allows garbage
collection of C identifier name sections despite the __start_/__stop_
references. Simply set the retain attribute so that GCC 11 (if
configure-time binutils is 2.36 or newer)/Clang 13 will set the
SHF_GNU_RETAIN section attribute to prevent garbage collection.
Without the patch, there are linker errors like the following with -z
start-stop-gc.
```
ld: error: undefined symbol: __start_SYSTEMD_BUS_ERROR_MAP
>>> referenced by bus-error.c:93 (../src/libsystemd/sd-bus/bus-error.c:93)
>>> sd-bus_bus-error.c.o:(bus_error_name_to_errno) in archive src/libsystemd/libsystemd_static.a
```
But it seems that we don't want those calls at all. The test was originally
added with the call in a6ee01caf3409ba9820e8824b9262fbac31a9f77, but I don't
see why we should override this. If the user wants to execute the test with
mempool disabled, we shouldn't ignore that.
basic/fileio: optimize buffer sizes in read_full_virtual_file()
We'd proceed rather inefficiently: the initial buffer size was LINE_MAX/2,
i.e. only 1k. We can read 4k at the same cost.
Also, we'd try to allocate 1025, 2049, 4097 bytes, i.e. always one higher than
the power-of-two size. Effectively the allocation would be bigger, and we'd
waste the additional space. So let's allocate aligned to the power-of-two size.
size=4095, 8191, 16383, so we allocate 4k, 8k, 16k.
basic/fileio: simplify calculation of buffer size in read_full_virtual_file()
We'd first assign a value up to SSIZE_MAX, and then immediately check if we
have a value bigger than READ_FULL_BYTES_MAX. This wasn't exactly wrong, but a
bit roundabout. Let's immediately assign the value from the appropriate range
or error out.
As noted in https://github.com/systemd/systemd/pull/17535#discussion_r534129256,
"raw" is misleading in this context. Let's use a more descriptive term.
This adds the same line to most of our .conf files.
Not for systemd/user.conf though, since we can't correctly display it right
now:
$ systemd-analyze cat-config --user systemd/user.conf
Option --user is not supported for cat-config right now.
For sysusers.d, tmpfiles.d, rules.d, etc, there is no single file. Maybe
we should short READMEs in /usr/lib/sysusers.d, /usr/lib/tmpfiles.d, etc.?
When following CNAME/DNAME redirects in the stub we currently first
iterate through the packet and pick up what we can use (in
dns_stub_collect_answer_by_question() and friends), following all
CNAMEs/DNAMEs, and would then issue dns_query_process_cname() to move
the DnsQuery object forward too, where we'd then possibly restart
the query and pick things up again, as above.
There's one thought error in this though: dns_query_process_cname()
tries to be smart and will internally follow not just a single
CNAME/DNAME redirect, but a chain of them if they are contained inside
the same packet until we reach the point where the answer is not
included in the packet anymore, where we'd restart the query. This was
great as long as we only focussed on the D-Bus and Varlink resolver
APIs, since there the CNAME/DNAME chain in the middle doesn't actually
matter, we just return information about the final name of the RR and
its content, and aren't interested in the chain to it. For the DNS stub
this is different however: there we need to place the full CNAME/DNAME
chain (and all the appropriate metadata RRs) in the stub reply.
Hence rework this so that we build on the fact that the previous commit
split dns_query_process_cname() in two:
1. dns_query_process_cname_one() will do exactly one CNAME/DNAME
redirect step. This will be called by the stub, so that we can pick
up matching RRs for every single step along the way.
2. dns_query_process_cname_many() will follow a chain as long as that's
possible within the same packet. It's thus pretty much identical to
the old dns_query_process_cname() call. This is what we now use in
the D-Bus and Varlink APIs. dns_query_process_cname_many() is
basically just a loop around dns_query_process_cname_one().
Any logic to follow and pick up RRs manually in the stub along the
CNAME/DNAME path is now dropped (i.e.
dns_stub_collect_answer_by_question() becomes trivially simple again),
we solely rely on dns_query_process_cname_one() to follow CNAME/DNAME
now: each step followed by a full call of dns_stub_assign_sections() to
copy out the RRs that matter.
Net result: things are a bit simpler again, as the only place we follow
CNAME/DNAME redirects is DnsQuery again, and stub answers are always
complete: they contain all CNAME/DNAME RRs on the way including all
their metadata we might pick up in the other sections.
resolved: split dns_query_process_cname() into two separate functions
This does some refactoring: the dns_query_process_cname() function
becomes two: dns_query_process_cname_one() and
dns_query_process_cname_many(). The former will process exactly one
CNAME chain element, the latter will follow a chain for as long as
possible within the current packet.
dns_query_process_cname_many() is mostly identical to the old
dns_query_process_cname(), and all existing code is moved over to using
that.
This is mostly preparation for the next commit, where we make direct use
of dns_query_process_cname_one().
This also renames the DNS_QUERY_RESTARTED return value to
DNS_QUERY_CNAME. That's because in the dns_query_process_cname_many()
case as before if we return this we restarted the query in case we
reached the end of the chain without a conclusive answer, as before. But
in dns_query_process_cname_one() we'll only go one step anyway, and
leave restarting if needed to the caller. Hence DNS_QUERY_RESTARTED is a
bit of a misnomer in that case.
This also gets rid of the weird tail recursion in
dns_query_process_cname() and replaces it with an explicit loop in
dns_query_process_cname_many(). The old recursion wasn't a security
issue since we put a limit on the number of CNAMEs we follow anyway, but
it's still icky to scale stack use by that.
Luca Boccassi [Thu, 25 Mar 2021 11:47:13 +0000 (11:47 +0000)]
test-dhcp6-client: add one more assert on memory mapping
Static analyzers need a hint that optval is not pointing
off the end of the msg_advertise array, since pos can go
up to the full length of it. The array is manually
constructed so we know this won't happen, but adding one
more assert should be enough to avoid false positives.
Previously we'd stick all answer sections RRs we acquired into
the authoritative section if we didn't find them directly answering our
question. Let's put them into additional instead. The authoritative
section should hence only include what comes from the upstream
authoritative section, and nothing else.
resolved: pass mDNS reply packets to each transaction exactly once
Previously we'd iterate through the RRs of an mDNS reply and then find
exactly one matching transaction on our scope for it, and pass it as
reply to that. If multiple RRs of the same packet match we'd pas the
packet multiple times to the transaction even.
This all doesn't really work anymore since there can be multiple open
transactions for the same key (with different flags), and it's kinda
ugly anywy. Hence let's turn this around: let's iterate through the
transactions and check if any of the included RRs match it, and if so
pass the packet to that transaction exactly once.
This speeds up mDNS a bit, since previously we'd oftentimes fail to find
all suitable transactions for an mDNS reply (because there can be
multiple transactions for the same RR key with different flags, and we
checked exactly one flag combination). Which would then mean the
transaction would time out, and be retried – at which point the cache
would be populated and thus it would still succeed, but only after this
timeout. With this fix this is corrected: every transaction that matches
will get the reply, instantly as we get it.
resolved: upgrade log level to LOG_NOTICE if we switch to fallback server (or back)
This is inspired by a recent thread on fedora-devel: it's noteworthy
when we switch to the fallback servers, since it might (or might not)
indicate some configuration problem.
David Tardon [Wed, 24 Mar 2021 13:45:02 +0000 (14:45 +0100)]
local-addresses: fix use of uninitialized value
This can happen if ifi fails to be read from the netlink message and the
error is ENODATA.
Fixes the following valgrind message when running netstat:
==164141== Conditional jump or move depends on uninitialised value(s)
==164141== at 0x524AE60: address_compare (local-addresses.c:29)
==164141== by 0x48BCC78: msort_with_tmp.part.0 (msort.c:105)
==164141== by 0x48BC9E4: msort_with_tmp (msort.c:45)
==164141== by 0x48BC9E4: msort_with_tmp.part.0 (msort.c:53)
==164141== by 0x48BCF85: msort_with_tmp (msort.c:45)
==164141== by 0x48BCF85: qsort_r (msort.c:297)
==164141== by 0x52500FC: UnknownInlinedFun (sort-util.h:47)
==164141== by 0x52500FC: local_gateways.constprop.0 (local-addresses.c:310)
==164141== by 0x5251C05: _nss_myhostname_gethostbyaddr2_r (nss-myhostname.c:456)
==164141== by 0x5252006: _nss_myhostname_gethostbyaddr_r (nss-myhostname.c:500)
==164141== by 0x498E7FE: gethostbyaddr_r@@GLIBC_2.2.5 (getXXbyYY_r.c:274)
==164141== by 0x498E560: gethostbyaddr (getXXbyYY.c:135)
==164141== by 0x121353: INET_rresolve.constprop.0 (inet.c:212)
==164141== by 0x1135B9: INET_sprint (inet.c:261)
==164141== by 0x121BFC: addr_do_one.constprop.0.isra.0 (netstat.c:1156)
Anita Zhang [Tue, 23 Mar 2021 07:49:28 +0000 (00:49 -0700)]
process-util: dont allocate max length to read /proc/PID/cmdline
Alternative title: Replace get_process_cmdline()'s fopen()/fread() with
read_full_virtual_file().
When RLIMIT_STACK is set to infinity:infinity, _SC_ARG_MAX will
return 4611686018427387903 (depending on the system, but definitely
something larger than most systems have). It's impractical to allocate this
in one go when most cmdlines are much shorter than that.
Instead use read_full_virtual_file() which seems to increase the buffer
depending on the size of the contents.
Unfortunately the new syscall causes test-event to hang. 32 bit architectures
seem affected: i686 and arm32 in fedora koji. 32 bit build of test-event hangs
reliably under valgrind:
If I set epoll_pwait2_absent=true, so the new function is never called, then
the issue does not reproduce. It seems to be strictly tied to the syscall.
Yu Watanabe [Tue, 23 Mar 2021 05:38:18 +0000 (14:38 +0900)]
firewall-util: probe firewall backend in fw_ctx_new()
FirewallContext is used by networkd and nspawn. Both allocates the
context when it is really necessary. Hence, it is not necessary to delay
probing backend.
Moreover, if iptables backend is not enabled on build, and nftables is
not supported by kernel, previously `fw_nftables_init()` is called
everytime when we try to configure masquerade or dnat. It causes
significant performance loss.
We rely on mktime() normalizing the time. The man page does not say that it'll
move the time forward, but our algorithm relies on this. So let's catch this
case explicitly.
With this patch:
$ TZ=Europe/Dublin faketime 2021-03-21 build/systemd-analyze calendar --iterations=5 'Sun *-*-* 01:00:00'
Normalized form: Sun *-*-* 01:00:00
Next elapse: Sun 2021-03-21 01:00:00 GMT
(in UTC): Sun 2021-03-21 01:00:00 UTC
From now: 59min left
Iter. #2: Sun 2021-04-04 01:00:00 IST
(in UTC): Sun 2021-04-04 00:00:00 UTC
From now: 1 weeks 6 days left <---- note the 2 week jump here
Iter. #3: Sun 2021-04-11 01:00:00 IST
(in UTC): Sun 2021-04-11 00:00:00 UTC
From now: 2 weeks 6 days left
Iter. #4: Sun 2021-04-18 01:00:00 IST
(in UTC): Sun 2021-04-18 00:00:00 UTC
From now: 3 weeks 6 days left
Iter. #5: Sun 2021-04-25 01:00:00 IST
(in UTC): Sun 2021-04-25 00:00:00 UTC
From now: 1 months 4 days left
The syntax like "0666" is very unclear. It only makes sense for some subset of
people who do C programming. Let's use the much more sensible modern python
syntax instead.
projects / thirdparty / systemd.git / log
