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

tests: add a fuzzer for dev_kmsg_record #10788

Merged
merged 5 commits into from
Nov 16, 2018
Merged

Conversation

evverx
Copy link
Member

@evverx evverx commented Nov 16, 2018

No description provided.

buffer = memdup(data, size);
assert_se(buffer);
s.syslog_fd = s.native_fd = s.stdout_fd = s.dev_kmsg_fd = s.audit_fd = s.hostname_fd = s.notify_fd = -1;
s.storage = STORAGE_NONE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not a matter but I prefer to use structured initializer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuwata thank you for the review!

This was originally borrowed verbatim from server_init, where there're a lot of assignments like that, which, of course, shouldn't be an excuse for blind copy-pasting :-) I've just changed that. Could you take a look?

By the way, I seem to have lost track of all the labels we have :-) What is "good-to-merge/with-minor-suggestions"? Does it mean that, generally, suggestions can be ignored? Or is it just a little bit softer way to say that the code isn't exactly ideal? :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, it's the first time I see this one too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, it's the first time I see this one too.

I saw that once in my PR, but I forgot which one.

Does it mean that, generally, suggestions can be ignored?

Yeah, I used the tag for that meaning :-) Sorry for the confusion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fuzzer has started to crash with

==220==ERROR: AddressSanitizer: ABRT on unknown address 0x0000000000dc (pc 0x7ff4953c8428 bp 0x7ffcf66ec290 sp 0x7ffcf66ec128 T0)
SCARINESS: 10 (signal)
    #0 0x7ff4953c8427 in gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x35427)
    #1 0x7ff4953ca029 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x37029)
    #2 0x7ff49666503a in log_assert_failed_realm /work/build/../../src/systemd/src/basic/log.c:805:9
    #3 0x7ff496614ecf in safe_close /work/build/../../src/systemd/src/basic/fd-util.c:66:17
    #4 0x548806 in server_done /work/build/../../src/systemd/src/journal/journald-server.c:2064:9
    #5 0x5349fa in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journald-kmsg.c:26:9
    #6 0x592755 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:571:15
    #7 0x590627 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:480:3
    #8 0x594432 in fuzzer::Fuzzer::MutateAndTestOne() /src/libfuzzer/FuzzerLoop.cpp:708:19
    #9 0x5973c6 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:839:5
    #10 0x574541 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:764:6
    #11 0x5675fc in main /src/libfuzzer/FuzzerMain.cpp:20:10
    #12 0x7ff4953b382f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #13 0x420f58 in _start (/out/fuzz-journald-kmsg+0x420f58)

Apparently, I forgot to initialize syslog_fd. I'll fix it shortly.

Yeah, I used the tag for that meaning :-) Sorry for the confusion.

No problem. I wasn't confused at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fuzzer has started to crash

I've just opened #10794 so that TravisCI and semaphoreci will be able to catch slip-ups like that in the future.

The function takes a pointer to a random block of memory and
the length of that block. It shouldn't crash every time it sees
a zero byte at the beginning there.

This should help the dev-kmsg fuzzer to keep going.
};
assert_se(sd_event_default(&s.event) >= 0);
buffer = memdup(data, size);
assert_se(buffer);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge those two lines?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, Coverity doesn't like assignments mixed with assertions. d9fb7af was supposed to fix this but apparently cases like that haven't been covered yet.

Copy link
Member

@keszybz keszybz Nov 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK. Nvm then.

@@ -106,7 +106,6 @@ int cunescape_one(const char *p, size_t length, char32_t *ret, bool *eight_bit)
int r = 1;

assert(p);
assert(*p);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the understatedness in the last sentence in the patch description.

@keszybz keszybz merged commit 708dab4 into systemd:master Nov 16, 2018
@evverx evverx deleted the fuzz-kmsg branch November 16, 2018 08:19
#include "journald-kmsg.h"

int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
Server s = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to initialize this here if you initialize it further down anyway

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poettering I addressed that in #10817.

@evverx evverx added the fuzzing Implementation of fuzzers and fixes for stuff found through fuzzing label Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzzing Implementation of fuzzers and fixes for stuff found through fuzzing journal tests
Development

Successfully merging this pull request may close these issues.

None yet

4 participants