Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build-system: bring back 'nonnull' #15704

Merged
merged 1 commit into from
May 6, 2020
Merged

Conversation

evverx
Copy link
Member

@evverx evverx commented May 5, 2020

Having taken a look at https://github.com/systemd/systemd/runs/645252074?check_suite_focus=true
where fuzz-journal-remote failed with

AddressSanitizer:DEADLYSIGNAL
=================================================================
==16==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f864f98948e bp 0x7ffde5c6b7c0 sp 0x7ffde5c6b560 T0)
==16==The signal is caused by a READ memory access.
==16==Hint: address points to the zero page.
SCARINESS: 10 (null-deref)
    #0 0x7f864f98948e in output_short /work/build/../../src/systemd/src/shared/logs-show.c
    #1 0x7f864f984624 in show_journal_entry /work/build/../../src/systemd/src/shared/logs-show.c:1154:15
    #2 0x7f864f984b63 in show_journal /work/build/../../src/systemd/src/shared/logs-show.c:1239:21
    #3 0x4cabab in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journal-remote.c:67:21
    #4 0x51fd16 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:556:15
    #5 0x51c330 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:470:3
    #6 0x523700 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:765:7
    #7 0x5246cd in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:792:3
    #8 0x4de3d1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:824:6
    #9 0x4cfb47 in main /src/libfuzzer/FuzzerMain.cpp:19:10
    #10 0x7f864e69782f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #11 0x41f2a8 in _start (out/fuzz-journal-remote+0x41f2a8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /work/build/../../src/systemd/src/shared/logs-show.c in output_short
==16==ABORTING
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x44,0x3d,0xa,0x5f,0x5f,0x52,0x45,0x41,0x4c,0x54,0x49,0x4d,0x45,0x5f,0x54,0x49,0x4d,0x45,0x53,0x54,0x41,0x4d,0x50,0x3d,0x31,0xa,0xa,
D=\x0a__REALTIME_TIMESTAMP=1\x0a\x0a
artifact_prefix='./'; Test unit written to ./crash-d635b9dd31cceff3c912fd45e1a58d7e90f0ad73
Base64: RD0KX19SRUFMVElNRV9USU1FU1RBTVA9MQoK

I was wondering why it hadn't been caught by the compiler even though clang should have failed to compile it with

../src/shared/logs-show.c:624:25: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
                        print_multiline(f, 4 + fieldlen + 1, 0, OUTPUT_FULL_WIDTH, 0, false,
                        ^
../src/shared/logs-show.c:161:24: note: callee declares array parameter as static here
                size_t highlight[static 2]) {
                       ^        ~~~~~~~~~~
../src/shared/logs-show.c:1239:21: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
                r = show_journal_entry(f, j, mode, n_columns, flags, NULL, NULL, ellipsized);
                    ^                                                      ~~~~
../src/shared/logs-show.c:1133:30: note: callee declares array parameter as static here
                const size_t highlight[static 2],
                             ^        ~~~~~~~~~~
2 warnings generated.

Given that judging by #13039 it doesn't seem to be
the first time issues like that have been missed I think it would be better to turn nonnull on
and get around false positives on a case-by-case basis with DISABLE_WARNING_NONNULL .. REENABLE_WARNING

Reopens #6119

Having taken a look at https://github.com/systemd/systemd/runs/645252074?check_suite_focus=true
where fuzz-journal-remote failed with
```
AddressSanitizer:DEADLYSIGNAL
=================================================================
==16==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f864f98948e bp 0x7ffde5c6b7c0 sp 0x7ffde5c6b560 T0)
==16==The signal is caused by a READ memory access.
==16==Hint: address points to the zero page.
SCARINESS: 10 (null-deref)
    #0 0x7f864f98948e in output_short /work/build/../../src/systemd/src/shared/logs-show.c
    #1 0x7f864f984624 in show_journal_entry /work/build/../../src/systemd/src/shared/logs-show.c:1154:15
    #2 0x7f864f984b63 in show_journal /work/build/../../src/systemd/src/shared/logs-show.c:1239:21
    #3 0x4cabab in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journal-remote.c:67:21
    #4 0x51fd16 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:556:15
    #5 0x51c330 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:470:3
    #6 0x523700 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:765:7
    #7 0x5246cd in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:792:3
    #8 0x4de3d1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:824:6
    #9 0x4cfb47 in main /src/libfuzzer/FuzzerMain.cpp:19:10
    #10 0x7f864e69782f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #11 0x41f2a8 in _start (out/fuzz-journal-remote+0x41f2a8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /work/build/../../src/systemd/src/shared/logs-show.c in output_short
==16==ABORTING
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x44,0x3d,0xa,0x5f,0x5f,0x52,0x45,0x41,0x4c,0x54,0x49,0x4d,0x45,0x5f,0x54,0x49,0x4d,0x45,0x53,0x54,0x41,0x4d,0x50,0x3d,0x31,0xa,0xa,
D=\x0a__REALTIME_TIMESTAMP=1\x0a\x0a
artifact_prefix='./'; Test unit written to ./crash-d635b9dd31cceff3c912fd45e1a58d7e90f0ad73
Base64: RD0KX19SRUFMVElNRV9USU1FU1RBTVA9MQoK
```
I was wondering why it hadn't been caught by the compiler even though clang should have failed to compile it with
```
../src/shared/logs-show.c:624:25: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
                        print_multiline(f, 4 + fieldlen + 1, 0, OUTPUT_FULL_WIDTH, 0, false,
                        ^
../src/shared/logs-show.c:161:24: note: callee declares array parameter as static here
                size_t highlight[static 2]) {
                       ^        ~~~~~~~~~~
../src/shared/logs-show.c:1239:21: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
                r = show_journal_entry(f, j, mode, n_columns, flags, NULL, NULL, ellipsized);
                    ^                                                      ~~~~
../src/shared/logs-show.c:1133:30: note: callee declares array parameter as static here
                const size_t highlight[static 2],
                             ^        ~~~~~~~~~~
2 warnings generated.
```

Given that judging by systemd#13039 it doesn't seem to be
the first time issues like that have been missed I think it would be better to turn nonnull on
and get around false positives on a case-by-case basis with DISABLE_WARNING_NONNULL .. REENABLE_WARNING

Reopens systemd#6119
@keszybz
Copy link
Member

keszybz commented May 5, 2020

If CI passes, then let's merge.

@keszybz keszybz added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label May 5, 2020
@keszybz
Copy link
Member

keszybz commented May 6, 2020

bionic-amd64 failed with with a timeout when rebooting. This patch only affects compilation, so it should be unrelated.

@keszybz keszybz added ci-failure-appears-unrelated and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels May 6, 2020
@keszybz keszybz merged commit 7f3a5eb into systemd:master May 6, 2020
@evverx evverx deleted the turn-on-nonnull branch May 6, 2020 08:04
evverx added a commit to evverx/systemd that referenced this pull request May 6, 2020
elvees-gerrit pushed a commit to elvees/mcom02-buildroot-external that referenced this pull request Sep 14, 2020
systemd 246 (used in Buildroot 2020.08) brings back fatality of nonnull
warning. GCC 7.x produces false positive nonnull warnings and fails
to compile systemd. See systemd issue [1].
See also [2].

[1] systemd/systemd#6119
[2] systemd/systemd#15704

Issue: #MCOM02SW-1868
Change-Id: I3b6d1ab0e0c30c0bfb57f87580f275897ec144ef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants