From dc1cf3580e2ec7dd164c95cec2f5568beaf3324b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 7 Nov 2022 22:23:10 +0100 Subject: [PATCH] Makefile & test-tool: replace "DC_SHA1" variable with a "define" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the root cause of technical debt we've been carrying since sha1collisiondetection was made the default in [1]. In a preceding commit we narrowly fixed a bug where the "DC_SHA1" variable would be unset (in combination with "NO_APPLE_COMMON_CRYPTO=" on OSX), even though we had the sha1collisiondetection library enabled. But the only reason we needed to have such a user-exposed knob went away with [1], and it's been doing nothing useful since then. We don't care if you define DC_SHA1=*, we only care that you don't ask for any other SHA-1 implementation. If it turns out that you didn't, we'll use sha1collisiondetection, whether you had "DC_SHA1" set or not. As a result of this being confusing we had e.g. [2] for cmake and the recent [3] for ci/lib.sh setting "DC_SHA1" explicitly, even though this was always a NOOP. A much simpler way to do this is to stop having the Makefile and CMakeLists.txt set "DC_SHA1" to be picked up by the test-lib.sh, let's instead add a trivial "test-tool sha1-is-sha1dc". It returns zero if we're using sha1collisiondetection, non-zero otherwise. 1. e6b07da2780 (Makefile: make DC_SHA1 the default, 2017-03-17) 2. c4b2f41b5f5 (cmake: support for testing git with ctest, 2020-06-26) 3. 1ad5c3df35a (ci: use DC_SHA1=YesPlease on osx-clang job for CI, 2022-10-20) Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- Makefile | 8 ++++---- ci/lib.sh | 2 +- contrib/buildsystems/CMakeLists.txt | 2 -- sha1dc_git.h | 1 + t/helper/test-sha1.c | 8 ++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0013-sha1dc.sh | 6 ++++-- 8 files changed, 20 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 9b5f872d107..0c79546712f 100644 --- a/Makefile +++ b/Makefile @@ -500,8 +500,10 @@ include shared.mak # Define BLK_SHA1 to make use of optimized C SHA-1 routines bundled # with git (in the block-sha1/ directory). # -# Define DC_SHA1 to enable the collision-detecting sha1 -# algorithm. This is slower, but may detect attempted collision attacks. +# If don't enable any of the *_SHA1 settings in this section, Git will +# default to its built-in sha1collisiondetection library, which is a +# collision-detecting sha1 This is slower, but may detect attempted +# collision attacks. # # ==== Options for the sha1collisiondetection library ==== # @@ -1867,7 +1869,6 @@ ifdef APPLE_COMMON_CRYPTO COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL BASIC_CFLAGS += -DSHA1_APPLE else - override DC_SHA1 = YesPlease BASIC_CFLAGS += -DSHA1_DC LIB_OBJS += sha1dc_git.o ifdef DC_SHA1_EXTERNAL @@ -3030,7 +3031,6 @@ GIT-BUILD-OPTIONS: FORCE @echo NO_REGEX=\''$(subst ','\'',$(subst ','\'',$(NO_REGEX)))'\' >>$@+ @echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+ @echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+ - @echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+ @echo SANITIZE_LEAK=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_LEAK)))'\' >>$@+ @echo SANITIZE_ADDRESS=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_ADDRESS)))'\' >>$@+ @echo X=\'$(X)\' >>$@+ diff --git a/ci/lib.sh b/ci/lib.sh index 1808e3b1ce1..24d20a5d648 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -260,7 +260,7 @@ macos-latest) else MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python2)" MAKEFLAGS="$MAKEFLAGS NO_APPLE_COMMON_CRYPTO=NoThanks" - MAKEFLAGS="$MAKEFLAGS DC_SHA1=YesPlease NO_OPENSSL=NoThanks" + MAKEFLAGS="$MAKEFLAGS NO_OPENSSL=NoThanks" fi ;; esac diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 3957e4cf8cd..2f6e0197ffa 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -1025,7 +1025,6 @@ set(NO_PERL ) set(NO_PTHREADS ) set(NO_PYTHON ) set(PAGER_ENV "LESS=FRX LV=-c") -set(DC_SHA1 YesPlease) set(RUNTIME_PREFIX true) set(NO_GETTEXT ) @@ -1061,7 +1060,6 @@ file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PERL='${NO_PERL}'\n") file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PTHREADS='${NO_PTHREADS}'\n") file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_UNIX_SOCKETS='${NO_UNIX_SOCKETS}'\n") file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PAGER_ENV='${PAGER_ENV}'\n") -file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "DC_SHA1='${DC_SHA1}'\n") file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "X='${EXE_EXTENSION}'\n") file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n") file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "RUNTIME_PREFIX='${RUNTIME_PREFIX}'\n") diff --git a/sha1dc_git.h b/sha1dc_git.h index 41e1c3fd3f7..60e3ce84395 100644 --- a/sha1dc_git.h +++ b/sha1dc_git.h @@ -17,6 +17,7 @@ void git_SHA1DCInit(SHA1_CTX *); void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *); void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len); +#define platform_SHA_IS_SHA1DC /* used by "test-tool sha1-is-sha1dc" */ #define platform_SHA_CTX SHA1_CTX #define platform_SHA1_Init git_SHA1DCInit #define platform_SHA1_Update git_SHA1DCUpdate diff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c index d860c387c38..71fe5c61455 100644 --- a/t/helper/test-sha1.c +++ b/t/helper/test-sha1.c @@ -5,3 +5,11 @@ int cmd__sha1(int ac, const char **av) { return cmd_hash_impl(ac, av, GIT_HASH_SHA1); } + +int cmd__sha1_is_sha1dc(int argc UNUSED, const char **argv UNUSED) +{ +#ifdef platform_SHA_IS_SHA1DC + return 0; +#endif + return 1; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 01cda9358df..775854c9f96 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -73,6 +73,7 @@ static struct test_cmd cmds[] = { { "scrap-cache-tree", cmd__scrap_cache_tree }, { "serve-v2", cmd__serve_v2 }, { "sha1", cmd__sha1 }, + { "sha1-is-sha1dc", cmd__sha1_is_sha1dc }, { "sha256", cmd__sha256 }, { "sigchain", cmd__sigchain }, { "simple-ipc", cmd__simple_ipc }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index ca2948066fd..7cf84eca12e 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -66,6 +66,7 @@ int cmd__run_command(int argc, const char **argv); int cmd__scrap_cache_tree(int argc, const char **argv); int cmd__serve_v2(int argc, const char **argv); int cmd__sha1(int argc, const char **argv); +int cmd__sha1_is_sha1dc(int argc, const char **argv); int cmd__oid_array(int argc, const char **argv); int cmd__sha256(int argc, const char **argv); int cmd__sigchain(int argc, const char **argv); diff --git a/t/t0013-sha1dc.sh b/t/t0013-sha1dc.sh index 9ad76080aa4..53240476896 100755 --- a/t/t0013-sha1dc.sh +++ b/t/t0013-sha1dc.sh @@ -6,9 +6,11 @@ TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh TEST_DATA="$TEST_DIRECTORY/t0013" -if test -z "$DC_SHA1" +test_lazy_prereq SHA1_IS_SHA1DC 'test-tool sha1-is-sha1dc' + +if ! test_have_prereq SHA1_IS_SHA1DC then - skip_all='skipping sha1 collision tests, DC_SHA1 not set' + skip_all='skipping sha1 collision tests, not using sha1collisiondetection' test_done fi