We document the rule that return values >= 0 of functions are supposed
to indicate success, and that in case of success all return parameters
should be initialized. Let's actually do so.
Just a tiny coding style fix-up.
Jun 11 14:29:12 krowka systemd[1]: /etc/systemd/system/workingdir.service:6: = path is not normalizedWorkingDirectory: /../../etc
↓
Jun 11 14:32:12 krowka systemd[1]: /etc/systemd/system/workingdir.service:6: WorkingDirectory= path is not normalized: /../../etc
Since bb28e68477 parsing failures of
certain unit file settings will result in load failures of units. This
introduces a new load state "bad-setting" that is entered in precisely
this case.
With this addition error messages on bad settings should be a lot more
explicit, as we don't have to show some generic "errno" error in that
case, but can explicitly say that a bad setting is at fault.
Internally this unit load state is entered as soon as any configuration
loader call returns ENOEXEC. Hence: config parser calls should return
ENOEXEC now for such essential unit file settings. Turns out, they
generally already do.
Fixes: #9107
oss-fuzz flags this as:
==1==WARNING: MemorySanitizer: use-of-uninitialized-value
0. 0x7fce77519ca5 in ascii_is_valid systemd/src/basic/utf8.c:252:9
1. 0x7fce774d203c in ellipsize_mem systemd/src/basic/string-util.c:544:13
2. 0x7fce7730a299 in print_multiline systemd/src/shared/logs-show.c:244:37
3. 0x7fce772ffdf3 in output_short systemd/src/shared/logs-show.c:495:25
4. 0x7fce772f5a27 in show_journal_entry systemd/src/shared/logs-show.c:1077:15
5. 0x7fce772f66ad in show_journal systemd/src/shared/logs-show.c:1164:29
6. 0x4a2fa0 in LLVMFuzzerTestOneInput systemd/src/fuzz/fuzz-journal-remote.c:64:21
...
I didn't reproduce the issue, but this looks like an obvious error: the length
is specified, so we shouldn't use the string with any functions for normal
C-strings.
This is mostly fall-out from d1a1f0aaf0,
however some cases are older bugs.
There might be more issues lurking, this was a simple grep for "%m"
across the tree, with all lines removed that mention "errno" at all.
We do this checks as protection against bind mount cycles on the same
file system. However, the check wasn't really effective for that, as
it would only detect cycles A → B → A this way. By using
fs_is_mount_point() we'll also detect cycles A → A.
Also, while we are at it, make these file system boundary checks
optional. This is not used anywhere, but might be eventually...
Most importantly though add a longer blurb explanation the why.
This fixes the copy routines on overlay filesystem, which typically
returns the underlying st_dev for files, symlinks, etc.
The value of st_dev is guaranteed to be the same for directories, so
checking it on directories only fixes this code on overlay filesystem
and still keeps it from traversing mount points (which was the original
intent.)
There's a small side effect here, by which regular (non-directory) files
with bind mounts will be copied by the new logic (while they were
skipped by the previous logic.)
Tested: ./build/test-copy with an overlay on /tmp.
Fixes: #9134
This way we don't need to repeat the argument twice.
I didn't replace all instances. I think it's better to leave out:
- asserts
- comparisons like x & y == x, which are mathematically equivalent, but
here we aren't checking if flags are set, but if the argument fits in the
flags.
The function is similar to path_kill_slashes() but also removes
initial './', trailing '/.', and '/./' in the path.
When the second argument of path_simplify() is false, then it
behaves as the same as path_kill_slashes(). Hence, this also
replaces path_kill_slashes() with path_simplify().
First, ellipsize() and ellipsize_mem() should not read past the input
buffer. Those functions take an explicit length for the input data, so they
should not assume that the buffer is terminated by a nul.
Second, ellipsization was off in various cases where wide on multi-byte
characters were used.
We had some basic test for ellipsize(), but apparently it wasn't enough to
catch more serious cases.
Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=8686.
* it fails with gcc8 when -O1 or -Os is used (and -ftree-vrp which is added by -O2 and higher isn't used)
../git/src/basic/time-util.c: In function 'format_timespan':
../git/src/basic/time-util.c:508:46: error: '%0*llu' directive output between 1 and 2147483647 bytes may cause result to exceed 'INT_MAX' [-Werror=format-truncation=]
"%s"USEC_FMT".%0*"PRI_USEC"%s",
^~~~
../git/src/basic/time-util.c:508:60: note: format string is defined here
"%s"USEC_FMT".%0*"PRI_USEC"%s",
../git/src/basic/time-util.c:508:46: note: directive argument in the range [0, 18446744073709551614]
"%s"USEC_FMT".%0*"PRI_USEC"%s",
^~~~
../git/src/basic/time-util.c:507:37: note: 'snprintf' output 4 or more bytes (assuming 2147483651) into a destination of size 4294967295
k = snprintf(p, l,
^~~~~~~~~~~~~~
"%s"USEC_FMT".%0*"PRI_USEC"%s",
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
p > buf ? " " : "",
~~~~~~~~~~~~~~~~~~~
a,
~~
j,
~~
b,
~~
table[i].suffix);
~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
[zj: change 'char' to 'signed char']
sparc sets the carry bit when a syscall fails. Use this information to
set errno and return -1 as appropriate.
The added test case calls raw_clone() with flags known to be invalid
according to the clone(2) manpage.
We already do that in get_process_cmdline(), which is very similar in
behaviour otherwise. Hence, let's be safe and also filter them in
get_process_comm(). Let's try to retain as much information as we can
though and escape rather than suppress unprintable characters. Let's not
increase comm names beyond the kernel limit on such names however.
Also see discussion about this here:
https://marc.info/?l=linux-api&m=152649570404881&w=2
For short buffer sizes cellescape() was a bit wasteful, as it might
suffice to to drop a single character to find enough place for the full
four byte ellipsis, if that one character was a four character escape.
With this rework we'll guarantee to drop the minimum number of
characters from the end to fit in the ellipsis.
If the buffers we write to are large this doesn't matter much. However,
if they are short (as they are when talking about the process comm
field) then it starts to matter that we put as much information as we
can in the space we get.
The boot id is stored twice, and different code paths use either one or the
other. So we need to store it both in the header and as a field for full
compatibility.
If the timestamp is above 9999-12-30, (or 2038-something-something on 32 bit),
use XXXX-XX-XX XX:XX:XX as the replacement.
The problem with refusing to print timestamps is that our code accepts such
timestamps, so we can't really just refuse to process them afterwards. Also, it
makes journal files non-portable, because suddently we might completely refuse
to print entries which are totally OK on a different machine.
The parser never accepted "__"-prefixed fields in binary format, but there was
a comment questioning this decision. Let's make it official, and remove the
comment.
Also, for clarity, let's move the dunder field parsing after the field
verification check. This doesn't change much, because invalid fields cannot be
known special fields, but is seems cleaner to first verify the validity of the
name, and then check if it is one of the known ones.
$ build-asan/fuzz-journal-remote test/fuzz-regressions/fuzz-journal-remote/crash-96dee870ea66d03e89ac321eee28ea63a9b9aa45
...
Ignoring invalid field: "S\020"
Ignoring invalid field: "S\020"
...
If the field name includes nul bytes, we won't print all of the name.
But that seems enough of a corner case to ignore.
`fuzz-journal-remote` seems to be failing under `msan` as soon as it starts:
$ sudo infra/helper.py run_fuzzer systemd fuzz-journal-remote
Running: docker run --rm -i --privileged -e FUZZING_ENGINE=libfuzzer -v /home/vagrant/oss-fuzz/build/out/systemd:/out -t gcr.io/oss-fuzz-base/base-runner run_fuzzer fuzz-journal-remote
Using seed corpus: fuzz-journal-remote_seed_corpus.zip
/out/fuzz-journal-remote -rss_limit_mb=2048 -timeout=25 /tmp/fuzz-journal-remote_corpus -max_len=65536 < /dev/null
INFO: Seed: 3380449479
INFO: Loaded 2 modules (36336 inline 8-bit counters): 36139 [0x7ff36ea31d39, 0x7ff36ea3aa64), 197 [0x9998c8, 0x99998d),
INFO: Loaded 2 PC tables (36336 PCs): 36139 [0x7ff36ea3aa68,0x7ff36eac7d18), 197 [0x999990,0x99a5e0),
INFO: 2 files found in /tmp/fuzz-journal-remote_corpus
INFO: seed corpus: files: 2 min: 4657b max: 7790b total: 12447b rss: 97Mb
Uninitialized bytes in __interceptor_pwrite64 at offset 24 inside [0x7fffdd4d7230, 240)
==15==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x7ff36e685e8a in journal_file_init_header /work/build/../../src/systemd/src/journal/journal-file.c:436:13
#1 0x7ff36e683a9d in journal_file_open /work/build/../../src/systemd/src/journal/journal-file.c:3333:21
#2 0x7ff36e68b8f6 in journal_file_open_reliably /work/build/../../src/systemd/src/journal/journal-file.c:3520:13
#3 0x4a3f35 in open_output /work/build/../../src/systemd/src/journal-remote/journal-remote.c:70:13
#4 0x4a34d0 in journal_remote_get_writer /work/build/../../src/systemd/src/journal-remote/journal-remote.c:136:21
#5 0x4a550f in get_source_for_fd /work/build/../../src/systemd/src/journal-remote/journal-remote.c:183:13
#6 0x4a46bd in journal_remote_add_source /work/build/../../src/systemd/src/journal-remote/journal-remote.c:235:13
#7 0x4a271c in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journal-remote.c:36:9
#8 0x4f27cc in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:524:13
#9 0x4efa0b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:448:3
#10 0x4f8e96 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:732:7
#11 0x4f9f73 in fuzzer::Fuzzer::Loop(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:752:3
#12 0x4bf329 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:756:6
#13 0x4ac391 in main /src/libfuzzer/FuzzerMain.cpp:20:10
#14 0x7ff36d14982f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
#15 0x41f9d8 in _start (/out/fuzz-journal-remote+0x41f9d8)
Uninitialized value was stored to memory at
#0 0x7ff36e61cd41 in sd_id128_randomize /work/build/../../src/systemd/src/libsystemd/sd-id128/sd-id128.c:288:16
#1 0x7ff36e685cec in journal_file_init_header /work/build/../../src/systemd/src/journal/journal-file.c:426:13
#2 0x7ff36e683a9d in journal_file_open /work/build/../../src/systemd/src/journal/journal-file.c:3333:21
#3 0x7ff36e68b8f6 in journal_file_open_reliably /work/build/../../src/systemd/src/journal/journal-file.c:3520:13
#4 0x4a3f35 in open_output /work/build/../../src/systemd/src/journal-remote/journal-remote.c:70:13
#5 0x4a34d0 in journal_remote_get_writer /work/build/../../src/systemd/src/journal-remote/journal-remote.c:136:21
#6 0x4a550f in get_source_for_fd /work/build/../../src/systemd/src/journal-remote/journal-remote.c:183:13
#7 0x4a46bd in journal_remote_add_source /work/build/../../src/systemd/src/journal-remote/journal-remote.c:235:13
#8 0x4a271c in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journal-remote.c:36:9
#9 0x4f27cc in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:524:13
#10 0x4efa0b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:448:3
#11 0x4f8e96 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:732:7
#12 0x4f9f73 in fuzzer::Fuzzer::Loop(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:752:3
#13 0x4bf329 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:756:6
#14 0x4ac391 in main /src/libfuzzer/FuzzerMain.cpp:20:10
#15 0x7ff36d14982f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
Uninitialized value was created by an allocation of 't' in the stack frame of function 'sd_id128_randomize'
#0 0x7ff36e61cb00 in sd_id128_randomize /work/build/../../src/systemd/src/libsystemd/sd-id128/sd-id128.c:274
SUMMARY: MemorySanitizer: use-of-uninitialized-value /work/build/../../src/systemd/src/journal/journal-file.c:436:13 in journal_file_init_header
Exiting
MS: 0 ; base unit: 0000000000000000000000000000000000000000
artifact_prefix='./'; Test unit written to ./crash-847911777b3096783f4ee70a69ab6d28380c810b
[vagrant@localhost oss-fuzz]$ sudo infra/helper.py check_build --sanitizer=memory systemd
Running: docker run --rm -i --privileged -e FUZZING_ENGINE=libfuzzer -e SANITIZER=memory -v /home/vagrant/oss-fuzz/build/out/systemd:/out -t gcr.io/oss-fuzz-base/base-runner test_all
INFO: performing bad build checks for /out/fuzz-dhcp-server.
INFO: performing bad build checks for /out/fuzz-journal-remote.
INFO: performing bad build checks for /out/fuzz-unit-file.
INFO: performing bad build checks for /out/fuzz-dns-packet.
4 fuzzers total, 0 seem to be broken (0%).
Check build passed.
It's a false positive which is most likely caused by
https://github.com/google/sanitizers/issues/852. I think it could be got around
by avoiding `getrandom` when the code is compiled with `msan`
We shouldn't just log arbitrary stuff, in particular newlines and control chars
Now:
Unknown dunder line __CURSORFACILITY=6\nSYSLOG_IDENTIFIER=/USR/SBIN/CRON\nMES…, ignoring.
Unknown dunder line __REALTIME_TIME[TAMP=1404101101501874\n__MONOTONIC_TIMEST…, ignoring.
It's not supposed to be the most efficient, but instead fast and simple to use.
I kept the logic in ellipsize_mem() to use unicode ellipsis even in non-unicode
locales. I'm not quite convinced things should be this way, especially that with
this patch it'd actually be simpler to always use "…" in unicode locale and "..."
otherwise, but Lennart wanted it this way for some reason.
