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

Make Centipede a default engine #9427

Closed
wants to merge 13 commits into from
Closed

Conversation

DonggeLiu
Copy link
Contributor

@DonggeLiu DonggeLiu commented Jan 16, 2023

Make Centipede a default fuzzing engine and apply it to more projects:

  • Add its build steps to expected_build_steps.json.
    • Should we delete the previous test_get_centipede_build_steps, given it has been covered in test_get_build_steps now?

@DonggeLiu DonggeLiu marked this pull request as draft January 16, 2023 00:02
@DonggeLiu
Copy link
Contributor Author

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

@DonggeLiu DonggeLiu force-pushed the make_centipede_default_engine branch from f623023 to be42b22 Compare January 16, 2023 02:16
@DonggeLiu
Copy link
Contributor Author

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

evverx added a commit to evverx/oss-fuzz that referenced this pull request Jan 16, 2023
Looks like until google#9427 is merged
it should be specified explicitly.
@evverx
Copy link
Contributor

evverx commented Jan 16, 2023

To judge from https://github.com/google/oss-fuzz/actions/runs/3928694793/jobs/6716605398 systemd fails to build with centipede due to I assume another variation on mesonbuild/meson#4542 or something like that. I can exclude centipede myself by explicitly listing the three fuzzing engines the build system currently supports later but to prevent the next systemd build from failing on CF it would be great if it could be excluded here.

@evverx
Copy link
Contributor

evverx commented Jan 16, 2023

Just to clarify I enabled it explicitly in #9428. It doesn't fail anywhere else yet.

@evverx
Copy link
Contributor

evverx commented Jan 16, 2023

Before I forget other projects using meson like dbus-broker fail to build with centipede too. I think if by analogy with COVERAGE_FLAGS_coverage OSS-Fuzz prepended -Wl to linker options it would make centipede work with meson as well because mesonbuild/meson#4542 was fixed by filtering out external linker flags passed via CFLAGS starting with -Wl. I'm not sure meson is going to start filtering out anything else.

@DonggeLiu
Copy link
Contributor Author

DonggeLiu commented Jan 17, 2023

Before I forget other projects using meson like dbus-broker fail to build with centipede too. I think if by analogy with COVERAGE_FLAGS_coverage OSS-Fuzz prepended -Wl to linker options it would make centipede work with meson as well because mesonbuild/meson#4542 was fixed by filtering out external linker flags passed via CFLAGS starting with -Wl. I'm not sure meson is going to start filtering out anything else.

Thanks, @evverx!
Yeah, Centipede fails on all projects that have -Werror -Wunused-command-line-argument. We made an attempt to address it, but that does not fix all projects, as you pointed out.

-Wl is a good point, thanks for suggesting that!
While I was experimenting -Wl on systemd, I got a different error:

../../src/systemd/src/basic/alloc-util.h:123:35: error: static declaration of 'reallocarray' follows non-static declaration
_alloc_(2, 3) static inline void *reallocarray(void *p, size_t need, size_t size) {
                                  ^
/usr/include/stdlib.h:559:14: note: previous declaration is here
extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)

Would you happen to know how to fix it? Thanks!

By the way, we plan to make Centipede support all projects, as it will replace libFuzzer.

@DonggeLiu
Copy link
Contributor Author

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

@DonggeLiu
Copy link
Contributor Author

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

@evverx
Copy link
Contributor

evverx commented Jan 17, 2023

While I was experimenting -Wl on systemd, I got a different error:

Looks like cc.has_function went sideways somewhere because it's supposed to set HAVE_REALLOCARRAY when reallocarray is shipped with glibc to prevent build failures exactly like that. I'll try to reproduce it locally to get the meson build logs. Just to make sure did it fail when LIBRARIES_FLAGS="-Wno-unused-command-line-argument -Wl,-ldl,-lrt,-lpthread,$SRC/centipede/weak.o" was added to CFLAGS?

@DonggeLiu
Copy link
Contributor Author

While I was experimenting -Wl on systemd, I got a different error:

Looks like cc.has_function went sideways somewhere because it's supposed to set HAVE_REALLOCARRAY when reallocarray is shipped with glibc to prevent build failures exactly like that. I'll try to reproduce it locally to get the meson build logs. Just to make sure did it fail when LIBRARIES_FLAGS="-Wno-unused-command-line-argument -Wl,-ldl,-lrt,-lpthread,$SRC/centipede/weak.o" was added to CFLAGS?

Yep, the same as in this commit Use -Wl for linker flags.

@evverx
Copy link
Contributor

evverx commented Jan 17, 2023

I rebuilt the base-builder image with those flags and reproduced the build failure. The good news is that systemd got to that point :-) (which means that -Wl works and get_supported_arguments(possible_cc_flags) no longer fails). cc.has_function fails to build its test programs with something like

Command line:  clang -L/usr/lib/clang/15.0.0/lib/linux /work/build/meson-private/tmp77ei02ar/testfile.c -o /work/build/meson-private/tmp77ei02ar/output.exe -O1 -fno-omit-frame-pointer -gline-tables-only -fsanitize-coverage=trace-loads -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fno-builtin -fsanitize-coverage=trace-pc-guard,pc-table,trace-cmp -O2 -gline-tables-only -Wno-unused-command-line-argument -fsanitize=fuzzer-no-link -D_FILE_OFFSET_BITS=64 -O0 -Werror=implicit-function-declaration -std=gnu11 -D_GNU_SOURCE
...
Compiler stderr:^M
 /usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x25^M
/tmp/testfile-bd5938.o: in function `main':^M
testfile.c:(.text.main[main]+0x41): undefined reference to `__sanitizer_cov_load8'^M
/usr/bin/ld: testfile.c:(.text.main[main]+0x52): undefined reference to `__sanitizer_cov_load8'^M
clang-15: error: linker command failed with exit code 1 (use -v to see invocation)^M

I'm not sure what to do about it yet. I don't think I can fix it in the systemd OSS-Fuzz build script because it's somewhere between meson and OSS-Fuzz.

@evverx
Copy link
Contributor

evverx commented Jan 17, 2023

dbus-broker no longer fails to build though (because cc.has_function isn't used there). lxc fails due to the same cc.has_function issue.

@DonggeLiu
Copy link
Contributor Author

DonggeLiu commented Jan 17, 2023

I rebuilt the base-builder image with those flags and reproduced the build failure. The good news is that systemd got to that point :-) (which means that -Wl works and get_supported_arguments(possible_cc_flags) no longer fails). cc.has_function fails to build its test programs with something like

Command line:  clang -L/usr/lib/clang/15.0.0/lib/linux /work/build/meson-private/tmp77ei02ar/testfile.c -o /work/build/meson-private/tmp77ei02ar/output.exe -O1 -fno-omit-frame-pointer -gline-tables-only -fsanitize-coverage=trace-loads -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fno-builtin -fsanitize-coverage=trace-pc-guard,pc-table,trace-cmp -O2 -gline-tables-only -Wno-unused-command-line-argument -fsanitize=fuzzer-no-link -D_FILE_OFFSET_BITS=64 -O0 -Werror=implicit-function-declaration -std=gnu11 -D_GNU_SOURCE
...
Compiler stderr:^M
 /usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x25^M
/tmp/testfile-bd5938.o: in function `main':^M
testfile.c:(.text.main[main]+0x41): undefined reference to `__sanitizer_cov_load8'^M
/usr/bin/ld: testfile.c:(.text.main[main]+0x52): undefined reference to `__sanitizer_cov_load8'^M
clang-15: error: linker command failed with exit code 1 (use -v to see invocation)^M

I'm not sure what to do about it yet. I don't think I can fix it in the systemd OSS-Fuzz build script because it's somewhere between meson and OSS-Fuzz.

My experience is that errors like /usr/bin/ld: testfile.c:(.text.main[main]+0x52): undefined reference to '__sanitizer_cov_load8'^M can be fixed by making use of weak.o from Centipede.
This weak.o defines the weak symbols of those functions.

However, I am not sure what is the best way to make use of it in systemd.
Would you happen to have some thoughts?

@evverx
Copy link
Contributor

evverx commented Jan 17, 2023

Ideally OSS-Fuzz should pass linker flags via LDFLAGS (at least that's where meson expects to find them: mesonbuild/meson#4542 (comment)). Having said that as far as I can remember LDFLAGS broke something else and were rolled back at some point.

With $SRC/centipede/weak.o added to LDFLAGS explicitly systemd can be built with centipede as far as I can see but it would have to be added to every project where has_function is used and given that I already have to carry kludges like 7ac2b25 I don't think it's very convenient.

@evverx
Copy link
Contributor

evverx commented Jan 17, 2023

FWIW I think in theory it could work if OSS-Fuzz passed those flags via LDFLAGS as well. It didn't work out with FI because it needed to switch linkers and some projects didn't expect it so CFLAGS was the way to go there but with centipede the linker is intact as far as I can tell.

@DonggeLiu
Copy link
Contributor Author

FWIW I think in theory it could work if OSS-Fuzz passed those flags via LDFLAGS as well. It didn't work out with FI because it needed to switch linkers and some projects didn't expect it so CFLAGS was the way to go there but with centipede the linker is intact as far as I can tell.

I see... Yeah adding it to LDFLAGS is not convenient.
But it's good to know that works! Thanks a lot!

I will try to think of ways to do it in one place : )

DonggeLiu added a commit to google/fuzzbench that referenced this pull request Jan 18, 2023
Mirrors [the fixes from
OSS-Fuzz](google/oss-fuzz#9427):
1. Use [`-Wl` on the linker
flags](google/oss-fuzz#9427 (comment)).
2. Use
[`LDFLAGS`](google/oss-fuzz#9427 (comment)).
@DonggeLiu
Copy link
Contributor Author

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

@jonathanmetzman
Copy link
Contributor

Related: #9299

@DonggeLiu
Copy link
Contributor Author

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

@DonggeLiu
Copy link
Contributor Author

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

@DonggeLiu
Copy link
Contributor Author

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

@DonggeLiu
Copy link
Contributor Author

@jonathanmetzman, 334aff6 compiles weak.o with $CC instead of $CXX as we discussed.
The undefined reference to operator new()/delete() error reemerged.
I suppose we will still need to compile/link target with -lc++ : )

@DonggeLiu
Copy link
Contributor Author

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

@jonathanmetzman
Copy link
Contributor

@jonathanmetzman, 334aff6 compiles weak.o with $CC instead of $CXX as we discussed. The undefined reference to operator new()/delete() error reemerged. I suppose we will still need to compile/link target with -lc++ : )

What is LIB_FUZZING_ENGINE compiled with? Also clang++?

@jonathanmetzman
Copy link
Contributor

@jonathanmetzman, 334aff6 compiles weak.o with $CC instead of $CXX as we discussed. The undefined reference to operator new()/delete() error reemerged. I suppose we will still need to compile/link target with -lc++ : )

What is LIB_FUZZING_ENGINE compiled with? Also clang++?

I think this doesn't happen with AFL(++) because I'm pretty sure they only use C for the part that gets linked to user code.

jonathanmetzman pushed a commit that referenced this pull request Feb 6, 2023
Comment on lines +34 to +35
# For Meson.
export LDFLAGS='/src/centipede/weak.o'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not clear why centipede is so unique that we need to support LDFLAGS after six years of not needing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which projects does this help? Most meson projects seem broken still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I will test removing it here.
I've added weak.o to ld wrapper, not sure if that works for meson.

# For Meson.
export LDFLAGS='/src/centipede/weak.o'

# For Centipede's ld
Copy link
Contributor

Choose a reason for hiding this comment

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

end with period.


# For Centipede's ld
export CENTIPEDE_BIN="$SRC/centipede/bin"
export PATH="$CENTIPEDE_BIN":$PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

Put ending quote on the end of this line.

@jonathanmetzman
Copy link
Contributor

It's extremely hard for me to test anything today because archive.ubuntu.com is down. But you can see here that a project that is having issues with libc++ not being linked in, is doing something incorrect. They are using CC to link fuzz targets despite our rule requiring use of CXX for this purpose

@jonathanmetzman
Copy link
Contributor

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

@jonathanmetzman
Copy link
Contributor

Related: #9299

@jonathanmetzman
Copy link
Contributor

It's extremely hard for me to test anything today because archive.ubuntu.com is down. But you can see here that a project that is having issues with libc++ not being linked in, is doing something incorrect. They are using CC to link fuzz targets despite our rule requiring use of CXX for this purpose

Looks like this hunch was correct #9599

@jonathanmetzman
Copy link
Contributor

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

1 similar comment
@jonathanmetzman
Copy link
Contributor

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

@jonathanmetzman
Copy link
Contributor

/gcbrun trial_build.py skcms systemd --sanitizer none address --fuzzing-engine libfuzzer afl honggfuzz

@DonggeLiu
Copy link
Contributor Author

DonggeLiu commented Feb 7, 2023

/gcbrun trial_build.py all --sanitizer none --fuzzing-engine centipede

How do we confirm the exact build ID of this command? (It's the second one above this.)
The build history page has two builds (4285852f, 4edf647e) on the latest commit 38d53f2, but I am not sure why the first one failed and the second passed.

There is a third one on the top, 827d915c. I presume that is the last one ( /gcbrun trial_build.py skcms systemd --sanitizer none address --fuzzing-engine libfuzzer afl honggfuzz)

@jonathanmetzman
Copy link
Contributor

How do we confirm the exact build ID of this command? (It's the second one above this.)
The build history page has two builds (4285852f, 4edf647e) on the latest commit 38d53f2, but I am not sure why the first one failed and the second passed.

I think youre right. But you have to look at what gets built to figure this out.

@DonggeLiu
Copy link
Contributor Author

How do we confirm the exact build ID of this command? (It's the second one above this.)
The build history page has two builds (4285852f, 4edf647e) on the latest commit 38d53f2, but I am not sure why the first one failed and the second passed.

I think youre right. But you have to look at what gets built to figure this out.

Both 4edf647e and 827d915c only tested skcms and systemd, which is why they passed.

Only 4285852f tested Centipede on all projects, and it failed as expected.

#!/bin/bash
/usr/bin/ld \$@ -ldl -lrt -lpthread -lc++ /src/centipede/weak.o
EOF
chmod 777 ${CENTIPEDE_BIN}/ld
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we setting this to be the linker somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@jonathanmetzman jonathanmetzman Feb 7, 2023

Choose a reason for hiding this comment

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

Ah got it. Do you mind giving me an hour to test #9300? I have a hunch most of the major fixes have been pushed and I think this PR has some unecessary complexity. I suspect this LD wrapper isn't necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I am working on google/fuzzbench#1632 now.

eamonnmcmanus pushed a commit to eamonnmcmanus/oss-fuzz that referenced this pull request Mar 15, 2023
@DonggeLiu DonggeLiu closed this May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants