{"thread":{"id":"49579","subject":"[PATCH 0/3] Fix gc segfault","startedAt":"2018-10-16T21:02:53Z","lastAt":"2018-10-17T10:14:15Z","messageCount":6,"participants":["Jameson Miller via GitGitGadget","Johannes Schindelin via GitGitGadget","brian m. carlson","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"360666","messageId":"pull.47.git.gitgitgadget@gmail.com","threadId":"49579","inReplyTo":null,"subject":"[PATCH 0/3] Fix gc segfault","fromName":"Jameson Miller via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-16T21:02:47Z","receivedAt":"2018-10-16T21:02:53Z","isPatch":true,"sender":{"key":"jamill@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/2045976?v=4"},"body":"In 9ac3f0e (pack-objects: fix performance issues on packing large deltas,\n2018-07-22), a mutex was introduced that is used to guard the call to set\nthe delta size. This commit added code to initialize it, but at an incorrect\nspot: in init_threaded_search(), while the call to oe_set_delta_size() (and\nhence to packing_data_lock()) can happen in the call chain check_object() <- \nget_object_details() <-prepare_pack() <- cmd_pack_objects(), which is long\nbefore theprepare_pack() function calls ll_find_deltas() (which initializes\nthe threaded search).\n\nAnother tell-tale that the mutex was initialized in an incorrect spot is\nthat the function to initialize it lives in builtin/, while the code that\nuses the mutex is defined in a libgit.a header file.\n\nLet's use a more appropriate function: prepare_packing_data(), which not\nonly lives in libgit.a, but has to be called before thepacking_data struct\nis used that contains that mutex.\n\nJohannes Schindelin (3):\n Fix typo 'detla' -> 'delta'\n pack-objects (mingw): demonstrate a segmentation fault with large\n deltas\n pack-objects (mingw): initialize `packing_data` mutex in the correct\n spot\n\n builtin/pack-objects.c | 1 -\n pack-objects.c | 3 +++\n pack-objects.h | 2 +-\n t/t5321-pack-large-objects.sh | 32 ++++++++++++++++++++++++++++++++\n 4 files changed, 36 insertions(+), 2 deletions(-)\n create mode 100755 t/t5321-pack-large-objects.sh\n\n\nbase-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-47%2Fjamill%2Ffix-gc-segfault-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-47/jamill/fix-gc-segfault-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/47\n-- \ngitgitgadget\n"},{"id":"360667","messageId":"4b752704bd1dd676823d735757e8ed5e48eeba07.1539723766.git.gitgitgadget@gmail.com","threadId":"49579","inReplyTo":"pull.47.git.gitgitgadget@gmail.com","subject":"[PATCH 1/3] Fix typo 'detla' -> 'delta'","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-16T21:02:49Z","receivedAt":"2018-10-16T21:02:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nSigned-off-by: Johannes Schindelin \n---\n pack-objects.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pack-objects.h b/pack-objects.h\nindex 2ca39cfcfe..86ee93feb4 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -377,7 +377,7 @@ static inline unsigned long oe_delta_size(struct packing_data *pack,\n \t\treturn e->delta_size_;\n \n \t/*\n-\t * pack->detla_size[] can't be NULL because oe_set_delta_size()\n+\t * pack->delta_size[] can't be NULL because oe_set_delta_size()\n \t * must have been called when a new delta is saved with\n \t * oe_set_delta().\n \t * If oe_delta() returns NULL (i.e. default state, which means\n-- \ngitgitgadget\n\n"},{"id":"360668","messageId":"eb6d10133ac9b1977db81ff9c243e77639226f85.1539723766.git.gitgitgadget@gmail.com","threadId":"49579","inReplyTo":"pull.47.git.gitgitgadget@gmail.com","subject":"[PATCH 2/3] pack-objects (mingw): demonstrate a segmentation fault with large deltas","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-16T21:02:50Z","receivedAt":"2018-10-16T21:02:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThere is a problem in the way 9ac3f0e5b3e4 (pack-objects: fix\nperformance issues on packing large deltas, 2018-07-22) initializes that\nmutex in the `packing_data` struct. The problem manifests in a\nsegmentation fault on Windows, when a mutex (AKA critical section) is\naccessed without being initialized. (With pthreads, you apparently do\nnot really have to initialize them?)\n\nThis was reported in https://github.com/git-for-windows/git/issues/1839.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5321-pack-large-objects.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n create mode 100755 t/t5321-pack-large-objects.sh\n\ndiff --git a/t/t5321-pack-large-objects.sh b/t/t5321-pack-large-objects.sh\nnew file mode 100755\nindex 0000000000..c36c66fbb4\n--- /dev/null\n+++ b/t/t5321-pack-large-objects.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2018 Johannes Schindelin\n+#\n+\n+test_description='git pack-object with \"large\" deltas\n+\n+'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-pack.sh\n+\n+# Two similar-ish objects that we have computed deltas between.\n+A=01d7713666f4de822776c7622c10f1b07de280dc\n+B=e68fe8129b546b101aee9510c5328e7f21ca1d18\n+\n+test_expect_success 'setup' '\n+\tclear_packs &&\n+\t{\n+\t\tpack_header 2 &&\n+\t\tpack_obj $A $B &&\n+\t\tpack_obj $B\n+\t} >ab.pack &&\n+\tpack_trailer ab.pack &&\n+\tgit index-pack --stdin \n\nIn 9ac3f0e5b3e4 (pack-objects: fix performance issues on packing large\ndeltas, 2018-07-22), a mutex was introduced that is used to guard the\ncall to set the delta size. This commit even added code to initialize\nit, but at an incorrect spot: in `init_threaded_search()`, while the\ncall to `oe_set_delta_size()` (and hence to `packing_data_lock()`) can\nhappen in the call chain `check_object()` <- `get_object_details()` <-\n`prepare_pack()` <- `cmd_pack_objects()`, which is long before the\n`prepare_pack()` function calls `ll_find_deltas()` (which initializes\nthe threaded search).\n\nAnother tell-tale that the mutex was initialized in an incorrect spot is\nthat the function to initialize it lives in builtin/, while the code\nthat uses the mutex is defined in a libgit.a header file.\n\nLet's use a more appropriate function: `prepare_packing_data()`, which\nnot only lives in libgit.a, but *has* to be called before the\n`packing_data` struct is used that contains that mutex.\n\nThis fixes https://github.com/git-for-windows/git/issues/1839.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/pack-objects.c | 1 -\n pack-objects.c | 3 +++\n t/t5321-pack-large-objects.sh | 2 +-\n 3 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex e6316d294d..e752cf9c7a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2363,7 +2363,6 @@ static void init_threaded_search(void)\n \tpthread_mutex_init(&cache_mutex, NULL);\n \tpthread_mutex_init(&progress_mutex, NULL);\n \tpthread_cond_init(&progress_cond, NULL);\n-\tpthread_mutex_init(&to_pack.lock, NULL);\n \told_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads);\n }\n \ndiff --git a/pack-objects.c b/pack-objects.c\nindex 7e624c30eb..b6cdbb0166 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -148,6 +148,9 @@ void prepare_packing_data(struct packing_data *pdata)\n \t\t\t\t\t 1U << OE_SIZE_BITS);\n \tpdata->oe_delta_size_limit = git_env_ulong(\"GIT_TEST_OE_DELTA_SIZE\",\n \t\t\t\t\t\t 1UL << OE_DELTA_SIZE_BITS);\n+#ifndef NO_PTHREADS\n+\tpthread_mutex_init(&pdata->lock, NULL);\n+#endif\n }\n \n struct object_entry *packlist_alloc(struct packing_data *pdata,\ndiff --git a/t/t5321-pack-large-objects.sh b/t/t5321-pack-large-objects.sh\nindex c36c66fbb4..a75eab87d3 100755\n--- a/t/t5321-pack-large-objects.sh\n+++ b/t/t5321-pack-large-objects.sh\n@@ -24,7 +24,7 @@ test_expect_success 'setup' '\n \tgit index-pack --stdin From: Johannes Schindelin \n> \n> There is a problem in the way 9ac3f0e5b3e4 (pack-objects: fix\n> performance issues on packing large deltas, 2018-07-22) initializes that\n> mutex in the `packing_data` struct. The problem manifests in a\n> segmentation fault on Windows, when a mutex (AKA critical section) is\n> accessed without being initialized. (With pthreads, you apparently do\n> not really have to initialize them?)\n\nThis is a good catch. You do have to initialize a pthread mutex, but on\namd64 Linux the default initializer is all zeros, so since we use a\nstatic variable, it happens to coincidentally have the right value, so\nwe don't notice.\n\nThanks for fixing this.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"360737","messageId":"20181017101411.GA12554@sigill.intra.peff.net","threadId":"49579","inReplyTo":"8979693922364ef2e4a84a29298b081e86fa9ffa.1539723766.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] pack-objects (mingw): initialize `packing_data` mutex in the correct spot","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-17T10:14:12Z","receivedAt":"2018-10-17T10:14:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 16, 2018 at 02:02:52PM -0700, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> In 9ac3f0e5b3e4 (pack-objects: fix performance issues on packing large\n> deltas, 2018-07-22), a mutex was introduced that is used to guard the\n> call to set the delta size. This commit even added code to initialize\n> it, but at an incorrect spot: in `init_threaded_search()`, while the\n> call to `oe_set_delta_size()` (and hence to `packing_data_lock()`) can\n> happen in the call chain `check_object()` <- `get_object_details()` <-\n> `prepare_pack()` <- `cmd_pack_objects()`, which is long before the\n> `prepare_pack()` function calls `ll_find_deltas()` (which initializes\n> the threaded search).\n> \n> Another tell-tale that the mutex was initialized in an incorrect spot is\n> that the function to initialize it lives in builtin/, while the code\n> that uses the mutex is defined in a libgit.a header file.\n> \n> Let's use a more appropriate function: `prepare_packing_data()`, which\n> not only lives in libgit.a, but *has* to be called before the\n> `packing_data` struct is used that contains that mutex.\n\nNicely explained. I think this is a good solution.\n\nBoth before and after your patch, we do still take the lock even in\nsingle-threaded scenarios (the case you found where we are not yet in\nthe delta search phase, but also when --threads=1). I think that should\nbe fine. It looks like we do that with the other locks in\npack-objects.c already.\n\nIn index-pack.c, we check a threads_active flag before looking at the\nlock, which could be another possible solution. I doubt it's any faster,\nthough (which is why I assume index-pack.c does it). Locking/unlocking a\nmutex should not really be much slower than checking the conditional\nflag in the first place.\n\nWhich is all a roundabout way of saying \"looks good to me\".\n\n-Peff\n"}]}