{"thread":{"id":"51944","subject":"[PATCH] shallow.c: Don't free unallocated slabs","startedAt":"2019-09-30T23:33:17Z","lastAt":"2019-10-02T06:01:51Z","messageCount":3,"participants":["Ali Utku Selen","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"383186","messageId":"20190930233310.19287-1-auselen@gmail.com","threadId":"51944","inReplyTo":null,"subject":"[PATCH] shallow.c: Don't free unallocated slabs","fromName":"Ali Utku Selen","fromEmail":"auselen@gmail.com","sentAt":"2019-09-30T23:33:10Z","receivedAt":"2019-09-30T23:33:17Z","isPatch":true,"sender":{"key":"auselen@gmail.com","avatar":null},"body":"Fix possible segfault when cloning a submodule shallow.\n\nSigned-off-by: Ali Utku Selen <auselen@gmail.com>\n---\nIt is possible to have unallocated slabs in shallow.c's commit_depth\nfor a shallow submodule with many commits.\n\nEasiest way to reproduce this I found was changing COMMIT_SLAB_SIZE to\n32 and run t7406-submodule-update.sh. Segfault happens in case 50:\n\"submodule update clone shallow submodule outside of depth\"\n\n shallow.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/shallow.c b/shallow.c\nindex 5fa2b15d37..c33ab94bd7 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -156,6 +156,8 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n \tfor (i = 0; i < depths.slab_count; i++) {\n \t\tint j;\n \n+\t\tif (!depths.slab[i])\n+\t\t\tcontinue;\n \t\tfor (j = 0; j < depths.slab_size; j++)\n \t\t\tfree(depths.slab[i][j]);\n \t}\n-- \n2.17.1\n\n"},{"id":"383212","messageId":"20191001150729.GI10875@sigill.intra.peff.net","threadId":"51944","inReplyTo":"20190930233310.19287-1-auselen@gmail.com","subject":"Re: [PATCH] shallow.c: Don't free unallocated slabs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-01T15:07:29Z","receivedAt":"2019-10-01T15:07:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 01, 2019 at 01:33:10AM +0200, Ali Utku Selen wrote:\n\n> Fix possible segfault when cloning a submodule shallow.\n\nThanks. Just looking at the context, this is clearly the right thing to\nbe doing.\n\n> It is possible to have unallocated slabs in shallow.c's commit_depth\n> for a shallow submodule with many commits.\n\nYeah, the trick here is that we may over-allocate the slab list but not\nfill all of the entries. This is really an internal implementation\ndetail of how the slab code works. It would be nice if callers didn't\nhave to care about it. Perhaps we ought to have a slab foreach()\nfunction that encapsulates this, which would let this caller do\nsomething like:\n\n  commit_depth_foreach(&depths, free_commit_depth);\n  commit_depth_clear(&depths);\n\nBut since this is the only place that looks into the slab in this way,\nI'm happy to take your much simpler fix in the meantime.\n\n> Easiest way to reproduce this I found was changing COMMIT_SLAB_SIZE to\n> 32 and run t7406-submodule-update.sh. Segfault happens in case 50:\n> \"submodule update clone shallow submodule outside of depth\"\n\nIt would be nice to have a test, but I suspect it would be kind of\nexpensive, since it requires 512kb+ of entries (and would obviously be\ndepending on this arbitrary internal value). Given the simplicity of the\nfix, I think we can live without it.\n\n-Peff\n"},{"id":"383247","messageId":"xmqqftkbfzy1.fsf@gitster-ct.c.googlers.com","threadId":"51944","inReplyTo":"20191001150729.GI10875@sigill.intra.peff.net","subject":"Re: [PATCH] shallow.c: Don't free unallocated slabs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-02T06:01:42Z","receivedAt":"2019-10-02T06:01:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... This is really an internal implementation\n> detail of how the slab code works. It would be nice if callers didn't\n> have to care about it. Perhaps we ought to have a slab foreach()\n> function that encapsulates this, which would let this caller do\n> something like:\n>\n>   commit_depth_foreach(&depths, free_commit_depth);\n>   commit_depth_clear(&depths);\n>\n> But since this is the only place that looks into the slab in this way,\n> I'm happy to take your much simpler fix in the meantime.\n\nLikewise.  Thanks, both.\n"}]}