{"thread":{"id":"60770","subject":"[PATCH 0/2] Run limited fuzz tests in GitHub CI","startedAt":"2024-01-19T21:38:15Z","lastAt":"2024-03-04T18:57:43Z","messageCount":6,"participants":["Josh Steadmon","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"487109","messageId":"cover.1705700054.git.steadmon@google.com","threadId":"60770","inReplyTo":null,"subject":"[PATCH 0/2] Run limited fuzz tests in GitHub CI","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-01-19T21:38:11Z","receivedAt":"2024-01-19T21:38:15Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Add a simple smoke test in CI to make sure that the fuzz tests can build\nand execute properly. While we already compile the fuzz-test objects in\nthe default make target, we don't link the executables due to these\nrequiring clang-specific support. However, this means that the fuzz\ntests have been vulnerable to unnoticed build breakages as the code that\nthey link against has changed over time.\n\nAdding this CI test should make such build breakages more visible more\nquickly.\n\n\nJosh Steadmon (2):\n  fuzz: fix fuzz test build rules\n  ci: build and run minimal fuzzers in GitHub CI\n\n .github/workflows/main.yml          | 11 +++++++++++\n Makefile                            | 17 +++++++++++------\n ci/run-build-and-minimal-fuzzers.sh | 19 +++++++++++++++++++\n oss-fuzz/dummy-cmd-main.c           | 14 ++++++++++++++\n 4 files changed, 55 insertions(+), 6 deletions(-)\n create mode 100755 ci/run-build-and-minimal-fuzzers.sh\n create mode 100644 oss-fuzz/dummy-cmd-main.c\n\n\nbase-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\n-- \n2.43.0.429.g432eaa2c6b-goog\n\n"},{"id":"487110","messageId":"9332e225e44b29be25d10229b05f0b9775b85568.1705700054.git.steadmon@google.com","threadId":"60770","inReplyTo":"cover.1705700054.git.steadmon@google.com","subject":"[PATCH 1/2] fuzz: fix fuzz test build rules","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-01-19T21:38:12Z","receivedAt":"2024-01-19T21:38:17Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When we originally added the fuzz tests in 5e47215080 (fuzz: add basic\nfuzz testing target., 2018-10-12), we went to some trouble to create a\nMakefile rule that allowed linking the fuzz executables without pulling\nin common-main.o. This was necessary to prevent the\nfuzzing-engine-provided main() from clashing with Git's main().\n\nHowever, since 19d75948ef (common-main.c: move non-trace2 exit()\nbehavior out of trace2.c, 2022-06-02), it has been necessary to link\ncommon-main.o due to moving the common_exit() function to that file.\nÆvar suggested a set of compiler flags to allow this in [1], but this\nwas never reflected in the Makefile.\n\nSince we now must include common-main.o, there's no reason to pick and\nchoose a subset of object files to link, so simplify the Makefile rule\nfor the fuzzer executables to just use libgit.a. While we're at it,\ninclude the necessary linker flag to allow multiple definitions\ndirectly in the Makefile rule, rather than requiring it to be passed on\nthe command-line each time. This means the Makefile rule as written is\nnow more compiler-specific, but this was already the case for the\nfuzzers themselves anyway.\n\n[1] https://lore.kernel.org/git/220607.8635ggupws.gmgdl@evledraar.gmail.com/\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\n Makefile                  | 14 ++++++++------\n oss-fuzz/dummy-cmd-main.c | 14 ++++++++++++++\n 2 files changed, 22 insertions(+), 6 deletions(-)\n create mode 100644 oss-fuzz/dummy-cmd-main.c\n\ndiff --git a/Makefile b/Makefile\nindex 15990ff312..1e9bd6430f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -752,6 +752,7 @@ SCRIPTS = $(SCRIPT_SH_GEN) \\\n \n ETAGS_TARGET = TAGS\n \n+FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o\n FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o\n FUZZ_OBJS += oss-fuzz/fuzz-date.o\n FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o\n@@ -762,7 +763,7 @@ fuzz-objs: $(FUZZ_OBJS)\n # Always build fuzz objects even if not testing, to prevent bit-rot.\n all:: $(FUZZ_OBJS)\n \n-FUZZ_PROGRAMS += $(patsubst %.o,%,$(FUZZ_OBJS))\n+FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n \n # Empty...\n EXTRA_PROGRAMS =\n@@ -3850,16 +3851,17 @@ cover_db_html: cover_db\n #\n # make CC=clang CXX=clang++ \\\n #      CFLAGS=\"-fsanitize=fuzzer-no-link,address\" \\\n-#      LIB_FUZZING_ENGINE=\"-fsanitize=fuzzer\" \\\n+#      LIB_FUZZING_ENGINE=\"-fsanitize=fuzzer,address\" \\\n #      fuzz-all\n #\n-FUZZ_CXXFLAGS ?= $(CFLAGS)\n+FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)\n \n .PHONY: fuzz-all\n \n-$(FUZZ_PROGRAMS): all\n-\t$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) $(LIB_OBJS) $(BUILTIN_OBJS) \\\n-\t\t$(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@\n+$(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS\n+\t$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \\\n+\t\t-Wl,--allow-multiple-definition \\\n+\t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)\n \n fuzz-all: $(FUZZ_PROGRAMS)\n \ndiff --git a/oss-fuzz/dummy-cmd-main.c b/oss-fuzz/dummy-cmd-main.c\nnew file mode 100644\nindex 0000000000..071cb231ba\n--- /dev/null\n+++ b/oss-fuzz/dummy-cmd-main.c\n@@ -0,0 +1,14 @@\n+#include \"git-compat-util.h\"\n+\n+/*\n+ * When linking the fuzzers, we link against common-main.o to pick up some\n+ * symbols. However, even though we ignore common-main:main(), we still need to\n+ * provide all the symbols it references. In the fuzzers' case, we need to\n+ * provide a dummy cmd_main() for the linker to be happy. It will never be\n+ * executed.\n+ */\n+\n+int cmd_main(int argc, const char **argv) {\n+\tBUG(\"We should not execute cmd_main() from a fuzz target\");\n+\treturn 1;\n+}\n-- \n2.43.0.429.g432eaa2c6b-goog\n\n"},{"id":"487111","messageId":"eb38274459cde83a22745172a1ed1d20aebc58a7.1705700054.git.steadmon@google.com","threadId":"60770","inReplyTo":"cover.1705700054.git.steadmon@google.com","subject":"[PATCH 2/2] ci: build and run minimal fuzzers in GitHub CI","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-01-19T21:38:13Z","receivedAt":"2024-01-19T21:38:19Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"To prevent bitrot, we would like to regularly exercise the fuzz tests in\norder to make sure they still link & run properly. We already compile\nthe fuzz test objects as part of the default `make` target, but we do\nnot link the executables due to the fuzz tests needing specific\ncompilers and compiler features. This has lead to frequent build\nbreakages for the fuzz tests.\n\nTo remedy this, we can add a CI step to actually link the fuzz\nexecutables, and run them (with finite input rather than the default\ninfinite random input mode) to verify that they execute properly.\n\nSince the main use of the fuzz tests is via OSS-Fuzz [1], and OSS-Fuzz\nonly runs tests on Linux [2], we only set up a CI test for the fuzzers\non Linux.\n\n[1] https://github.com/google/oss-fuzz\n[2] https://google.github.io/oss-fuzz/further-reading/fuzzer-environment/\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\n .github/workflows/main.yml          | 11 +++++++++++\n Makefile                            |  3 +++\n ci/run-build-and-minimal-fuzzers.sh | 19 +++++++++++++++++++\n 3 files changed, 33 insertions(+)\n create mode 100755 ci/run-build-and-minimal-fuzzers.sh\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 9fdbd54028..4d97da57ec 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -309,6 +309,17 @@ jobs:\n       with:\n         name: failed-tests-${{matrix.vector.jobname}}\n         path: ${{env.FAILED_TEST_ARTIFACTS}}\n+  fuzz-smoke-test:\n+    name: fuzz smoke test\n+    needs: ci-config\n+    if: needs.ci-config.outputs.enabled == 'yes'\n+    env:\n+      CC: clang\n+    runs-on: ubuntu-latest\n+    steps:\n+    - uses: actions/checkout@v3\n+    - run: ci/install-dependencies.sh\n+    - run: ci/run-build-and-minimal-fuzzers.sh\n   dockerized:\n     name: ${{matrix.vector.jobname}} (${{matrix.vector.image}})\n     needs: ci-config\ndiff --git a/Makefile b/Makefile\nindex 1e9bd6430f..2e94c566e0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -752,6 +752,9 @@ SCRIPTS = $(SCRIPT_SH_GEN) \\\n \n ETAGS_TARGET = TAGS\n \n+# If you add a new fuzzer, please also make sure to run it in\n+# ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and\n+# runs in the future.\n FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o\n FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o\n FUZZ_OBJS += oss-fuzz/fuzz-date.o\ndiff --git a/ci/run-build-and-minimal-fuzzers.sh b/ci/run-build-and-minimal-fuzzers.sh\nnew file mode 100755\nindex 0000000000..8ba486f659\n--- /dev/null\n+++ b/ci/run-build-and-minimal-fuzzers.sh\n@@ -0,0 +1,19 @@\n+#!/bin/sh\n+#\n+# Build and test Git's fuzzers\n+#\n+\n+. ${0%/*}/lib.sh\n+\n+group \"Build fuzzers\" make \\\n+\tCC=clang \\\n+\tCXX=clang++ \\\n+\tCFLAGS=\"-fsanitize=fuzzer-no-link,address\" \\\n+\tLIB_FUZZING_ENGINE=\"-fsanitize=fuzzer,address\" \\\n+\tfuzz-all\n+\n+for fuzzer in commit-graph date pack-headers pack-idx ; do\n+\tbegin_group \"fuzz-$fuzzer\"\n+\t./oss-fuzz/fuzz-$fuzzer -verbosity=0 -runs=1 || exit 1\n+\tend_group \"fuzz-$fuzzer\"\n+done\n-- \n2.43.0.429.g432eaa2c6b-goog\n\n"},{"id":"487113","messageId":"xmqqil3phs9p.fsf@gitster.g","threadId":"60770","inReplyTo":"cover.1705700054.git.steadmon@google.com","subject":"Re: [PATCH 0/2] Run limited fuzz tests in GitHub CI","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-19T22:28:50Z","receivedAt":"2024-01-19T22:28:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> Add a simple smoke test in CI to make sure that the fuzz tests can build\n> and execute properly. While we already compile the fuzz-test objects in\n> the default make target, we don't link the executables due to these\n> requiring clang-specific support. However, this means that the fuzz\n> tests have been vulnerable to unnoticed build breakages as the code that\n> they link against has changed over time.\n>\n> Adding this CI test should make such build breakages more visible more\n> quickly.\n\nNice.\n\n> Josh Steadmon (2):\n>   fuzz: fix fuzz test build rules\n>   ci: build and run minimal fuzzers in GitHub CI\n>\n>  .github/workflows/main.yml          | 11 +++++++++++\n>  Makefile                            | 17 +++++++++++------\n>  ci/run-build-and-minimal-fuzzers.sh | 19 +++++++++++++++++++\n>  oss-fuzz/dummy-cmd-main.c           | 14 ++++++++++++++\n>  4 files changed, 55 insertions(+), 6 deletions(-)\n>  create mode 100755 ci/run-build-and-minimal-fuzzers.sh\n>  create mode 100644 oss-fuzz/dummy-cmd-main.c\n>\n>\n> base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\n"},{"id":"487114","messageId":"xmqqcytxhrgs.fsf@gitster.g","threadId":"60770","inReplyTo":"9332e225e44b29be25d10229b05f0b9775b85568.1705700054.git.steadmon@google.com","subject":"Re: [PATCH 1/2] fuzz: fix fuzz test build rules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-19T22:46:11Z","receivedAt":"2024-01-19T22:46:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> @@ -762,7 +763,7 @@ fuzz-objs: $(FUZZ_OBJS)\n>  # Always build fuzz objects even if not testing, to prevent bit-rot.\n>  all:: $(FUZZ_OBJS)\n\nSo, this is what you referred to in your proposed log message.  We\ndo build objects to prevent bit-rot, but we do not link, so it is\nmerely half a protection.\n\n> ...\n>  fuzz-all: $(FUZZ_PROGRAMS)\n\nBut there is this target.  I wonder if it makes it even better to\nupdate the \"always build fuzz objects\" one?  Given that some folks\nmay not have the necessary clang toochain for linking, it may\nprobably be a bit too much, perhaps?\n\nIt definitely is an improvement to build them in the CI environment,\nlike you have in [2/2].\n\nThanks.  Will queue.\n\n\n"},{"id":"489915","messageId":"ZeYZolhLCftYALYU@google.com","threadId":"60770","inReplyTo":"xmqqcytxhrgs.fsf@gitster.g","subject":"Re: [PATCH 1/2] fuzz: fix fuzz test build rules","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-04T18:57:38Z","receivedAt":"2024-03-04T18:57:43Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.01.19 14:46, Junio C Hamano wrote:\n> Josh Steadmon <steadmon@google.com> writes:\n> \n> > @@ -762,7 +763,7 @@ fuzz-objs: $(FUZZ_OBJS)\n> >  # Always build fuzz objects even if not testing, to prevent bit-rot.\n> >  all:: $(FUZZ_OBJS)\n> \n> So, this is what you referred to in your proposed log message.  We\n> do build objects to prevent bit-rot, but we do not link, so it is\n> merely half a protection.\n> \n> > ...\n> >  fuzz-all: $(FUZZ_PROGRAMS)\n> \n> But there is this target.  I wonder if it makes it even better to\n> update the \"always build fuzz objects\" one?  Given that some folks\n> may not have the necessary clang toochain for linking, it may\n> probably be a bit too much, perhaps?\n\nIndeed, this would have caught the previous common-main issue. I'll send\na followup to fix this, thanks for the suggestion!\n"}]}