{"thread":{"id":"62713","subject":"[PATCH 00/10] Fix segfaults when using the unsafe SHA1 backend","startedAt":"2024-12-30T14:24:10Z","lastAt":"2025-01-09T05:58:05Z","messageCount":19,"participants":["Patrick Steinhardt","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"509734","messageId":"20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im","threadId":"62713","inReplyTo":null,"subject":"[PATCH 00/10] Fix segfaults when using the unsafe SHA1 backend","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:24:00Z","receivedAt":"2024-12-30T14:24:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series fixes two segfaults when using the unsafe SHA1\nbackend. The segfaults are caused by us mixing safe and unsafe variants\nof the API. As such, it only surfaces when actually building Git with an\nunsafe backend that is different from the safe one.\n\nI noticed those errors while wiring up the build option in the Meson\nbuild system. As such, this patch series doesn't only fix the segfaults,\nbut also includes patches for the Meson infrastructure and is structured\nas follows:\n\n  - Patches 1 and 2 fix the segfaults.\n\n  - Patch 3 makes the segfaults visible via our CI.\n\n  - Patches 4 to 10 wire up the build infrastructure in Meson.\n\nGiven that we're close to the release of Git v2.48 I'd be fine with\nmerging just the first three patches. I can then re-send the Meson stuff\nearly in the next development cycle.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (10):\n      bulk-checkin: fix segfault with unsafe SHA1 backend\n      builtin/fast-import: fix segfault with unsafe SHA1 backend\n      ci: exercise unsafe OpenSSL backend\n      meson: consistenlty spell 'CommonCrypto'\n      meson: deduplicate access to SHA1/SHA256 backend options\n      meson: require SecurityFramework when it's used as SHA1 backend\n      meson: simplify conditions for HTTPS and SHA1 dependencies\n      meson: add missing dots for build options\n      meson: wire up unsafe SHA1 backend\n      meson: provide a summary of configured backends\n\n builtin/fast-import.c     |  2 +-\n bulk-checkin.c            |  2 +-\n ci/run-build-and-tests.sh |  1 +\n meson.build               | 51 ++++++++++++++++++++++++++++++++++++-----------\n meson_options.txt         |  8 +++++---\n 5 files changed, 47 insertions(+), 17 deletions(-)\n\n\n---\nbase-commit: 306ab352f4e98f6809ce52fc4e5d63fb947d0635\nchange-id: 20241230-pks-meson-sha1-unsafe-03c785824eee\n\n"},{"id":"509735","messageId":"20241230-pks-meson-sha1-unsafe-v1-2-efb276e171f5@pks.im","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im","subject":"[PATCH 02/10] builtin/fast-import: fix segfault with unsafe SHA1 backend","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:24:02Z","receivedAt":"2024-12-30T14:24:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Same as with the preceding commit, git-fast-import(1) is using the safe\nvariant to initialize a hashfile checkpoint. This leads to a segfault\nwhen passing the checkpoint into the hashfile subsystem because it would\nuse the unsafe variants instead:\n\n    ++ git --git-dir=R/.git fast-import --big-file-threshold=1\n    AddressSanitizer:DEADLYSIGNAL\n    =================================================================\n    ==577126==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000040 (pc 0x7ffff7a01a99 bp 0x5070000009c0 sp 0x7fffffff5b30 T0)\n    ==577126==The signal is caused by a READ memory access.\n    ==577126==Hint: address points to the zero page.\n        #0 0x7ffff7a01a99 in EVP_MD_CTX_copy_ex (/nix/store/h1ydpxkw9qhjdxjpic1pdc2nirggyy6f-openssl-3.3.2/lib/libcrypto.so.3+0x201a99) (BuildId: 41746a580d39075fc85e8c8065b6c07fb34e97d4)\n        #1 0x555555ddde56 in openssl_SHA1_Clone ../sha1/openssl.h:40:2\n        #2 0x555555dce2fc in git_hash_sha1_clone_unsafe ../object-file.c:123:2\n        #3 0x555555c2d5f8 in hashfile_checkpoint ../csum-file.c:211:2\n        #4 0x5555559647d1 in stream_blob ../builtin/fast-import.c:1110:2\n        #5 0x55555596247b in parse_and_store_blob ../builtin/fast-import.c:2031:3\n        #6 0x555555967f91 in file_change_m ../builtin/fast-import.c:2408:5\n        #7 0x55555595d8a2 in parse_new_commit ../builtin/fast-import.c:2768:4\n        #8 0x55555595bb7a in cmd_fast_import ../builtin/fast-import.c:3614:4\n        #9 0x555555b1f493 in run_builtin ../git.c:480:11\n        #10 0x555555b1bfef in handle_builtin ../git.c:740:9\n        #11 0x555555b1e6f4 in run_argv ../git.c:807:4\n        #12 0x555555b1b87a in cmd_main ../git.c:947:19\n        #13 0x5555561649e6 in main ../common-main.c:64:11\n        #14 0x7ffff742a1fb in __libc_start_call_main (/nix/store/65h17wjrrlsj2rj540igylrx7fqcd6vq-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: bf320110569c8ec2425e9a0c5e4eb7e97f1fb6e4)\n        #15 0x7ffff742a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/65h17wjrrlsj2rj540igylrx7fqcd6vq-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: bf320110569c8ec2425e9a0c5e4eb7e97f1fb6e4)\n        #16 0x555555772c84 in _start (git+0x21ec84)\n\n    ==577126==Register values:\n    rax = 0x0000511000000cc0  rbx = 0x0000000000000000  rcx = 0x000000000000000c  rdx = 0x0000000000000000\n    rdi = 0x0000000000000000  rsi = 0x00005070000009c0  rbp = 0x00005070000009c0  rsp = 0x00007fffffff5b30\n     r8 = 0x0000000000000000   r9 = 0x0000000000000000  r10 = 0x0000000000000000  r11 = 0x00007ffff7a01a30\n    r12 = 0x0000000000000000  r13 = 0x00007fffffff6b60  r14 = 0x00007ffff7ffd000  r15 = 0x00005555563b9910\n    AddressSanitizer can not provide additional info.\n    SUMMARY: AddressSanitizer: SEGV (/nix/store/h1ydpxkw9qhjdxjpic1pdc2nirggyy6f-openssl-3.3.2/lib/libcrypto.so.3+0x201a99) (BuildId: 41746a580d39075fc85e8c8065b6c07fb34e97d4) in EVP_MD_CTX_copy_ex\n    ==577126==ABORTING\n    ./test-lib.sh: line 1039: 577126 Aborted                 git --git-dir=R/.git fast-import --big-file-threshold=1 < input\n    error: last command exited with $?=134\n    not ok 167 - R: blob bigger than threshold\n\nThe segfault is only exposed in case the unsafe and safe backends are\ndifferent from one another.\n\nFix the issue by initializing the context with the unsafe SHA1 variant.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fast-import.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 1fa2929a01b7dfee52b653248bba802884f6be6a..0f86392761abbe6acb217fef7f4fe7c3ff5ac1fa 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -1106,7 +1106,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n \t\t|| (pack_size + PACK_SIZE_THRESHOLD + len) < pack_size)\n \t\tcycle_packfile();\n \n-\tthe_hash_algo->init_fn(&checkpoint.ctx);\n+\tthe_hash_algo->unsafe_init_fn(&checkpoint.ctx);\n \thashfile_checkpoint(pack_file, &checkpoint);\n \toffset = checkpoint.offset;\n \n\n-- \n2.48.0.rc0.311.gb6c66824c1.dirty\n\n"},{"id":"509736","messageId":"20241230-pks-meson-sha1-unsafe-v1-1-efb276e171f5@pks.im","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im","subject":"[PATCH 01/10] bulk-checkin: fix segfault with unsafe SHA1 backend","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:24:01Z","receivedAt":"2024-12-30T14:24:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 1b9e9be8b4 (csum-file.c: use unsafe SHA-1 implementation when\navailable, 2024-09-26) we have converted our `struct hashfile` to use\nthe unsafe SHA1 backend, which results in a significant speedup. One\nneeds to be careful with how to use that structure now though because\ncallers need to consistently use either the safe or unsafe variants of\nSHA1, as otherwise one can easily trigger corruption.\n\nAs it turns out, we have one inconsistent usage in our tree because we\ndirectly initialize `struct hashfile_checkpoint::ctx` with the safe\nvariant of SHA1, but end up writing to that context with the unsafe\nones. This went unnoticed so far because our CI systems do not exercise\ndifferent hash functions for these two backends, and consequently safe\nand unsafe variants are equivalent. But when using SHA1DC as safe and\nOpenSSL as unsafe backend this leads to a crash an t1050:\n\n    ++ git -c core.compression=0 add large1\n    AddressSanitizer:DEADLYSIGNAL\n    =================================================================\n    ==1367==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000040 (pc 0x7ffff7a01a99 bp 0x507000000db0 sp 0x7fffffff5690 T0)\n    ==1367==The signal is caused by a READ memory access.\n    ==1367==Hint: address points to the zero page.\n        #0 0x7ffff7a01a99 in EVP_MD_CTX_copy_ex (/nix/store/h1ydpxkw9qhjdxjpic1pdc2nirggyy6f-openssl-3.3.2/lib/libcrypto.so.3+0x201a99) (BuildId: 41746a580d39075fc85e8c8065b6c07fb34e97d4)\n        #1 0x555555ddde56 in openssl_SHA1_Clone ../sha1/openssl.h:40:2\n        #2 0x555555dce2fc in git_hash_sha1_clone_unsafe ../object-file.c:123:2\n        #3 0x555555c2d5f8 in hashfile_checkpoint ../csum-file.c:211:2\n        #4 0x555555b9905d in deflate_blob_to_pack ../bulk-checkin.c:286:4\n        #5 0x555555b98ae9 in index_blob_bulk_checkin ../bulk-checkin.c:362:15\n        #6 0x555555ddab62 in index_blob_stream ../object-file.c:2756:9\n        #7 0x555555dda420 in index_fd ../object-file.c:2778:9\n        #8 0x555555ddad76 in index_path ../object-file.c:2796:7\n        #9 0x555555e947f3 in add_to_index ../read-cache.c:771:7\n        #10 0x555555e954a4 in add_file_to_index ../read-cache.c:804:9\n        #11 0x5555558b5c39 in add_files ../builtin/add.c:355:7\n        #12 0x5555558b412e in cmd_add ../builtin/add.c:578:18\n        #13 0x555555b1f493 in run_builtin ../git.c:480:11\n        #14 0x555555b1bfef in handle_builtin ../git.c:740:9\n        #15 0x555555b1e6f4 in run_argv ../git.c:807:4\n        #16 0x555555b1b87a in cmd_main ../git.c:947:19\n        #17 0x5555561649e6 in main ../common-main.c:64:11\n        #18 0x7ffff742a1fb in __libc_start_call_main (/nix/store/65h17wjrrlsj2rj540igylrx7fqcd6vq-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: bf320110569c8ec2425e9a0c5e4eb7e97f1fb6e4)\n        #19 0x7ffff742a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/65h17wjrrlsj2rj540igylrx7fqcd6vq-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: bf320110569c8ec2425e9a0c5e4eb7e97f1fb6e4)\n        #20 0x555555772c84 in _start (git+0x21ec84)\n\n    ==1367==Register values:\n    rax = 0x0000511000001080  rbx = 0x0000000000000000  rcx = 0x000000000000000c  rdx = 0x0000000000000000\n    rdi = 0x0000000000000000  rsi = 0x0000507000000db0  rbp = 0x0000507000000db0  rsp = 0x00007fffffff5690\n     r8 = 0x0000000000000000   r9 = 0x0000000000000000  r10 = 0x0000000000000000  r11 = 0x00007ffff7a01a30\n    r12 = 0x0000000000000000  r13 = 0x00007fffffff6b38  r14 = 0x00007ffff7ffd000  r15 = 0x00005555563b9910\n    AddressSanitizer can not provide additional info.\n    SUMMARY: AddressSanitizer: SEGV (/nix/store/h1ydpxkw9qhjdxjpic1pdc2nirggyy6f-openssl-3.3.2/lib/libcrypto.so.3+0x201a99) (BuildId: 41746a580d39075fc85e8c8065b6c07fb34e97d4) in EVP_MD_CTX_copy_ex\n    ==1367==ABORTING\n    ./test-lib.sh: line 1023:  1367 Aborted                 git $config add large1\n    error: last command exited with $?=134\n    not ok 4 - add with -c core.compression=0\n\nFix the issue by using the unsafe variant instead.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n bulk-checkin.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 4a70a70a951cfd1a488339a33bf3a76b5152a344..433070a3bda461a2ad62da67cc5515f8822d2df3 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -272,7 +272,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\t\t\t  OBJ_BLOB, size);\n \tthe_hash_algo->init_fn(&ctx);\n \tthe_hash_algo->update_fn(&ctx, obuf, header_len);\n-\tthe_hash_algo->init_fn(&checkpoint.ctx);\n+\tthe_hash_algo->unsafe_init_fn(&checkpoint.ctx);\n \n \t/* Note: idx is non-NULL when we are writing */\n \tif ((flags & HASH_WRITE_OBJECT) != 0)\n\n-- \n2.48.0.rc0.311.gb6c66824c1.dirty\n\n"},{"id":"509737","messageId":"20241230-pks-meson-sha1-unsafe-v1-3-efb276e171f5@pks.im","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im","subject":"[PATCH 03/10] ci: exercise unsafe OpenSSL backend","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:24:03Z","receivedAt":"2024-12-30T14:24:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the preceding commit we have fixed a segfault when using an unsafe\nSHA1 backend that is different from the safe one. This segfault only\nwent by unnoticed because we never set up an unsafe backend in our CI\nsystems. Fix this ommission by setting `OPENSSL_SHA1_UNSAFE` in our\nTEST-vars job.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n ci/run-build-and-tests.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex c4a41bba0b84df57f6e60aeac2de29dbc0e27dc1..76667a1277720d74e09e8da227b5e0832003e0e2 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -17,6 +17,7 @@ linux-gcc)\n \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n \t;;\n linux-TEST-vars)\n+\texport OPENSSL_SHA1_UNSAFE=YesPlease\n \texport GIT_TEST_SPLIT_INDEX=yes\n \texport GIT_TEST_MERGE_ALGORITHM=recursive\n \texport GIT_TEST_FULL_IN_PACK_ARRAY=true\n\n-- \n2.48.0.rc0.311.gb6c66824c1.dirty\n\n"},{"id":"509739","messageId":"20241230-pks-meson-sha1-unsafe-v1-4-efb276e171f5@pks.im","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im","subject":"[PATCH 04/10] meson: consistenlty spell 'CommonCrypto'","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:24:04Z","receivedAt":"2024-12-30T14:24:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The 'CommonCrypto' backend can be specified as HTTPS and SHA1 backends,\nbut the value that one needs to use is inconsistent across those two\nbuild options. Unify it to 'CommonCrypto'.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n meson.build       | 2 +-\n meson_options.txt | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/meson.build b/meson.build\nindex 0064eb64f546a6349a8694ce251bd352febda6fe..9da58dafe0f1023cc72f4ea3eff5515c9d479099 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1367,7 +1367,7 @@ if sha1_backend == 'sha1dc'\n     'sha1dc/sha1.c',\n     'sha1dc/ubc_check.c',\n   ]\n-elif sha1_backend == 'common-crypto'\n+elif sha1_backend == 'CommonCrypto'\n   libgit_c_args += '-DCOMMON_DIGEST_FOR_OPENSSL'\n   libgit_c_args += '-DSHA1_APPLE'\n   # Apple CommonCrypto requires chunking\ndiff --git a/meson_options.txt b/meson_options.txt\nindex 4be7eab39939178ae2ffde1ff9e78f83a1b482b2..a7f308d217f29ef301848e63623a49207ef83125 100644\n--- a/meson_options.txt\n+++ b/meson_options.txt\n@@ -49,7 +49,7 @@ option('regex', type: 'feature', value: 'auto',\n # Backends.\n option('https_backend', type: 'combo', value: 'auto', choices: ['auto', 'openssl', 'CommonCrypto', 'none'],\n   description: 'The HTTPS backend to use when connecting to remotes.')\n-option('sha1_backend', type: 'combo', choices: ['openssl', 'block', 'sha1dc', 'common-crypto'], value: 'sha1dc',\n+option('sha1_backend', type: 'combo', choices: ['openssl', 'block', 'sha1dc', 'CommonCrypto'], value: 'sha1dc',\n   description: 'The backend used for hashing objects with the SHA1 object format')\n option('sha256_backend', type: 'combo', choices: ['openssl', 'nettle', 'gcrypt', 'block'], value: 'block',\n   description: 'The backend used for hashing objects with the SHA256 object format')\n\n-- \n2.48.0.rc0.311.gb6c66824c1.dirty\n\n"},{"id":"509738","messageId":"20241230-pks-meson-sha1-unsafe-v1-5-efb276e171f5@pks.im","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im","subject":"[PATCH 05/10] meson: deduplicate access to SHA1/SHA256 backend options","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:24:05Z","receivedAt":"2024-12-30T14:24:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We've got a couple of repeated calls to `get_option()` for the SHA1 and\nSHA256 backend options. While not an issue, it makes the code needlessly\nverbose.\n\nFix this by consistently using a local variable.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n meson.build | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/meson.build b/meson.build\nindex 9da58dafe0f1023cc72f4ea3eff5515c9d479099..6fa4d900ee02f0b80bc3c36d58a07a118ec3fb20 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1326,6 +1326,8 @@ if not meson.is_cross_build() and fs.exists('/dev/tty')\n endif\n \n https_backend = get_option('https_backend')\n+sha1_backend = get_option('sha1_backend')\n+sha256_backend = get_option('sha256_backend')\n \n security_framework = dependency('Security', required: https_backend == 'CommonCrypto')\n core_foundation_framework = dependency('CoreFoundation', required: security_framework.found())\n@@ -1333,7 +1335,7 @@ if https_backend == 'auto' and security_framework.found()\n   https_backend = 'CommonCrypto'\n endif\n \n-openssl_required = https_backend == 'openssl' or get_option('sha1_backend') == 'openssl' or get_option('sha256_backend') == 'openssl'\n+openssl_required = https_backend == 'openssl' or sha1_backend == 'openssl' or sha256_backend == 'openssl'\n openssl = dependency('openssl', required: openssl_required, default_options: ['default_library=static'])\n if https_backend == 'auto' and openssl.found()\n   https_backend = 'openssl'\n@@ -1354,7 +1356,6 @@ if https_backend != 'openssl'\n   libgit_c_args += '-DNO_OPENSSL'\n endif\n \n-sha1_backend = get_option('sha1_backend')\n if sha1_backend == 'sha1dc'\n   libgit_c_args += '-DSHA1_DC'\n   libgit_c_args += '-DSHA1DC_NO_STANDARD_INCLUDES=1'\n@@ -1382,7 +1383,6 @@ else\n   error('Unhandled SHA1 backend ' + sha1_backend)\n endif\n \n-sha256_backend = get_option('sha256_backend')\n if sha256_backend == 'openssl'\n   libgit_c_args += '-DSHA256_OPENSSL'\n   libgit_dependencies += openssl\n\n-- \n2.48.0.rc0.311.gb6c66824c1.dirty\n\n"},{"id":"509740","messageId":"20241230-pks-meson-sha1-unsafe-v1-6-efb276e171f5@pks.im","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im","subject":"[PATCH 06/10] meson: require SecurityFramework when it's used as SHA1 backend","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:24:06Z","receivedAt":"2024-12-30T14:24:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The Security framework is required when we use CommonCrypto either as\nHTTPS or SHA1 backend, but we only require it in case it is set up as\nHTTPS backend. Fix this.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n meson.build | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/meson.build b/meson.build\nindex 6fa4d900ee02f0b80bc3c36d58a07a118ec3fb20..bc75ad954a4949342125b769d3d8d8362ef4e8a3 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1329,7 +1329,7 @@ https_backend = get_option('https_backend')\n sha1_backend = get_option('sha1_backend')\n sha256_backend = get_option('sha256_backend')\n \n-security_framework = dependency('Security', required: https_backend == 'CommonCrypto')\n+security_framework = dependency('Security', required: https_backend == 'CommonCrypto' or sha1_backend == 'CommonCrypto')\n core_foundation_framework = dependency('CoreFoundation', required: security_framework.found())\n if https_backend == 'auto' and security_framework.found()\n   https_backend = 'CommonCrypto'\n\n-- \n2.48.0.rc0.311.gb6c66824c1.dirty\n\n"},{"id":"509741","messageId":"20241230-pks-meson-sha1-unsafe-v1-7-efb276e171f5@pks.im","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im","subject":"[PATCH 07/10] meson: simplify conditions for HTTPS and SHA1 dependencies","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:24:07Z","receivedAt":"2024-12-30T14:24:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The conditions used to figure out whteher the Security framework or\nOpenSSL library is required are a bit convoluted because they can be\npulled in via the HTTPS, SHA1 or SHA256 backends. Refactor them to be\neasier to read.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n meson.build | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/meson.build b/meson.build\nindex bc75ad954a4949342125b769d3d8d8362ef4e8a3..46f807899b7bae33dd6aa7a94a54931d69ab0d62 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1329,13 +1329,13 @@ https_backend = get_option('https_backend')\n sha1_backend = get_option('sha1_backend')\n sha256_backend = get_option('sha256_backend')\n \n-security_framework = dependency('Security', required: https_backend == 'CommonCrypto' or sha1_backend == 'CommonCrypto')\n+security_framework = dependency('Security', required: 'CommonCrypto' in [https_backend, sha1_backend])\n core_foundation_framework = dependency('CoreFoundation', required: security_framework.found())\n if https_backend == 'auto' and security_framework.found()\n   https_backend = 'CommonCrypto'\n endif\n \n-openssl_required = https_backend == 'openssl' or sha1_backend == 'openssl' or sha256_backend == 'openssl'\n+openssl_required = 'openssl' in [https_backend, sha1_backend, sha256_backend]\n openssl = dependency('openssl', required: openssl_required, default_options: ['default_library=static'])\n if https_backend == 'auto' and openssl.found()\n   https_backend = 'openssl'\n\n-- \n2.48.0.rc0.311.gb6c66824c1.dirty\n\n"},{"id":"509742","messageId":"20241230-pks-meson-sha1-unsafe-v1-8-efb276e171f5@pks.im","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im","subject":"[PATCH 08/10] meson: add missing dots for build options","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:24:08Z","receivedAt":"2024-12-30T14:24:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Most of our Meson build options end with a trailing dot, but those for\nour SHA1 and SHA256 backends don't. Add it.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n meson_options.txt | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/meson_options.txt b/meson_options.txt\nindex a7f308d217f29ef301848e63623a49207ef83125..d8d283982bcdd0f688556e0102c0133061dfb304 100644\n--- a/meson_options.txt\n+++ b/meson_options.txt\n@@ -50,9 +50,9 @@ option('regex', type: 'feature', value: 'auto',\n option('https_backend', type: 'combo', value: 'auto', choices: ['auto', 'openssl', 'CommonCrypto', 'none'],\n   description: 'The HTTPS backend to use when connecting to remotes.')\n option('sha1_backend', type: 'combo', choices: ['openssl', 'block', 'sha1dc', 'CommonCrypto'], value: 'sha1dc',\n-  description: 'The backend used for hashing objects with the SHA1 object format')\n+  description: 'The backend used for hashing objects with the SHA1 object format.')\n option('sha256_backend', type: 'combo', choices: ['openssl', 'nettle', 'gcrypt', 'block'], value: 'block',\n-  description: 'The backend used for hashing objects with the SHA256 object format')\n+  description: 'The backend used for hashing objects with the SHA256 object format.')\n \n # Build tweaks.\n option('macos_use_homebrew_gettext', type: 'boolean', value: true,\n\n-- \n2.48.0.rc0.311.gb6c66824c1.dirty\n\n"},{"id":"509743","messageId":"20241230-pks-meson-sha1-unsafe-v1-9-efb276e171f5@pks.im","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im","subject":"[PATCH 09/10] meson: wire up unsafe SHA1 backend","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:24:09Z","receivedAt":"2024-12-30T14:24:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 06c92dafb8 (Makefile: allow specifying a SHA-1 for non-cryptographic\nuses, 2024-09-26), we have introduced a cryptographically-insecure\nbackend for SHA1 that can optionally be used in some contexts where the\nprocessed data is not security relevant. This effort was in-flight with\nthe effort to introduce Meson, so we don't have an equivalent here.\n\nWire up a new build option that lets users pick an unsafe SHA1 backend.\n\nNote that for simplicity's sake we have to drop the error condition\naround an unhandled SHA1 backend. This should be fine though given that\nMeson verifies the value for combo-options for us.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n meson.build       | 40 ++++++++++++++++++++++++++++++----------\n meson_options.txt |  2 ++\n 2 files changed, 32 insertions(+), 10 deletions(-)\n\ndiff --git a/meson.build b/meson.build\nindex 46f807899b7bae33dd6aa7a94a54931d69ab0d62..dc82c23cb4f07646a9a7bb96fefcf832f9840975 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1327,15 +1327,16 @@ endif\n \n https_backend = get_option('https_backend')\n sha1_backend = get_option('sha1_backend')\n+sha1_unsafe_backend = get_option('sha1_unsafe_backend')\n sha256_backend = get_option('sha256_backend')\n \n-security_framework = dependency('Security', required: 'CommonCrypto' in [https_backend, sha1_backend])\n+security_framework = dependency('Security', required: 'CommonCrypto' in [https_backend, sha1_backend, sha1_unsafe_backend])\n core_foundation_framework = dependency('CoreFoundation', required: security_framework.found())\n if https_backend == 'auto' and security_framework.found()\n   https_backend = 'CommonCrypto'\n endif\n \n-openssl_required = 'openssl' in [https_backend, sha1_backend, sha256_backend]\n+openssl_required = 'openssl' in [https_backend, sha1_backend, sha1_unsafe_backend, sha256_backend]\n openssl = dependency('openssl', required: openssl_required, default_options: ['default_library=static'])\n if https_backend == 'auto' and openssl.found()\n   https_backend = 'openssl'\n@@ -1368,19 +1369,38 @@ if sha1_backend == 'sha1dc'\n     'sha1dc/sha1.c',\n     'sha1dc/ubc_check.c',\n   ]\n-elif sha1_backend == 'CommonCrypto'\n+endif\n+if sha1_backend == 'CommonCrypto' or sha1_unsafe_backend == 'CommonCrypto'\n+  if sha1_backend == 'CommonCrypto'\n+    libgit_c_args += '-DSHA1_APPLE'\n+  endif\n+  if sha1_unsafe_backend == 'CommonCrypto'\n+    libgit_c_args += '-DSHA1_APPLE_UNSAFE'\n+  endif\n+\n   libgit_c_args += '-DCOMMON_DIGEST_FOR_OPENSSL'\n-  libgit_c_args += '-DSHA1_APPLE'\n   # Apple CommonCrypto requires chunking\n   libgit_c_args += '-DSHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L'\n-elif sha1_backend == 'openssl'\n-  libgit_c_args += '-DSHA1_OPENSSL'\n+endif\n+if sha1_backend == 'openssl' or sha1_unsafe_backend == 'openssl'\n+  if sha1_backend == 'openssl'\n+    libgit_c_args += '-DSHA1_OPENSSL'\n+  endif\n+  if sha1_unsafe_backend == 'openssl'\n+    libgit_c_args += '-DSHA1_OPENSSL_UNSAFE'\n+  endif\n+\n   libgit_dependencies += openssl\n-elif sha1_backend == 'block'\n-  libgit_c_args += '-DSHA1_BLK'\n+endif\n+if sha1_backend == 'block' or sha1_unsafe_backend == 'block'\n+  if sha1_backend == 'block'\n+    libgit_c_args += '-DSHA1_BLK'\n+  endif\n+  if sha1_unsafe_backend == 'block'\n+    libgit_c_args += '-DSHA1_BLK_UNSAFE'\n+  endif\n+\n   libgit_sources += 'block-sha1/sha1.c'\n-else\n-  error('Unhandled SHA1 backend ' + sha1_backend)\n endif\n \n if sha256_backend == 'openssl'\ndiff --git a/meson_options.txt b/meson_options.txt\nindex d8d283982bcdd0f688556e0102c0133061dfb304..8282b1dea8e852fbd3a28309a96fdc83412f245d 100644\n--- a/meson_options.txt\n+++ b/meson_options.txt\n@@ -51,6 +51,8 @@ option('https_backend', type: 'combo', value: 'auto', choices: ['auto', 'openssl\n   description: 'The HTTPS backend to use when connecting to remotes.')\n option('sha1_backend', type: 'combo', choices: ['openssl', 'block', 'sha1dc', 'CommonCrypto'], value: 'sha1dc',\n   description: 'The backend used for hashing objects with the SHA1 object format.')\n+option('sha1_unsafe_backend', type: 'combo', choices: ['openssl', 'block', 'CommonCrypto', 'none'], value: 'none',\n+  description: 'The backend used for hashing data with the SHA1 object format in case no cryptographic security is needed.')\n option('sha256_backend', type: 'combo', choices: ['openssl', 'nettle', 'gcrypt', 'block'], value: 'block',\n   description: 'The backend used for hashing objects with the SHA256 object format.')\n \n\n-- \n2.48.0.rc0.311.gb6c66824c1.dirty\n\n"},{"id":"509744","messageId":"20241230-pks-meson-sha1-unsafe-v1-10-efb276e171f5@pks.im","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im","subject":"[PATCH 10/10] meson: provide a summary of configured backends","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:24:10Z","receivedAt":"2024-12-30T14:24:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are a couple of backends from which the user can choose for HTTPS,\nSHA1, its unsafe variant as well as SHA256. Provide a summary of the\nconfigured values to make these more discoverable.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n meson.build | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/meson.build b/meson.build\nindex dc82c23cb4f07646a9a7bb96fefcf832f9840975..7361eb2eaad422e7a6c6ed95d275615836c21cdb 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1943,3 +1943,10 @@ summary({\n   'perl': perl_features_enabled,\n   'python': python.found(),\n }, section: 'Auto-detected features')\n+\n+summary({\n+  'https': https_backend,\n+  'sha1': sha1_backend,\n+  'sha1_unsafe': sha1_unsafe_backend,\n+  'sha256': sha256_backend,\n+}, section: 'Backends')\n\n-- \n2.48.0.rc0.311.gb6c66824c1.dirty\n\n"},{"id":"509749","messageId":"Z3LNVzw9UlS7SWqe@nand.local","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-2-efb276e171f5@pks.im","subject":"Re: [PATCH 02/10] builtin/fast-import: fix segfault with unsafe SHA1","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-12-30T17:22:34Z","receivedAt":"2024-12-30T17:22:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Dec 30, 2024 at 03:24:02PM +0100, Patrick Steinhardt wrote:\n> Same as with the preceding commit, git-fast-import(1) is using the safe\n> variant to initialize a hashfile checkpoint. This leads to a segfault\n> when passing the checkpoint into the hashfile subsystem because it would\n> use the unsafe variants instead:\n>\n>     ++ git --git-dir=R/.git fast-import --big-file-threshold=1\n>     AddressSanitizer:DEADLYSIGNAL\n>     =================================================================\n>     ==577126==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000040 (pc 0x7ffff7a01a99 bp 0x5070000009c0 sp 0x7fffffff5b30 T0)\n>     ==577126==The signal is caused by a READ memory access.\n>     ==577126==Hint: address points to the zero page.\n>         #0 0x7ffff7a01a99 in EVP_MD_CTX_copy_ex (/nix/store/h1ydpxkw9qhjdxjpic1pdc2nirggyy6f-openssl-3.3.2/lib/libcrypto.so.3+0x201a99) (BuildId: 41746a580d39075fc85e8c8065b6c07fb34e97d4)\n>         #1 0x555555ddde56 in openssl_SHA1_Clone ../sha1/openssl.h:40:2\n>         #2 0x555555dce2fc in git_hash_sha1_clone_unsafe ../object-file.c:123:2\n>         #3 0x555555c2d5f8 in hashfile_checkpoint ../csum-file.c:211:2\n>         #4 0x5555559647d1 in stream_blob ../builtin/fast-import.c:1110:2\n>         #5 0x55555596247b in parse_and_store_blob ../builtin/fast-import.c:2031:3\n>         #6 0x555555967f91 in file_change_m ../builtin/fast-import.c:2408:5\n>         #7 0x55555595d8a2 in parse_new_commit ../builtin/fast-import.c:2768:4\n>         #8 0x55555595bb7a in cmd_fast_import ../builtin/fast-import.c:3614:4\n>         #9 0x555555b1f493 in run_builtin ../git.c:480:11\n>         #10 0x555555b1bfef in handle_builtin ../git.c:740:9\n>         #11 0x555555b1e6f4 in run_argv ../git.c:807:4\n>         #12 0x555555b1b87a in cmd_main ../git.c:947:19\n>         #13 0x5555561649e6 in main ../common-main.c:64:11\n>         #14 0x7ffff742a1fb in __libc_start_call_main (/nix/store/65h17wjrrlsj2rj540igylrx7fqcd6vq-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: bf320110569c8ec2425e9a0c5e4eb7e97f1fb6e4)\n>         #15 0x7ffff742a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/65h17wjrrlsj2rj540igylrx7fqcd6vq-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: bf320110569c8ec2425e9a0c5e4eb7e97f1fb6e4)\n>         #16 0x555555772c84 in _start (git+0x21ec84)\n\nWow. This is a really good catch, and I'm embarrassed to have introduced\nthe bug in the first place.\n\nIf I can state back to you what I think is going on to make sure that\nwe're on the same page... In fast-import, we initialize a\nhashfile_checkpoint struct using regular the_hash_algo callbacks, like\nthe_hash_algo->init_fn(), the_hash_algo->update_fn(), etc. But we also\nhave a hashfile, which will use the unsafe variants of these functions.\n\nIf we're using a specialized unsafe variant, then calling\nhashfile_checkpoint() will crash. In your example, the crash happens\nwhen we try and call the clone_fn, but I think it could also happen\nwith hashflush() when it tries to call update_fn().\n\nI thought that my series here:\n\n    https://lore.kernel.org/git/cover.1732130001.git.me@ttaylorr.com/\n\nwould have prevented this, but I don't think that it would have in its\ncurrent state. A little more on that in a second...\n\n> ---\n>  builtin/fast-import.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> index 1fa2929a01b7dfee52b653248bba802884f6be6a..0f86392761abbe6acb217fef7f4fe7c3ff5ac1fa 100644\n> --- a/builtin/fast-import.c\n> +++ b/builtin/fast-import.c\n> @@ -1106,7 +1106,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n>  \t\t|| (pack_size + PACK_SIZE_THRESHOLD + len) < pack_size)\n>  \t\tcycle_packfile();\n>\n> -\tthe_hash_algo->init_fn(&checkpoint.ctx);\n> +\tthe_hash_algo->unsafe_init_fn(&checkpoint.ctx);\n\nThis will obviously fix the issue at hand, but I don't think this is any\nless brittle than before. The hash function implementation here needs to\nagree with that used in the hashfile API. This change makes that\nhappen, but only using side information that the hashfile API uses the\nunsafe variants.\n\nThe series I mentioned above made the algop a property of the hashfile\nitself, and introduced a specialized \"unsafe\" variant that did not\nrequire using separate callbacks.\n\nSo that on its own would not have saved us here (or in the other spot\nfrom the previous patch). But something on top like this would have:\n\n--- 8< ---\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 76d5c20f14..89fc20d436 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -1101,7 +1101,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n \t\t|| (pack_size + PACK_SIZE_THRESHOLD + len) < pack_size)\n \t\tcycle_packfile();\n\n-\tthe_hash_algo->init_fn(&checkpoint.ctx);\n+\tpack_file->algop->init_fn(&checkpoint.ctx);\n \thashfile_checkpoint(pack_file, &checkpoint);\n \toffset = checkpoint.offset;\n--- >8 ---\n\n(and an analogous change to the other spot).\n\nI think we should perhaps combine forces here. My ideal end-state is to\nhave the unsafe_hash_algo() stuff land from my earlier series, then have\nthese two fixes (adjusted to the new world order as above), and finally\nthe Meson fixes after that.\n\nDoes that seem like a plan to you? If so, I can put everything together\nand send it out (if you're OK with me forging your s-o-b).\n\nThanks,\nTaylor\n"},{"id":"509750","messageId":"Z3LY9gLVdQQWk0Zc@nand.local","threadId":"62713","inReplyTo":"20241230-pks-meson-sha1-unsafe-v1-3-efb276e171f5@pks.im","subject":"Re: [PATCH 03/10] ci: exercise unsafe OpenSSL backend","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-12-30T17:31:34Z","receivedAt":"2024-12-30T17:31:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Dec 30, 2024 at 03:24:03PM +0100, Patrick Steinhardt wrote:\n> In the preceding commit we have fixed a segfault when using an unsafe\n> SHA1 backend that is different from the safe one. This segfault only\n> went by unnoticed because we never set up an unsafe backend in our CI\n> systems. Fix this ommission by setting `OPENSSL_SHA1_UNSAFE` in our\n> TEST-vars job.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  ci/run-build-and-tests.sh | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n> index c4a41bba0b84df57f6e60aeac2de29dbc0e27dc1..76667a1277720d74e09e8da227b5e0832003e0e2 100755\n> --- a/ci/run-build-and-tests.sh\n> +++ b/ci/run-build-and-tests.sh\n> @@ -17,6 +17,7 @@ linux-gcc)\n>  \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n>  \t;;\n>  linux-TEST-vars)\n> +\texport OPENSSL_SHA1_UNSAFE=YesPlease\n>  \texport GIT_TEST_SPLIT_INDEX=yes\n>  \texport GIT_TEST_MERGE_ALGORITHM=recursive\n>  \texport GIT_TEST_FULL_IN_PACK_ARRAY=true\n\nI think that this is a great idea, thanks for adding it!\n\nThanks,\nTaylor\n"},{"id":"509845","messageId":"Z3fhK1ACzJfVehM2@pks.im","threadId":"62713","inReplyTo":"Z3LNVzw9UlS7SWqe@nand.local","subject":"Re: [PATCH 02/10] builtin/fast-import: fix segfault with unsafe SHA1","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T13:08:01Z","receivedAt":"2025-01-03T13:08:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Dec 30, 2024 at 12:22:34PM -0500, Taylor Blau wrote:\n> On Mon, Dec 30, 2024 at 03:24:02PM +0100, Patrick Steinhardt wrote:\n> > diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> > index 1fa2929a01b7dfee52b653248bba802884f6be6a..0f86392761abbe6acb217fef7f4fe7c3ff5ac1fa 100644\n> > --- a/builtin/fast-import.c\n> > +++ b/builtin/fast-import.c\n> > @@ -1106,7 +1106,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n> >  \t\t|| (pack_size + PACK_SIZE_THRESHOLD + len) < pack_size)\n> >  \t\tcycle_packfile();\n> >\n> > -\tthe_hash_algo->init_fn(&checkpoint.ctx);\n> > +\tthe_hash_algo->unsafe_init_fn(&checkpoint.ctx);\n> \n> This will obviously fix the issue at hand, but I don't think this is any\n> less brittle than before. The hash function implementation here needs to\n> agree with that used in the hashfile API. This change makes that\n> happen, but only using side information that the hashfile API uses the\n> unsafe variants.\n\nYup, I only cared about fixing the segfault because we're close to the\nv2.48 release. I agree that the overall state is still extremely brittle\nright now.\n\n[snip]\n> I think we should perhaps combine forces here. My ideal end-state is to\n> have the unsafe_hash_algo() stuff land from my earlier series, then have\n> these two fixes (adjusted to the new world order as above), and finally\n> the Meson fixes after that.\n> \n> Does that seem like a plan to you? If so, I can put everything together\n> and send it out (if you're OK with me forging your s-o-b).\n\nI think the ideal state would be if the hashing function used was stored\nas part of `struct git_hash_ctx`. So the flow basically becomes for\nexample:\n\n    ```\n    struct git_hash_ctx ctx;\n    struct object_id oid;\n\n    git_hash_sha1_init(&ctx);\n    git_hash_update(&ctx, data);\n    git_hash_final_oid(&oid, &ctx);\n    ```\n\nNote how the intermediate calls don't need to know which hash function\nyou used to initialize the `struct git_hash_ctx` -- the structure itself\nshould remember what it has been initilized with and do the right thing.\n\nPatrick\n"},{"id":"509866","messageId":"xmqqbjwnsw3u.fsf@gitster.g","threadId":"62713","inReplyTo":"Z3fhK1ACzJfVehM2@pks.im","subject":"Re: [PATCH 02/10] builtin/fast-import: fix segfault with unsafe SHA1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-03T16:25:25Z","receivedAt":"2025-01-03T16:25:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I think the ideal state would be if the hashing function used was stored\n> as part of `struct git_hash_ctx`. So the flow basically becomes for\n> example:\n>\n>     ```\n>     struct git_hash_ctx ctx;\n>     struct object_id oid;\n>\n>     git_hash_sha1_init(&ctx);\n>     git_hash_update(&ctx, data);\n>     git_hash_final_oid(&oid, &ctx);\n>     ```\n>\n> Note how the intermediate calls don't need to know which hash function\n> you used to initialize the `struct git_hash_ctx` -- the structure itself\n> should remember what it has been initilized with and do the right thing.\n\nYup, that sounds perfect.\n"},{"id":"510011","messageId":"Z3wsZjAqbfI/EdVe@nand.local","threadId":"62713","inReplyTo":"Z3fhK1ACzJfVehM2@pks.im","subject":"Re: [PATCH 02/10] builtin/fast-import: fix segfault with unsafe SHA1","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-01-06T19:17:58Z","receivedAt":"2025-01-06T19:18:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jan 03, 2025 at 02:08:01PM +0100, Patrick Steinhardt wrote:\n> On Mon, Dec 30, 2024 at 12:22:34PM -0500, Taylor Blau wrote:\n> > On Mon, Dec 30, 2024 at 03:24:02PM +0100, Patrick Steinhardt wrote:\n> > > diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> > > index 1fa2929a01b7dfee52b653248bba802884f6be6a..0f86392761abbe6acb217fef7f4fe7c3ff5ac1fa 100644\n> > > --- a/builtin/fast-import.c\n> > > +++ b/builtin/fast-import.c\n> > > @@ -1106,7 +1106,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n> > >  \t\t|| (pack_size + PACK_SIZE_THRESHOLD + len) < pack_size)\n> > >  \t\tcycle_packfile();\n> > >\n> > > -\tthe_hash_algo->init_fn(&checkpoint.ctx);\n> > > +\tthe_hash_algo->unsafe_init_fn(&checkpoint.ctx);\n> >\n> > This will obviously fix the issue at hand, but I don't think this is any\n> > less brittle than before. The hash function implementation here needs to\n> > agree with that used in the hashfile API. This change makes that\n> > happen, but only using side information that the hashfile API uses the\n> > unsafe variants.\n>\n> Yup, I only cared about fixing the segfault because we're close to the\n> v2.48 release. I agree that the overall state is still extremely brittle\n> right now.\n>\n> [snip]\n> > I think we should perhaps combine forces here. My ideal end-state is to\n> > have the unsafe_hash_algo() stuff land from my earlier series, then have\n> > these two fixes (adjusted to the new world order as above), and finally\n> > the Meson fixes after that.\n> >\n> > Does that seem like a plan to you? If so, I can put everything together\n> > and send it out (if you're OK with me forging your s-o-b).\n>\n> I think the ideal state would be if the hashing function used was stored\n> as part of `struct git_hash_ctx`. So the flow basically becomes for\n> example:\n>\n>     ```\n>     struct git_hash_ctx ctx;\n>     struct object_id oid;\n>\n>     git_hash_sha1_init(&ctx);\n>     git_hash_update(&ctx, data);\n>     git_hash_final_oid(&oid, &ctx);\n>     ```\n>\n> Note how the intermediate calls don't need to know which hash function\n> you used to initialize the `struct git_hash_ctx` -- the structure itself\n> should remember what it has been initilized with and do the right thing.\n\nI'm not sure I'm following you here. In the stream_blob() function\nwithin fast-import, the problem isn't that we're switching hash\nfunctions mid-stream, but that we're initializing the hashfile_context\nstructure with the wrong hash function to begin with.\n\nYou snipped it out of your reply, but I think that my suggestion to do:\n\n    pack_file->algop->init_fn(&checkpoint.ctx);\n\nwould harden us against the broken behavior we're seeing here.\n\nAs a separate defense-in-depth measure, we could teach functions from\nthe hashfile API which deal with hashfile_checkpoint structure to ensure\nthat the hashfile and its checkpoint both use the same algorithm (by\nadding a hash_algo field to the hashfile_checkpoint structure).\n\nThanks,\nTaylor\n"},{"id":"510064","messageId":"Z30YvHKjA_b6_xwt@pks.im","threadId":"62713","inReplyTo":"Z3wsZjAqbfI/EdVe@nand.local","subject":"Re: [PATCH 02/10] builtin/fast-import: fix segfault with unsafe SHA1","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-07T12:06:20Z","receivedAt":"2025-01-07T12:06:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 06, 2025 at 02:17:58PM -0500, Taylor Blau wrote:\n> On Fri, Jan 03, 2025 at 02:08:01PM +0100, Patrick Steinhardt wrote:\n> > On Mon, Dec 30, 2024 at 12:22:34PM -0500, Taylor Blau wrote:\n> > > On Mon, Dec 30, 2024 at 03:24:02PM +0100, Patrick Steinhardt wrote:\n> > > > diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> > > > index 1fa2929a01b7dfee52b653248bba802884f6be6a..0f86392761abbe6acb217fef7f4fe7c3ff5ac1fa 100644\n> > > > --- a/builtin/fast-import.c\n> > > > +++ b/builtin/fast-import.c\n> > > > @@ -1106,7 +1106,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n> > > >  \t\t|| (pack_size + PACK_SIZE_THRESHOLD + len) < pack_size)\n> > > >  \t\tcycle_packfile();\n> > > >\n> > > > -\tthe_hash_algo->init_fn(&checkpoint.ctx);\n> > > > +\tthe_hash_algo->unsafe_init_fn(&checkpoint.ctx);\n> > >\n> > > This will obviously fix the issue at hand, but I don't think this is any\n> > > less brittle than before. The hash function implementation here needs to\n> > > agree with that used in the hashfile API. This change makes that\n> > > happen, but only using side information that the hashfile API uses the\n> > > unsafe variants.\n> >\n> > Yup, I only cared about fixing the segfault because we're close to the\n> > v2.48 release. I agree that the overall state is still extremely brittle\n> > right now.\n> >\n> > [snip]\n> > > I think we should perhaps combine forces here. My ideal end-state is to\n> > > have the unsafe_hash_algo() stuff land from my earlier series, then have\n> > > these two fixes (adjusted to the new world order as above), and finally\n> > > the Meson fixes after that.\n> > >\n> > > Does that seem like a plan to you? If so, I can put everything together\n> > > and send it out (if you're OK with me forging your s-o-b).\n> >\n> > I think the ideal state would be if the hashing function used was stored\n> > as part of `struct git_hash_ctx`. So the flow basically becomes for\n> > example:\n> >\n> >     ```\n> >     struct git_hash_ctx ctx;\n> >     struct object_id oid;\n> >\n> >     git_hash_sha1_init(&ctx);\n> >     git_hash_update(&ctx, data);\n> >     git_hash_final_oid(&oid, &ctx);\n> >     ```\n> >\n> > Note how the intermediate calls don't need to know which hash function\n> > you used to initialize the `struct git_hash_ctx` -- the structure itself\n> > should remember what it has been initilized with and do the right thing.\n> \n> I'm not sure I'm following you here. In the stream_blob() function\n> within fast-import, the problem isn't that we're switching hash\n> functions mid-stream, but that we're initializing the hashfile_context\n> structure with the wrong hash function to begin with.\n\nTrue, but it would have been a non-issue if the hash context itself knew\nwhich hash function to use for updates. Sure, we would've used the slow\nvariant of SHA1 instead of the fast-but-unsafe one. But that feels like\nthe lesser evil compared to crashing.\n\n> You snipped it out of your reply, but I think that my suggestion to do:\n> \n>     pack_file->algop->init_fn(&checkpoint.ctx);\n> \n> would harden us against the broken behavior we're seeing here.\n> \n> As a separate defense-in-depth measure, we could teach functions from\n> the hashfile API which deal with hashfile_checkpoint structure to ensure\n> that the hashfile and its checkpoint both use the same algorithm (by\n> adding a hash_algo field to the hashfile_checkpoint structure).\n\nI would think that it were even harder to abuse if it wasn't the\nhashfile API, but the hash API that remembered the algorithm.\n\nPatrick\n"},{"id":"510199","messageId":"Z37QS/MnGUZ7zcrR@nand.local","threadId":"62713","inReplyTo":"Z30YvHKjA_b6_xwt@pks.im","subject":"Re: [PATCH 02/10] builtin/fast-import: fix segfault with unsafe SHA1","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-01-08T19:21:47Z","receivedAt":"2025-01-08T19:21:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 07, 2025 at 01:06:20PM +0100, Patrick Steinhardt wrote:\n> > > > I think we should perhaps combine forces here. My ideal end-state is to\n> > > > have the unsafe_hash_algo() stuff land from my earlier series, then have\n> > > > these two fixes (adjusted to the new world order as above), and finally\n> > > > the Meson fixes after that.\n> > > >\n> > > > Does that seem like a plan to you? If so, I can put everything together\n> > > > and send it out (if you're OK with me forging your s-o-b).\n> > >\n> > > I think the ideal state would be if the hashing function used was stored\n> > > as part of `struct git_hash_ctx`. So the flow basically becomes for\n> > > example:\n> > >\n> > >     ```\n> > >     struct git_hash_ctx ctx;\n> > >     struct object_id oid;\n> > >\n> > >     git_hash_sha1_init(&ctx);\n> > >     git_hash_update(&ctx, data);\n> > >     git_hash_final_oid(&oid, &ctx);\n> > >     ```\n> > >\n> > > Note how the intermediate calls don't need to know which hash function\n> > > you used to initialize the `struct git_hash_ctx` -- the structure itself\n> > > should remember what it has been initilized with and do the right thing.\n> >\n> > I'm not sure I'm following you here. In the stream_blob() function\n> > within fast-import, the problem isn't that we're switching hash\n> > functions mid-stream, but that we're initializing the hashfile_context\n> > structure with the wrong hash function to begin with.\n>\n> True, but it would have been a non-issue if the hash context itself knew\n> which hash function to use for updates. Sure, we would've used the slow\n> variant of SHA1 instead of the fast-but-unsafe one. But that feels like\n> the lesser evil compared to crashing.\n\nFor posterity, Patrick and I used some of our monthly meeting this morning to\nspend some time together pairing on this idea.\n\nIt ended up being a dead-end, since this approach only protects you\nagainst changing the hash function mid-stream, and not using the\nincorrect context type from the union.\n\nThat was along the lines of what I was originally thinking, and so I\nresurrected my series to introduce 'unsafe_hash_algo()' here:\n\n    https://lore.kernel.org/git/cover.1736363652.git.me@ttaylorr.com/\n\nI got the impression that Patrick and I are on the same page there as\nthat being a good path forward, but I'll let him chime in in case I\nmisunderstood anything.\n\nThanks,\nTaylor\n"},{"id":"510212","messageId":"Z39lZ1E6zm88PgCf@pks.im","threadId":"62713","inReplyTo":"Z37QS/MnGUZ7zcrR@nand.local","subject":"Re: [PATCH 02/10] builtin/fast-import: fix segfault with unsafe SHA1","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-09T05:57:59Z","receivedAt":"2025-01-09T05:58:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 08, 2025 at 02:21:47PM -0500, Taylor Blau wrote:\n> On Tue, Jan 07, 2025 at 01:06:20PM +0100, Patrick Steinhardt wrote:\n> > > > > I think we should perhaps combine forces here. My ideal end-state is to\n> > > > > have the unsafe_hash_algo() stuff land from my earlier series, then have\n> > > > > these two fixes (adjusted to the new world order as above), and finally\n> > > > > the Meson fixes after that.\n> > > > >\n> > > > > Does that seem like a plan to you? If so, I can put everything together\n> > > > > and send it out (if you're OK with me forging your s-o-b).\n> > > >\n> > > > I think the ideal state would be if the hashing function used was stored\n> > > > as part of `struct git_hash_ctx`. So the flow basically becomes for\n> > > > example:\n> > > >\n> > > >     ```\n> > > >     struct git_hash_ctx ctx;\n> > > >     struct object_id oid;\n> > > >\n> > > >     git_hash_sha1_init(&ctx);\n> > > >     git_hash_update(&ctx, data);\n> > > >     git_hash_final_oid(&oid, &ctx);\n> > > >     ```\n> > > >\n> > > > Note how the intermediate calls don't need to know which hash function\n> > > > you used to initialize the `struct git_hash_ctx` -- the structure itself\n> > > > should remember what it has been initilized with and do the right thing.\n> > >\n> > > I'm not sure I'm following you here. In the stream_blob() function\n> > > within fast-import, the problem isn't that we're switching hash\n> > > functions mid-stream, but that we're initializing the hashfile_context\n> > > structure with the wrong hash function to begin with.\n> >\n> > True, but it would have been a non-issue if the hash context itself knew\n> > which hash function to use for updates. Sure, we would've used the slow\n> > variant of SHA1 instead of the fast-but-unsafe one. But that feels like\n> > the lesser evil compared to crashing.\n> \n> For posterity, Patrick and I used some of our monthly meeting this morning to\n> spend some time together pairing on this idea.\n> \n> It ended up being a dead-end, since this approach only protects you\n> against changing the hash function mid-stream, and not using the\n> incorrect context type from the union.\n> \n> That was along the lines of what I was originally thinking, and so I\n> resurrected my series to introduce 'unsafe_hash_algo()' here:\n> \n>     https://lore.kernel.org/git/cover.1736363652.git.me@ttaylorr.com/\n> \n> I got the impression that Patrick and I are on the same page there as\n> that being a good path forward, but I'll let him chime in in case I\n> misunderstood anything.\n\nNo misunderstanding, we're both on the same page. Thanks!\n\nPatrick\n"}]}