Skip to content
This repository has been archived by the owner on Aug 6, 2020. It is now read-only.

Hardcode path references to /etc #17

Closed
wants to merge 1 commit into from
Closed

Conversation

aszlig
Copy link
Member

@aszlig aszlig commented Mar 3, 2018

So this all started with me wondering why the VirtualBox test didn't show up the log for the guest machines, which are logged via a service that just echoes all the lines into the journal.

In NixOS VM tests, the journald configuration contains a few extra lines:

ForwardToConsole=yes
MaxLevelConsole=debug

These lines are to ensure that outputs from services are directly shown in the test log.

At that time I wasn't sure whether this is a systemd-related issue, so I bisected this and the result was the systemd bump in NixOS/nixpkgs@eb862c4.

I've straced pid 1 and journald (unfortunately at a very late stage during bootup, otherwise I would have noticed) and couldn't find a clue about what's going on, so I bisected systemd itself (which takes ages,
as you constantly need to reapply and fix our patches).

Unfortunately, this didn't help either, so after some sleep (finally) I went through the changed Nix expression of the systemd package line by line, comparing it with the old expression file and I found that the old one had sysconfdir set to /etc but installed it with a sysconfdir set to $out/etc.

The reason why this was and is necessary is that while we certainly want to have the stock systemd config files in $out/etc, but we want them to be read from /etc at runtime.

Hence, the change here hardcodes /etc in all locations that are referenced by the code, because Meson doesn't have a way to change this during install (it's only possible to set DESTDIR, which changes the
root directory, not even the prefix).

After applying this, the VM tests finally show the outputs from units again.

So this all started with me wondering why the VirtualBox test didn't
show up the log for the guest machines, which are logged via a service
that just echoes all the lines into the journal.

In NixOS VM tests, the journald configuration contains a few extra
lines:

ForwardToConsole=yes
MaxLevelConsole=debug

These lines are to ensure that outputs from services are directly shown
in the test log.

At that time I wasn't sure whether this is a systemd-related issue, so I
bisected this and the result was the systemd bump in
NixOS/nixpkgs@eb862c4.

I've straced pid 1 and journald (unfortunately at a very late stage
during bootup, otherwise I would have noticed) and couldn't find a clue
about what's going on, so I bisected systemd itself (which takes ages,
as you constantly need to reapply and fix our patches).

Unfortunately, this didn't help either, so after some sleep (finally) I
went through the changed Nix expression of the systemd package line by
line, comparing it with the old expression file and I found that the old
one had sysconfdir set to /etc but *installed* it with a sysconfdir set
to $out/etc.

The reason why this was and is necessary is that while we certainly want
to have the stock systemd config files in $out/etc, but we want them to
be read from /etc at runtime.

Hence, the change here hardcodes /etc in all locations that are
referenced by the code, because Meson doesn't have a way to change this
during install (it's only possible to set DESTDIR, which changes the
*root* directory, not even the prefix).

After applying this, the VM tests finally show the outputs from units
again.

Signed-off-by: aszlig <aszlig@nix.build>
@Mic92
Copy link
Member

Mic92 commented Mar 3, 2018

this should be no longer necessary with: #15

@Mic92
Copy link
Member

Mic92 commented Mar 3, 2018

I use the following to compile systemd: Mic92/nixpkgs@35442b6#diff-dd830f049b8e42facd9261dc425529f1L90

@aszlig
Copy link
Member Author

aszlig commented Mar 3, 2018

Okay, this indeed fixes the problem and it's even more upstream-ready.

@aszlig aszlig closed this Mar 3, 2018
@aszlig aszlig deleted the etc-fix branch March 3, 2018 01:16
aszlig added a commit to NixOS/nixpkgs that referenced this pull request Mar 3, 2018
Updated to the latest version of the nixos-v237 branch, which fixes two
things:

  * Make sure that systemd looks in /etc for configuration files.
    NixOS/systemd#15
  * Fix handling of the x-initrd.mount option.
    NixOS/systemd#16

I've added NixOS VM tests for both to ensure we won't run into
regressions. The newly added systemd test only tests for that and is by
no means exhaustive, but it's a start.

Personally I only wanted to fix the former issue, because that's the one
I've been debugging. After sending in a pull request for our systemd
fork (NixOS/systemd#17) I got a notice from
@Mic92, that he already fixed this and his fix was even better as it's
even suitable for upstream (so we hopefully can drop that patch
someday).

The reason why the second one came in was simply because it has been
merged before the former, but I thought it would be a good idea to have
tests for that as well.

In addition I've removed the sysconfdir=$out/etc entry to make sure the
default (/etc) is used. Installing is still done to $out, because those
directories that were previously into sysconfdir now get into
factoryconfdir.

Quote from commit NixOS/systemd@98067cc:

  By default systemd should read all its configuration from /etc.
  Therefore we rely on -Dsysconfdir=/etc in meson as default value.
  Unfortunately this would also lead to installation of systemd's own
  configuration files to `/etc` whereas we are limited to /nix/store. To
  counter that this commit introduces two new configuration variables
  `factoryconfdir` and `factorypkgconfdir` to install systemd's own
  configuration into nix store again, while having executables looking
  up files in /etc.

Tested this change against all of the NixOS VM tests we have in
nixos/release.nix. Between this change and its parent no new tests were
failing (although a lot of them were flaky).

Signed-off-by: aszlig <aszlig@nix.build>
Cc: @Mic92, @tk-ecotelecom, @edolstra, @fpletz
Fixes: #35415
Fixes: #35268
fpletz pushed a commit that referenced this pull request Aug 3, 2018
Fuzzing with AddressSanitizer reports an error here:
==11==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7fe53f5497d8 at pc 0x7fe53ef055c9 bp 0x7ffd344e9380 sp 0x7ffd344e9378
READ of size 4 at 0x7fe53f5497d8 thread T0
SCARINESS: 27 (4-byte-read-global-buffer-overflow-far-from-bounds)
    #0 0x7fe53ef055c8 in bus_error_name_to_errno /work/build/../../src/systemd/src/libsystemd/sd-bus/bus-error.c:118:24
    #1 0x7fe53ef0577b in bus_error_setfv /work/build/../../src/systemd/src/libsystemd/sd-bus/bus-error.c:274:17
    #2 0x7fe53ef0595a in sd_bus_error_setf /work/build/../../src/systemd/src/libsystemd/sd-bus/bus-error.c:284:21
    #3 0x561059 in manager_load_unit_prepare /work/build/../../src/systemd/src/core/manager.c
    #4 0x560680 in manager_load_unit /work/build/../../src/systemd/src/core/manager.c:1773:13
    #5 0x5d49a6 in unit_add_dependency_by_name /work/build/../../src/systemd/src/core/unit.c:2882:13
    #6 0x538996 in config_parse_unit_deps /work/build/../../src/systemd/src/core/load-fragment.c:152:21
    #7 0x6db771 in next_assignment /work/build/../../src/systemd/src/shared/conf-parser.c:155:32
    #8 0x6d697e in parse_line /work/build/../../src/systemd/src/shared/conf-parser.c:273:16
    #9 0x6d5c48 in config_parse /work/build/../../src/systemd/src/shared/conf-parser.c:390:21
    #10 0x535678 in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-unit-file.c:41:16
    #11 0x73bd60 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:517:13
    #12 0x73a39f in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:442:3
    #13 0x73d9bc in fuzzer::Fuzzer::MutateAndTestOne() /src/libfuzzer/FuzzerLoop.cpp:650:19
    #14 0x73fa05 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:773:5
    #15 0x71f75d in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:754:6
    #16 0x71285c in main /src/libfuzzer/FuzzerMain.cpp:20:10
    #17 0x7fe53da0482f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #18 0x430e68 in _start (/out/fuzz-unit-file+0x430e68)

0x7fe53f5497d8 is located 8 bytes to the right of global variable 'bus_common_errors' defined in '../../src/systemd/src/libsystemd/sd-bus/bus-common-errors.c:28:51' (0x7fe53f549300) of size 1232
SUMMARY: AddressSanitizer: global-buffer-overflow /work/build/../../src/systemd/src/libsystemd/sd-bus/bus-error.c:118:24 in bus_error_name_to_errno
Shadow bytes around the buggy address:
  0x0ffd27ea12a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffd27ea12b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffd27ea12c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffd27ea12d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffd27ea12e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0ffd27ea12f0: 00 00 00 00 00 00 00 00 00 00 f9[f9]f9 f9 f9 f9
  0x0ffd27ea1300: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0ffd27ea1310: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0ffd27ea1320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffd27ea1330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffd27ea1340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==11==ABORTING

but I think it's a false positive because of our low-level magic in how this
area is constructed.
Mic92 pushed a commit that referenced this pull request Feb 22, 2019
This function returns 0 on success and a negative value on failure. On success,
it writes the parsed action to the address passed in its third argument.

`bus_set_transient_emergency_action` does this:

 r = parse_emergency_action(s, system, &v);
 if (v < 0)
     // handle failure

However, `v` is not updated if the function fails, and this should be checking
`r` instead of `v`.

The result of this is that if an invalid failure (or success) action is
specified, systemd ends up creating the unit anyway and then misbehaves if it
tries to run the failure action because the action value comes from
uninitialized stack data. In my case, this resulted in a failed assertion:

 Program received signal SIGABRT, Aborted.
 0x00007fe52cca0d7f in raise () from /snap/usr/lib/libc.so.6
 (gdb) bt
 #0  0x00007fe52cca0d7f in raise () from /snap/usr/lib/libc.so.6
 #1  0x00007fe52cc8b672 in abort () from /snap/usr/lib/libc.so.6
 #2  0x00007fe52d66f169 in log_assert_failed_realm (realm=LOG_REALM_SYSTEMD, text=0x56177ab8e000 "action < _EMERGENCY_ACTION_MAX", file=0x56177ab8dfb8 "../src/core/emergency-action.c", line=33, func=0x56177ab8e2b0 <__PRETTY_FUNCTION__.14207> "emergency_action") at ../src/basic/log.c:795
 #3  0x000056177aa98cf4 in emergency_action (m=0x56177c992cb0, action=2059118610, options=(unknown: 0), reboot_arg=0x0, exit_status=1, reason=0x7ffdd2df4290 "unit run-u0.service failed") at ../src/core/emergency-action.c:33
 #4  0x000056177ab2b739 in unit_notify (u=0x56177c9eb340, os=UNIT_ACTIVE, ns=UNIT_FAILED, flags=(unknown: 0)) at ../src/core/unit.c:2504
 #5  0x000056177aaf62ed in service_set_state (s=0x56177c9eb340, state=SERVICE_FAILED) at ../src/core/service.c:1104
 #6  0x000056177aaf8a29 in service_enter_dead (s=0x56177c9eb340, f=SERVICE_SUCCESS, allow_restart=true) at ../src/core/service.c:1712
 #7  0x000056177aaf9233 in service_enter_signal (s=0x56177c9eb340, state=SERVICE_FINAL_SIGKILL, f=SERVICE_SUCCESS) at ../src/core/service.c:1854
 #8  0x000056177aaf921b in service_enter_signal (s=0x56177c9eb340, state=SERVICE_FINAL_SIGTERM, f=SERVICE_SUCCESS) at ../src/core/service.c:1852
 #9  0x000056177aaf8eb3 in service_enter_stop_post (s=0x56177c9eb340, f=SERVICE_SUCCESS) at ../src/core/service.c:1788
 #10 0x000056177aaf91eb in service_enter_signal (s=0x56177c9eb340, state=SERVICE_STOP_SIGKILL, f=SERVICE_SUCCESS) at ../src/core/service.c:1850
 #11 0x000056177aaf91bc in service_enter_signal (s=0x56177c9eb340, state=SERVICE_STOP_SIGTERM, f=SERVICE_FAILURE_EXIT_CODE) at ../src/core/service.c:1848
 #12 0x000056177aaf9759 in service_enter_running (s=0x56177c9eb340, f=SERVICE_FAILURE_EXIT_CODE) at ../src/core/service.c:1941
 #13 0x000056177ab005b7 in service_sigchld_event (u=0x56177c9eb340, pid=112, code=1, status=1) at ../src/core/service.c:3296
 #14 0x000056177aad84b5 in manager_invoke_sigchld_event (m=0x56177c992cb0, u=0x56177c9eb340, si=0x7ffdd2df48f0) at ../src/core/manager.c:2444
 #15 0x000056177aad88df in manager_dispatch_sigchld (source=0x56177c994710, userdata=0x56177c992cb0) at ../src/core/manager.c:2508
 #16 0x00007fe52d72f807 in source_dispatch (s=0x56177c994710) at ../src/libsystemd/sd-event/sd-event.c:2846
 #17 0x00007fe52d730f7d in sd_event_dispatch (e=0x56177c993530) at ../src/libsystemd/sd-event/sd-event.c:3229
 #18 0x00007fe52d73142e in sd_event_run (e=0x56177c993530, timeout=18446744073709551615) at ../src/libsystemd/sd-event/sd-event.c:3286
 #19 0x000056177aad9f71 in manager_loop (m=0x56177c992cb0) at ../src/core/manager.c:2906
 #20 0x000056177aa7c876 in invoke_main_loop (m=0x56177c992cb0, ret_reexecute=0x7ffdd2df4bff, ret_retval=0x7ffdd2df4c04, ret_shutdown_verb=0x7ffdd2df4c58, ret_fds=0x7ffdd2df4c70, ret_switch_root_dir=0x7ffdd2df4c48, ret_switch_root_init=0x7ffdd2df4c50, ret_error_message=0x7ffdd2df4c60) at ../src/core/main.c:1792
 #21 0x000056177aa7f251 in main (argc=2, argv=0x7ffdd2df4e78) at ../src/core/main.c:2573

Fix this by checking the correct variable.
andir pushed a commit that referenced this pull request Sep 7, 2019
We would not send the property because we'd call sd_bus_get_current_message()
which would return NULL. If there is no message, we cannot support /self or
/auto, but things are still OK if a path with a session name is given.

Traceback when the issue is triggered:

 #2  we'd call sd_bus_get_current_message() here, which would return NULL, and
     session_object_find() would immediately return 0.
 #3  0x00000000004289b7 in session_object_find (bus=0x9f1110, path=0xa160b0 "/org/freedesktop/login1/session/c2",
     interface=0x9efda0 "org.freedesktop.login1.Session", userdata=0x9852f0, found=0x7ffe3e975fe8, error=0x7ffe3e9760b0)
     at ../src/login/logind-session-dbus.c:620
 #4  0x00007ff74bfdde39 in node_vtable_get_userdata (bus=0x9f1110, path=0xa160b0 "/org/freedesktop/login1/session/c2",
     c=0x9f6d58, userdata=0x7ffe3e976070, error=0x7ffe3e9760b0) at ../src/libsystemd/sd-bus/bus-objects.c:37
 #5  0x00007ff74bfe49af in emit_properties_changed_on_interface (bus=0x9f1110,
     prefix=0xa133a0 "/org/freedesktop/login1/session", path=0xa160b0 "/org/freedesktop/login1/session/c2",
     interface=0x43f9f8 "org.freedesktop.login1.Session", require_fallback=true, found_interface=0x7ffe3e976163,
     names=0x7ffe3e9761b0) at ../src/libsystemd/sd-bus/bus-objects.c:2088
 #6  0x00007ff74bfe56a4 in sd_bus_emit_properties_changed_strv (bus=0x9f1110,
     path=0xa160b0 "/org/freedesktop/login1/session/c2", interface=0x43f9f8 "org.freedesktop.login1.Session",
     names=0x7ffe3e9761b0) at ../src/libsystemd/sd-bus/bus-objects.c:2291
 #7  0x00000000004292ea in session_send_changed (s=0xa16e10, properties=0x43ee27 "Active")
    at ../src/login/logind-session-dbus.c:730
 #8  0x0000000000424cd7 in seat_set_active (s=0x9ee280, session=0xa16e10) at ../src/login/logind-seat.c:249
 #9  0x00000000004251cf in seat_active_vt_changed (s=0x9ee280, vtnr=3) at ../src/login/logind-seat.c:361
 #10 0x000000000042547b in seat_read_active_vt (s=0x9ee280) at ../src/login/logind-seat.c:395
 #11 0x000000000040ab5c in manager_dispatch_console (s=0x9f0320, fd=8, revents=8, userdata=0x9852f0)
     at ../src/login/logind.c:588
 #12 0x00007ff74c042d5f in source_dispatch (s=0x9f0320) at ../src/libsystemd/sd-event/sd-event.c:2828
 #13 0x00007ff74c04469f in sd_event_dispatch (e=0x9ef340) at ../src/libsystemd/sd-event/sd-event.c:3241
 #14 0x00007ff74c044b58 in sd_event_run (e=0x9ef340, timeout=18446744073709551615)
     at ../src/libsystemd/sd-event/sd-event.c:3299
 #15 0x000000000040d7e8 in manager_run (m=0x9852f0) at ../src/login/logind.c:1186
 #16 0x000000000040db58 in run (argc=1, argv=0x7ffe3e976728) at ../src/login/logind.c:1234
 #17 0x000000000040dc30 in main (argc=1, argv=0x7ffe3e976728) at ../src/login/logind.c:1244

Fixes systemd#13437. Bug introduced in 3b92c08.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants