core/transaction: several fixes for merge_unit_ids() #26875

yuwata · 2023-03-18T03:22:40Z

Follow-up for 924775e.
Fixes #26872.

…string Follow-up for 924775e. The loop run with `STRV_FOREACH_PAIR()`, hence `if (*(unit_id+1))` is not a good way to detect if there exist a next entry. Fixes systemd#26872.

As we ignores the failure in merge_unit_ids(), so unit_ids may be NULL.

yuwata · 2023-03-18T03:23:00Z

cc @mrc0mmand @evverx

evverx · 2023-03-18T06:52:40Z

@yuwata given how often this part of PID1 is affected by issues like that I think it would probably make sense to add a couple of tests. The test from #25387 doesn't fully cover these codepaths apparently. I also found another use-after-free (#24990 (comment)) at the time and I'm not sure that is covered by any tests either.

yuwata · 2023-03-18T10:38:38Z

@yuwata given how often this part of PID1 is affected by issues like that I think it would probably make sense to add a couple of tests. The test from #25387 doesn't fully cover these codepaths apparently. I also found another use-after-free (#24990 (comment)) at the time and I'm not sure that is covered by any tests either.

IIRC, the testcase added by #25387 covers the use-after-free you found, I think, but not sure.
Anyway, adding more tests for such the core part of PID1 should be welcome in general, but not easy to add that at least for me...

mrc0mmand · 2023-03-18T11:08:45Z

The good thing is that with this PR I can't no longer trigger the heap-buffer-overflow (thanks!). The bad thing is that #24452 is still happening, so the search continues.

keszybz · 2023-03-20T09:43:37Z

src/core/transaction.c

                                                    unit_id == array ? "ordering cycle" : "dependency",
                                                    *unit_id, *job_type),
-                                   "%s", unit_ids);
+                                   "%s", strna(unit_ids));


Wouldn't strnull() be better? I think we should use that instead to generally signify that we're not printing something because of an allocation failure.

As suggested at #26331 (comment), we should introduce stroom() or so, to indicate OOM error. I'd like to keep that as is in this PR. Let's handle OOM error in logging more consistently in another PR.

Provides coverage for systemd#26872. With systemd#26875 reverted: [16444.287652] testsuite-03.sh[71]: + for i in {0..19} [16444.287652] testsuite-03.sh[71]: + systemctl start transaction-cycle0.service [16444.359503] systemd[1]: ================================================================= [16444.360321] systemd[1]: ==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6180002e578e at pc 0x7f73b25ec7a6 bp 0x7ffc5531c6f0 sp 0x7ffc5531be68 [16444.360798] systemd[1]: [16444.361044] systemd[1]: READ of size 783 at 0x6180002e578e thread T0 (systemd) [16444.391684] systemd[1]: #0 0x7f73b25ec7a5 (/lib64/libasan.so.5+0x557a5) [16444.392167] systemd[1]: #1 0x7f73b260a1d5 in __interceptor_vasprintf (/lib64/libasan.so.5+0x731d5) [16444.392442] systemd[1]: #2 0x7f73afa1d1e1 in log_format_iovec ../src/basic/log.c:996 [16444.392750] systemd[1]: #3 0x7f73afa1e7b6 in log_struct_internal ../src/basic/log.c:1058 [16444.393101] systemd[1]: #4 0x7f73b1979136 in transaction_verify_order_one ../src/core/transaction.c:392 [16444.393540] systemd[1]: #5 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.393946] systemd[1]: #6 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394262] systemd[1]: #7 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394532] systemd[1]: #8 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394812] systemd[1]: #9 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 ...

Provides coverage for systemd/systemd#26872. With systemd/systemd#26875 reverted: [16444.287652] testsuite-03.sh[71]: + for i in {0..19} [16444.287652] testsuite-03.sh[71]: + systemctl start transaction-cycle0.service [16444.359503] systemd[1]: ================================================================= [16444.360321] systemd[1]: ==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6180002e578e at pc 0x7f73b25ec7a6 bp 0x7ffc5531c6f0 sp 0x7ffc5531be68 [16444.360798] systemd[1]: [16444.361044] systemd[1]: READ of size 783 at 0x6180002e578e thread T0 (systemd) [16444.391684] systemd[1]: #0 0x7f73b25ec7a5 (/lib64/libasan.so.5+0x557a5) [16444.392167] systemd[1]: systemd#1 0x7f73b260a1d5 in __interceptor_vasprintf (/lib64/libasan.so.5+0x731d5) [16444.392442] systemd[1]: systemd#2 0x7f73afa1d1e1 in log_format_iovec ../src/basic/log.c:996 [16444.392750] systemd[1]: systemd#3 0x7f73afa1e7b6 in log_struct_internal ../src/basic/log.c:1058 [16444.393101] systemd[1]: systemd#4 0x7f73b1979136 in transaction_verify_order_one ../src/core/transaction.c:392 [16444.393540] systemd[1]: systemd#5 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.393946] systemd[1]: systemd#6 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394262] systemd[1]: systemd#7 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394532] systemd[1]: systemd#8 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394812] systemd[1]: systemd#9 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 ... (cherry picked from commit 0651e71)

Provides coverage for systemd/systemd#26872. With systemd/systemd#26875 reverted: [16444.287652] testsuite-03.sh[71]: + for i in {0..19} [16444.287652] testsuite-03.sh[71]: + systemctl start transaction-cycle0.service [16444.359503] systemd[1]: ================================================================= [16444.360321] systemd[1]: ==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6180002e578e at pc 0x7f73b25ec7a6 bp 0x7ffc5531c6f0 sp 0x7ffc5531be68 [16444.360798] systemd[1]: [16444.361044] systemd[1]: READ of size 783 at 0x6180002e578e thread T0 (systemd) [16444.391684] systemd[1]: #0 0x7f73b25ec7a5 (/lib64/libasan.so.5+0x557a5) [16444.392167] systemd[1]: #1 0x7f73b260a1d5 in __interceptor_vasprintf (/lib64/libasan.so.5+0x731d5) [16444.392442] systemd[1]: #2 0x7f73afa1d1e1 in log_format_iovec ../src/basic/log.c:996 [16444.392750] systemd[1]: #3 0x7f73afa1e7b6 in log_struct_internal ../src/basic/log.c:1058 [16444.393101] systemd[1]: #4 0x7f73b1979136 in transaction_verify_order_one ../src/core/transaction.c:392 [16444.393540] systemd[1]: #5 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.393946] systemd[1]: #6 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394262] systemd[1]: #7 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394532] systemd[1]: #8 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394812] systemd[1]: #9 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 ... (cherry picked from commit 0651e71)

Provides coverage for systemd/systemd#26872. With systemd/systemd#26875 reverted: [16444.287652] testsuite-03.sh[71]: + for i in {0..19} [16444.287652] testsuite-03.sh[71]: + systemctl start transaction-cycle0.service [16444.359503] systemd[1]: ================================================================= [16444.360321] systemd[1]: ==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6180002e578e at pc 0x7f73b25ec7a6 bp 0x7ffc5531c6f0 sp 0x7ffc5531be68 [16444.360798] systemd[1]: [16444.361044] systemd[1]: READ of size 783 at 0x6180002e578e thread T0 (systemd) [16444.391684] systemd[1]: #0 0x7f73b25ec7a5 (/lib64/libasan.so.5+0x557a5) [16444.392167] systemd[1]: systemd#1 0x7f73b260a1d5 in __interceptor_vasprintf (/lib64/libasan.so.5+0x731d5) [16444.392442] systemd[1]: systemd#2 0x7f73afa1d1e1 in log_format_iovec ../src/basic/log.c:996 [16444.392750] systemd[1]: systemd#3 0x7f73afa1e7b6 in log_struct_internal ../src/basic/log.c:1058 [16444.393101] systemd[1]: systemd#4 0x7f73b1979136 in transaction_verify_order_one ../src/core/transaction.c:392 [16444.393540] systemd[1]: systemd#5 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.393946] systemd[1]: systemd#6 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394262] systemd[1]: systemd#7 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394532] systemd[1]: systemd#8 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394812] systemd[1]: systemd#9 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 ... (cherry picked from commit 0651e71) (cherry picked from commit fdc6ce1)

Provides coverage for systemd/systemd#26872. With systemd/systemd#26875 reverted: [16444.287652] testsuite-03.sh[71]: + for i in {0..19} [16444.287652] testsuite-03.sh[71]: + systemctl start transaction-cycle0.service [16444.359503] systemd[1]: ================================================================= [16444.360321] systemd[1]: ==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6180002e578e at pc 0x7f73b25ec7a6 bp 0x7ffc5531c6f0 sp 0x7ffc5531be68 [16444.360798] systemd[1]: [16444.361044] systemd[1]: READ of size 783 at 0x6180002e578e thread T0 (systemd) [16444.391684] systemd[1]: #0 0x7f73b25ec7a5 (/lib64/libasan.so.5+0x557a5) [16444.392167] systemd[1]: #1 0x7f73b260a1d5 in __interceptor_vasprintf (/lib64/libasan.so.5+0x731d5) [16444.392442] systemd[1]: #2 0x7f73afa1d1e1 in log_format_iovec ../src/basic/log.c:996 [16444.392750] systemd[1]: #3 0x7f73afa1e7b6 in log_struct_internal ../src/basic/log.c:1058 [16444.393101] systemd[1]: #4 0x7f73b1979136 in transaction_verify_order_one ../src/core/transaction.c:392 [16444.393540] systemd[1]: #5 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.393946] systemd[1]: #6 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394262] systemd[1]: #7 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394532] systemd[1]: #8 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394812] systemd[1]: #9 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 ... (cherry picked from commit 0651e71) (cherry picked from commit fdc6ce1)

Provides coverage for systemd/systemd#26872. With systemd/systemd#26875 reverted: [16444.287652] testsuite-03.sh[71]: + for i in {0..19} [16444.287652] testsuite-03.sh[71]: + systemctl start transaction-cycle0.service [16444.359503] systemd[1]: ================================================================= [16444.360321] systemd[1]: ==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6180002e578e at pc 0x7f73b25ec7a6 bp 0x7ffc5531c6f0 sp 0x7ffc5531be68 [16444.360798] systemd[1]: [16444.361044] systemd[1]: READ of size 783 at 0x6180002e578e thread T0 (systemd) [16444.391684] systemd[1]: #0 0x7f73b25ec7a5 (/lib64/libasan.so.5+0x557a5) [16444.392167] systemd[1]: systemd#1 0x7f73b260a1d5 in __interceptor_vasprintf (/lib64/libasan.so.5+0x731d5) [16444.392442] systemd[1]: systemd#2 0x7f73afa1d1e1 in log_format_iovec ../src/basic/log.c:996 [16444.392750] systemd[1]: systemd#3 0x7f73afa1e7b6 in log_struct_internal ../src/basic/log.c:1058 [16444.393101] systemd[1]: systemd#4 0x7f73b1979136 in transaction_verify_order_one ../src/core/transaction.c:392 [16444.393540] systemd[1]: systemd#5 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.393946] systemd[1]: systemd#6 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394262] systemd[1]: systemd#7 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394532] systemd[1]: systemd#8 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394812] systemd[1]: systemd#9 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 ... (cherry picked from commit 0651e71) (cherry picked from commit fdc6ce1) (cherry picked from commit 4ac2071)

Provides coverage for systemd/systemd#26872. With systemd/systemd#26875 reverted: [16444.287652] testsuite-03.sh[71]: + for i in {0..19} [16444.287652] testsuite-03.sh[71]: + systemctl start transaction-cycle0.service [16444.359503] systemd[1]: ================================================================= [16444.360321] systemd[1]: ==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6180002e578e at pc 0x7f73b25ec7a6 bp 0x7ffc5531c6f0 sp 0x7ffc5531be68 [16444.360798] systemd[1]: [16444.361044] systemd[1]: READ of size 783 at 0x6180002e578e thread T0 (systemd) [16444.391684] systemd[1]: #0 0x7f73b25ec7a5 (/lib64/libasan.so.5+0x557a5) [16444.392167] systemd[1]: #1 0x7f73b260a1d5 in __interceptor_vasprintf (/lib64/libasan.so.5+0x731d5) [16444.392442] systemd[1]: #2 0x7f73afa1d1e1 in log_format_iovec ../src/basic/log.c:996 [16444.392750] systemd[1]: #3 0x7f73afa1e7b6 in log_struct_internal ../src/basic/log.c:1058 [16444.393101] systemd[1]: #4 0x7f73b1979136 in transaction_verify_order_one ../src/core/transaction.c:392 [16444.393540] systemd[1]: #5 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.393946] systemd[1]: #6 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394262] systemd[1]: #7 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394532] systemd[1]: #8 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 [16444.394812] systemd[1]: #9 0x7f73b197ac82 in transaction_verify_order_one ../src/core/transaction.c:463 ... (cherry picked from commit 0651e71) (cherry picked from commit fdc6ce1) (cherry picked from commit 4ac2071)

yuwata added 3 commits March 18, 2023 12:12

core/transaction: make merge_unit_ids() always return NUL-terminated …

366eced

…string Follow-up for 924775e. The loop run with `STRV_FOREACH_PAIR()`, hence `if (*(unit_id+1))` is not a good way to detect if there exist a next entry. Fixes systemd#26872.

core/transaction: make merge_unit_ids() return non-NULL on success

999f165

core/transaction: do not log "(null)"

5803c24

As we ignores the failure in merge_unit_ids(), so unit_ids may be NULL.

github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Mar 18, 2023

This comment was marked as off-topic.

Sign in to view

yuwata added pid1 needs-stable-backport labels Mar 18, 2023

yuwata requested a review from keszybz March 18, 2023 03:38

jamacku mentioned this pull request Mar 18, 2023

New GitHub Code Scanning feature makes unwanted comments on each PR redhat-plumbers-in-action/differential-shellcheck#215

Closed

keszybz reviewed Mar 20, 2023

View reviewed changes

keszybz added good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels Mar 20, 2023

mrc0mmand mentioned this pull request Mar 20, 2023

RHEL 9.2 "to-backport" list redhat-plumbers/systemd-rhel9#154

Closed

yuwata removed the good-to-merge/with-minor-suggestions label Mar 20, 2023

yuwata merged commit 719bbb3 into systemd:main Mar 20, 2023

yuwata deleted the core-transaction branch March 20, 2023 15:42

keszybz removed the needs-stable-backport label Mar 28, 2023

bryce-cahill mentioned this pull request Apr 26, 2023

init: Assertion 'hashmap_isempty(m->jobs)' failed at src/core/manager.c:1474, function manager_clear_jobs_and_units(). Aborting. #24577

Closed

mrc0mmand mentioned this pull request Jun 23, 2023

test: test transactions with cycles #28140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

core/transaction: several fixes for merge_unit_ids() #26875

core/transaction: several fixes for merge_unit_ids() #26875

Uh oh!

yuwata commented Mar 18, 2023 •

edited

Loading

Uh oh!

yuwata commented Mar 18, 2023

Uh oh!

This comment was marked as off-topic.

evverx commented Mar 18, 2023

Uh oh!

yuwata commented Mar 18, 2023

Uh oh!

mrc0mmand commented Mar 18, 2023

Uh oh!

keszybz Mar 20, 2023

Uh oh!

yuwata Mar 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Uh oh!

core/transaction: several fixes for merge_unit_ids() #26875

core/transaction: several fixes for merge_unit_ids() #26875

Uh oh!

Conversation

yuwata commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuwata commented Mar 18, 2023

Uh oh!

This comment was marked as off-topic.

evverx commented Mar 18, 2023

Uh oh!

yuwata commented Mar 18, 2023

Uh oh!

mrc0mmand commented Mar 18, 2023

Uh oh!

keszybz Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

yuwata Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

yuwata commented Mar 18, 2023 •

edited

Loading