| Message ID | eef15e3d3da3ca6953fa8bf3ade190da8e68bf46.1709673020.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuzz: build fuzzers by default on Linux | expand |
Josh Steadmon <steadmon@google.com> writes: > Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we > have compiled object files for the fuzz tests as part of the default > 'make all' target. This helps prevent bit-rot in lesser-used parts of > the codebase, by making sure that incompatible changes are caught at > build time. > > However, since we never linked the fuzzer executables, this did not > protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test > build rules, 2024-01-19), it's now possible to link the fuzzer > executables without using a fuzzing engine and a variety of > compiler-specific (and compiler-version-specific) flags, at least on > Linux. So let's add a platform-specific option in config.mak.uname to > link the executables as part of the default `make all` target. > > Suggested-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Josh Steadmon <steadmon@google.com> > --- > Makefile | 14 +++++++++++--- > config.mak.uname | 1 + > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/Makefile b/Makefile > index 4e255c81f2..f74e96d7c2 100644 > --- a/Makefile > +++ b/Makefile > @@ -409,6 +409,9 @@ include shared.mak > # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c` > # that implements the `fsm_os_settings__*()` routines. > # > +# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test > +# programs in oss-fuzz/. > +# > # === Optional library: libintl === > # > # Define NO_GETTEXT if you don't want Git output to be translated. > @@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o > .PHONY: fuzz-objs > fuzz-objs: $(FUZZ_OBJS) > > -# Always build fuzz objects even if not testing, to prevent bit-rot. > -all:: $(FUZZ_OBJS) > - > FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS))) > > # Empty... > @@ -2368,6 +2368,14 @@ ifndef NO_TCLTK > endif > $(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)' > > +# Build fuzz programs if possible, or at least compile the object files; even > +# without the necessary fuzzing support, this prevents bit-rot. > +ifdef LINK_FUZZ_PROGRAMS > +all:: $(FUZZ_PROGRAMS) > +else > +all:: $(FUZZ_OBJS) > +endif It would have been easier on the eyes if we had the fuzz things together, perhaps like this simplified version? We build FUZZ_OBJS either way, and when the LINK_FUZZ_PROGRAMS is requested, we follow the fuzz-all recipe, too. diff --git c/Makefile w/Makefile index 4e255c81f2..46e457a7a8 100644 --- c/Makefile +++ w/Makefile @@ -409,6 +409,9 @@ include shared.mak # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c` # that implements the `fsm_os_settings__*()` routines. # +# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test +# programs in oss-fuzz/. +# # === Optional library: libintl === # # Define NO_GETTEXT if you don't want Git output to be translated. @@ -766,6 +769,12 @@ fuzz-objs: $(FUZZ_OBJS) # Always build fuzz objects even if not testing, to prevent bit-rot. all:: $(FUZZ_OBJS) +# Build fuzz programs, even without the necessary fuzzing support, +# this prevents bit-rot. +ifdef LINK_FUZZ_PROGRAMS +all:: fuzz-all +endif + FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS))) # Empty...
On 2024.03.05 13:44, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > > Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we > > have compiled object files for the fuzz tests as part of the default > > 'make all' target. This helps prevent bit-rot in lesser-used parts of > > the codebase, by making sure that incompatible changes are caught at > > build time. > > > > However, since we never linked the fuzzer executables, this did not > > protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test > > build rules, 2024-01-19), it's now possible to link the fuzzer > > executables without using a fuzzing engine and a variety of > > compiler-specific (and compiler-version-specific) flags, at least on > > Linux. So let's add a platform-specific option in config.mak.uname to > > link the executables as part of the default `make all` target. > > > > Suggested-by: Junio C Hamano <gitster@pobox.com> > > Signed-off-by: Josh Steadmon <steadmon@google.com> > > --- > > Makefile | 14 +++++++++++--- > > config.mak.uname | 1 + > > 2 files changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index 4e255c81f2..f74e96d7c2 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -409,6 +409,9 @@ include shared.mak > > # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c` > > # that implements the `fsm_os_settings__*()` routines. > > # > > +# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test > > +# programs in oss-fuzz/. > > +# > > # === Optional library: libintl === > > # > > # Define NO_GETTEXT if you don't want Git output to be translated. > > @@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o > > .PHONY: fuzz-objs > > fuzz-objs: $(FUZZ_OBJS) > > > > -# Always build fuzz objects even if not testing, to prevent bit-rot. > > -all:: $(FUZZ_OBJS) > > - > > FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS))) > > > > # Empty... > > @@ -2368,6 +2368,14 @@ ifndef NO_TCLTK > > endif > > $(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)' > > > > +# Build fuzz programs if possible, or at least compile the object files; even > > +# without the necessary fuzzing support, this prevents bit-rot. > > +ifdef LINK_FUZZ_PROGRAMS > > +all:: $(FUZZ_PROGRAMS) > > +else > > +all:: $(FUZZ_OBJS) > > +endif > > It would have been easier on the eyes if we had the fuzz things > together, perhaps like this simplified version? We build FUZZ_OBJS > either way, and when the LINK_FUZZ_PROGRAMS is requested, we follow > the fuzz-all recipe, too. We need the LINK_FUZZ_PROGRAMS conditional to happen after we import config.mak.uname (line 1434 in my V1). We also need to define FUZZ_OBJS prior to adding it to OBJECTS (line 2698 in V1). I can move all of the fuzz-definition within that range, keeping everything in one place at the cost of a larger diff. I'll do that for V2, but if you prefer otherwise please let me know. Although I'm not 100% sure that we even need to add FUZZ_OBJS to OBJECTS, so let me check that tomorrow. If not, then I can move everything to the bottom of the Makefile where we also define fuzz-all and the build rules for FUZZ_PROGRAMS. > diff --git c/Makefile w/Makefile > index 4e255c81f2..46e457a7a8 100644 > --- c/Makefile > +++ w/Makefile > @@ -409,6 +409,9 @@ include shared.mak > # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c` > # that implements the `fsm_os_settings__*()` routines. > # > +# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test > +# programs in oss-fuzz/. > +# > # === Optional library: libintl === > # > # Define NO_GETTEXT if you don't want Git output to be translated. > @@ -766,6 +769,12 @@ fuzz-objs: $(FUZZ_OBJS) > # Always build fuzz objects even if not testing, to prevent bit-rot. > all:: $(FUZZ_OBJS) > > +# Build fuzz programs, even without the necessary fuzzing support, > +# this prevents bit-rot. > +ifdef LINK_FUZZ_PROGRAMS > +all:: fuzz-all > +endif > + > FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS))) > > # Empty...
On 2024.04.09 14:58, Josh Steadmon wrote: > On 2024.03.05 13:44, Junio C Hamano wrote: > > Josh Steadmon <steadmon@google.com> writes: > > > > > Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we > > > have compiled object files for the fuzz tests as part of the default > > > 'make all' target. This helps prevent bit-rot in lesser-used parts of > > > the codebase, by making sure that incompatible changes are caught at > > > build time. > > > > > > However, since we never linked the fuzzer executables, this did not > > > protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test > > > build rules, 2024-01-19), it's now possible to link the fuzzer > > > executables without using a fuzzing engine and a variety of > > > compiler-specific (and compiler-version-specific) flags, at least on > > > Linux. So let's add a platform-specific option in config.mak.uname to > > > link the executables as part of the default `make all` target. > > > > > > Suggested-by: Junio C Hamano <gitster@pobox.com> > > > Signed-off-by: Josh Steadmon <steadmon@google.com> > > > --- > > > Makefile | 14 +++++++++++--- > > > config.mak.uname | 1 + > > > 2 files changed, 12 insertions(+), 3 deletions(-) > > > > > > diff --git a/Makefile b/Makefile > > > index 4e255c81f2..f74e96d7c2 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -409,6 +409,9 @@ include shared.mak > > > # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c` > > > # that implements the `fsm_os_settings__*()` routines. > > > # > > > +# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test > > > +# programs in oss-fuzz/. > > > +# > > > # === Optional library: libintl === > > > # > > > # Define NO_GETTEXT if you don't want Git output to be translated. > > > @@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o > > > .PHONY: fuzz-objs > > > fuzz-objs: $(FUZZ_OBJS) > > > > > > -# Always build fuzz objects even if not testing, to prevent bit-rot. > > > -all:: $(FUZZ_OBJS) > > > - > > > FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS))) > > > > > > # Empty... > > > @@ -2368,6 +2368,14 @@ ifndef NO_TCLTK > > > endif > > > $(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)' > > > > > > +# Build fuzz programs if possible, or at least compile the object files; even > > > +# without the necessary fuzzing support, this prevents bit-rot. > > > +ifdef LINK_FUZZ_PROGRAMS > > > +all:: $(FUZZ_PROGRAMS) > > > +else > > > +all:: $(FUZZ_OBJS) > > > +endif > > > > It would have been easier on the eyes if we had the fuzz things > > together, perhaps like this simplified version? We build FUZZ_OBJS > > either way, and when the LINK_FUZZ_PROGRAMS is requested, we follow > > the fuzz-all recipe, too. > > We need the LINK_FUZZ_PROGRAMS conditional to happen after we import > config.mak.uname (line 1434 in my V1). We also need to define FUZZ_OBJS > prior to adding it to OBJECTS (line 2698 in V1). I can move all of the > fuzz-definition within that range, keeping everything in one place at > the cost of a larger diff. I'll do that for V2, but if you prefer > otherwise please let me know. > > Although I'm not 100% sure that we even need to add FUZZ_OBJS to > OBJECTS, so let me check that tomorrow. If not, then I can move > everything to the bottom of the Makefile where we also define fuzz-all > and the build rules for FUZZ_PROGRAMS. It turns out we do need FUZZ_OBJS in OBJECTS so that we define a build rule, otherwise the Makefile doesn't know how to compile the fuzzer objects. So V2 will have most of the fuzzer rules in the line (1434,2698) range.
Josh Steadmon <steadmon@google.com> writes: > It turns out we do need FUZZ_OBJS in OBJECTS so that we define a build > rule, otherwise the Makefile doesn't know how to compile the fuzzer > objects. So V2 will have most of the fuzzer rules in the line > (1434,2698) range. The reasoning and the conclusion sound sensible. Thanks.
On Tue, Apr 09, 2024 at 02:58:28PM -0700, Josh Steadmon wrote: > > It would have been easier on the eyes if we had the fuzz things > > together, perhaps like this simplified version? We build FUZZ_OBJS > > either way, and when the LINK_FUZZ_PROGRAMS is requested, we follow > > the fuzz-all recipe, too. > > We need the LINK_FUZZ_PROGRAMS conditional to happen after we import > config.mak.uname (line 1434 in my V1). We also need to define FUZZ_OBJS > prior to adding it to OBJECTS (line 2698 in V1). I can move all of the > fuzz-definition within that range, keeping everything in one place at > the cost of a larger diff. I'll do that for V2, but if you prefer > otherwise please let me know. > > Although I'm not 100% sure that we even need to add FUZZ_OBJS to > OBJECTS, so let me check that tomorrow. If not, then I can move > everything to the bottom of the Makefile where we also define fuzz-all > and the build rules for FUZZ_PROGRAMS. The conditional has to be read handled while reading the Makefile, but as a "simple" variable, OBJECTS isn't expanded until the whole Makefile has been read. So for example this out-of-order definition works: diff --git a/Makefile b/Makefile index 533eaae612..5dbf1935a1 100644 --- a/Makefile +++ b/Makefile @@ -755,6 +755,7 @@ ETAGS_TARGET = TAGS # If you add a new fuzzer, please also make sure to run it in # ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and # runs in the future. +OBJECTS += $(FUZZ_OBJS) FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o FUZZ_OBJS += oss-fuzz/fuzz-config.o @@ -2695,7 +2696,6 @@ OBJECTS += $(SCALAR_OBJS) OBJECTS += $(PROGRAM_OBJS) OBJECTS += $(TEST_OBJS) OBJECTS += $(XDIFF_OBJS) -OBJECTS += $(FUZZ_OBJS) OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS) OBJECTS += $(UNIT_TEST_OBJS) Now whether that is useful for organizing the Makefile, I don't know, but I thought I'd throw it out there in case it helps you. -Peff
diff --git a/Makefile b/Makefile index 4e255c81f2..f74e96d7c2 100644 --- a/Makefile +++ b/Makefile @@ -409,6 +409,9 @@ include shared.mak # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c` # that implements the `fsm_os_settings__*()` routines. # +# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test +# programs in oss-fuzz/. +# # === Optional library: libintl === # # Define NO_GETTEXT if you don't want Git output to be translated. @@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o .PHONY: fuzz-objs fuzz-objs: $(FUZZ_OBJS) -# Always build fuzz objects even if not testing, to prevent bit-rot. -all:: $(FUZZ_OBJS) - FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS))) # Empty... @@ -2368,6 +2368,14 @@ ifndef NO_TCLTK endif $(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)' +# Build fuzz programs if possible, or at least compile the object files; even +# without the necessary fuzzing support, this prevents bit-rot. +ifdef LINK_FUZZ_PROGRAMS +all:: $(FUZZ_PROGRAMS) +else +all:: $(FUZZ_OBJS) +endif + please_set_SHELL_PATH_to_a_more_modern_shell: @$$(:) diff --git a/config.mak.uname b/config.mak.uname index dacc95172d..6579c36a99 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -68,6 +68,7 @@ ifeq ($(uname_S),Linux) ifneq ($(findstring .el7.,$(uname_R)),) BASIC_CFLAGS += -std=c99 endif + LINK_FUZZ_PROGRAMS = YesPlease endif ifeq ($(uname_S),GNU/kFreeBSD) HAVE_ALLOCA_H = YesPlease
Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we have compiled object files for the fuzz tests as part of the default 'make all' target. This helps prevent bit-rot in lesser-used parts of the codebase, by making sure that incompatible changes are caught at build time. However, since we never linked the fuzzer executables, this did not protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test build rules, 2024-01-19), it's now possible to link the fuzzer executables without using a fuzzing engine and a variety of compiler-specific (and compiler-version-specific) flags, at least on Linux. So let's add a platform-specific option in config.mak.uname to link the executables as part of the default `make all` target. Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Josh Steadmon <steadmon@google.com> --- Makefile | 14 +++++++++++--- config.mak.uname | 1 + 2 files changed, 12 insertions(+), 3 deletions(-)