{"thread":{"id":"47927","subject":"Use of uninitialised value of size 8 in sha1_name.c","startedAt":"2018-02-26T09:04:30Z","lastAt":"2018-02-28T21:07:01Z","messageCount":14,"participants":["Christian Couder","Jeff King","Derrick Stolee","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"340276","messageId":"CAP8UFD23z9YDukO=O+cK=o_0DLcxbkXWzp4rCA1kRXGTZ-TMcQ@mail.gmail.com","threadId":"47927","inReplyTo":null,"subject":"Use of uninitialised value of size 8 in sha1_name.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-02-26T09:04:22Z","receivedAt":"2018-02-26T09:04:30Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Derrick,\n\nThese days when running:\n\n./t5616-partial-clone.sh --valgrind\n\non master, I get a bunch of:\n\n==21455== Use of uninitialised value of size 8\n==21455== at 0x2D2A73: get_hex_char_from_oid (sha1_name.c:492)\n==21455== by 0x2D2AFE: extend_abbrev_len (sha1_name.c:502)\n==21455== by 0x2D2C3D: find_abbrev_len_for_pack (sha1_name.c:551)\n==21455== by 0x2D2CFF: find_abbrev_len_packed (sha1_name.c:569)\n==21455== by 0x2D2E12: find_unique_abbrev_r (sha1_name.c:608)\n==21455== by 0x2DCB66: strbuf_add_unique_abbrev (strbuf.c:877)\n==21455== by 0x14F7CE: update_local_ref (fetch.c:700)\n==21455== by 0x1500CF: store_updated_refs (fetch.c:871)\n==21455== by 0x15035B: fetch_refs (fetch.c:932)\n==21455== by 0x150CF8: do_fetch (fetch.c:1146)\n==21455== by 0x1515AB: fetch_one (fetch.c:1370)\n==21455== by 0x151A1D: cmd_fetch (fetch.c:1457)\n==21455== Uninitialised value was created by a stack allocation\n==21455== at 0x2D2B2E: find_abbrev_len_for_pack (sha1_name.c:513)\n==21455==\n\nA quick git blame seems to point to 0e87b85683 (sha1_name: minimize\nOID comparisons during disambiguation, 2017-10-12).\n\nIt is difficult to tell for sure though as t5616-partial-clone.sh was\nadded after that commit.\n\nBest,\nChristian.\n"},{"id":"340279","messageId":"20180226095311.GA14831@sigill.intra.peff.net","threadId":"47927","inReplyTo":"CAP8UFD23z9YDukO=O+cK=o_0DLcxbkXWzp4rCA1kRXGTZ-TMcQ@mail.gmail.com","subject":"Re: Use of uninitialised value of size 8 in sha1_name.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-26T09:53:11Z","receivedAt":"2018-02-26T09:53:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 26, 2018 at 10:04:22AM +0100, Christian Couder wrote:\n\n> ==21455== Use of uninitialised value of size 8\n> ==21455== at 0x2D2A73: get_hex_char_from_oid (sha1_name.c:492)\n> ==21455== by 0x2D2AFE: extend_abbrev_len (sha1_name.c:502)\n> ==21455== by 0x2D2C3D: find_abbrev_len_for_pack (sha1_name.c:551)\n> ==21455== by 0x2D2CFF: find_abbrev_len_packed (sha1_name.c:569)\n> ==21455== by 0x2D2E12: find_unique_abbrev_r (sha1_name.c:608)\n> ==21455== by 0x2DCB66: strbuf_add_unique_abbrev (strbuf.c:877)\n> ==21455== by 0x14F7CE: update_local_ref (fetch.c:700)\n> ==21455== by 0x1500CF: store_updated_refs (fetch.c:871)\n> ==21455== by 0x15035B: fetch_refs (fetch.c:932)\n> ==21455== by 0x150CF8: do_fetch (fetch.c:1146)\n> ==21455== by 0x1515AB: fetch_one (fetch.c:1370)\n> ==21455== by 0x151A1D: cmd_fetch (fetch.c:1457)\n> ==21455== Uninitialised value was created by a stack allocation\n> ==21455== at 0x2D2B2E: find_abbrev_len_for_pack (sha1_name.c:513)\n> ==21455==\n> \n> A quick git blame seems to point to 0e87b85683 (sha1_name: minimize\n> OID comparisons during disambiguation, 2017-10-12).\n> \n> It is difficult to tell for sure though as t5616-partial-clone.sh was\n> added after that commit.\n\nI think that commit is to blame, though the error isn't exactly where\nthat stack trace puts it. Try this:\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 611c7d24dd..6f7f36436f 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -547,7 +547,10 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n \t */\n \tmad->init_len = 0;\n \tif (!match) {\n-\t\tnth_packed_object_oid(&oid, p, first);\n+\t\twarning(\"p->num_objects = %u, first = %u\",\n+\t\t\tp->num_objects, first);\n+\t\tif (!nth_packed_object_oid(&oid, p, first))\n+\t\t\tdie(\"oops!\");\n \t\textend_abbrev_len(&oid, mad);\n \t} else if (first < num - 1) {\n \t\tnth_packed_object_oid(&oid, p, first + 1);\n\nI get failures all over the test suite, like this:\n\n warning: p->num_objects = 4, first = 3\n warning: p->num_objects = 8, first = 6\n warning: p->num_objects = 10, first = 0\n warning: p->num_objects = 4, first = 0\n warning: p->num_objects = 8, first = 0\n warning: p->num_objects = 10, first = 10\n fatal: oops!\n\nAny time the abbreviated hex would go after the last object in the pack,\nthen first==p->num_objects, and nth_packed_object_oid() will fail. That\nleaves uninitialized data in \"oid\", which is what valgrind complains\nabout when we examine it in extend_abbrev_len().\n\nMost of the time the code behaves correctly anyway, because the\nuninitialized bytes are unlikely to match up with our hex and extend the\nlength. Probably we just need to detect that case and skip the call to\nextend_abbrev_len() altogether.\n\n-Peff\n"},{"id":"340281","messageId":"CAP8UFD01wkpNuXSHxQTETi3+tWBM0E=iYXQeT2r7tGs=2Yq2EA@mail.gmail.com","threadId":"47927","inReplyTo":"20180226095311.GA14831@sigill.intra.peff.net","subject":"Re: Use of uninitialised value of size 8 in sha1_name.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-02-26T10:23:10Z","receivedAt":"2018-02-26T10:23:17Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Feb 26, 2018 at 10:53 AM, Jeff King wrote:\n> On Mon, Feb 26, 2018 at 10:04:22AM +0100, Christian Couder wrote:\n>\n>> ==21455== Use of uninitialised value of size 8\n>> ==21455== at 0x2D2A73: get_hex_char_from_oid (sha1_name.c:492)\n>> ==21455== by 0x2D2AFE: extend_abbrev_len (sha1_name.c:502)\n>> ==21455== by 0x2D2C3D: find_abbrev_len_for_pack (sha1_name.c:551)\n>> ==21455== by 0x2D2CFF: find_abbrev_len_packed (sha1_name.c:569)\n>> ==21455== by 0x2D2E12: find_unique_abbrev_r (sha1_name.c:608)\n>> ==21455== by 0x2DCB66: strbuf_add_unique_abbrev (strbuf.c:877)\n>> ==21455== by 0x14F7CE: update_local_ref (fetch.c:700)\n>> ==21455== by 0x1500CF: store_updated_refs (fetch.c:871)\n>> ==21455== by 0x15035B: fetch_refs (fetch.c:932)\n>> ==21455== by 0x150CF8: do_fetch (fetch.c:1146)\n>> ==21455== by 0x1515AB: fetch_one (fetch.c:1370)\n>> ==21455== by 0x151A1D: cmd_fetch (fetch.c:1457)\n>> ==21455== Uninitialised value was created by a stack allocation\n>> ==21455== at 0x2D2B2E: find_abbrev_len_for_pack (sha1_name.c:513)\n>> ==21455==\n>>\n>> A quick git blame seems to point to 0e87b85683 (sha1_name: minimize\n>> OID comparisons during disambiguation, 2017-10-12).\n>>\n>> It is difficult to tell for sure though as t5616-partial-clone.sh was\n>> added after that commit.\n>\n> I think that commit is to blame, though the error isn't exactly where\n> that stack trace puts it. Try this:\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 611c7d24dd..6f7f36436f 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -547,7 +547,10 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n> */\n> mad->init_len = 0;\n> if (!match) {\n> - nth_packed_object_oid(&oid, p, first);\n> + warning(\"p->num_objects = %u, first = %u\",\n> + p->num_objects, first);\n> + if (!nth_packed_object_oid(&oid, p, first))\n> + die(\"oops!\");\n> extend_abbrev_len(&oid, mad);\n> } else if (first < num - 1) {\n> nth_packed_object_oid(&oid, p, first + 1);\n>\n> I get failures all over the test suite, like this:\n>\n> warning: p->num_objects = 4, first = 3\n> warning: p->num_objects = 8, first = 6\n> warning: p->num_objects = 10, first = 0\n> warning: p->num_objects = 4, first = 0\n> warning: p->num_objects = 8, first = 0\n> warning: p->num_objects = 10, first = 10\n> fatal: oops!\n\nYeah, I tried to t5601-clone.sh with --valgrind and I also get one\nerror (the same \"Uninitialised value\" error actually).\n\n> Any time the abbreviated hex would go after the last object in the pack,\n> then first==p->num_objects, and nth_packed_object_oid() will fail. That\n> leaves uninitialized data in \"oid\", which is what valgrind complains\n> about when we examine it in extend_abbrev_len().\n>\n> Most of the time the code behaves correctly anyway, because the\n> uninitialized bytes are unlikely to match up with our hex and extend the\n> length. Probably we just need to detect that case and skip the call to\n> extend_abbrev_len() altogether.\n\nYeah, something like:\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 611c7d24dd..a041d8d24f 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n */\n mad->init_len = 0;\n if (!match) {\n- nth_packed_object_oid(&oid, p, first);\n- extend_abbrev_len(&oid, mad);\n+ if (nth_packed_object_oid(&oid, p, first))\n+ extend_abbrev_len(&oid, mad);\n } else if (first < num - 1) {\n- nth_packed_object_oid(&oid, p, first + 1);\n- extend_abbrev_len(&oid, mad);\n+ if (nth_packed_object_oid(&oid, p, first + 1))\n+ extend_abbrev_len(&oid, mad);\n }\n if (first > 0) {\n- nth_packed_object_oid(&oid, p, first - 1);\n- extend_abbrev_len(&oid, mad);\n+ if (nth_packed_object_oid(&oid, p, first - 1))\n+ extend_abbrev_len(&oid, mad);\n }\n mad->init_len = mad->cur_len;\n }\n\nseems to prevent valgrind from complaining when running t5616-partial-clone.sh.\n"},{"id":"340302","messageId":"0a85ea3b-3f64-f67d-b4d5-a761cbc4c6db@gmail.com","threadId":"47927","inReplyTo":"CAP8UFD01wkpNuXSHxQTETi3+tWBM0E=iYXQeT2r7tGs=2Yq2EA@mail.gmail.com","subject":"Re: Use of uninitialised value of size 8 in sha1_name.c","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-02-26T14:06:27Z","receivedAt":"2018-02-26T14:06:35Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/26/2018 5:23 AM, Christian Couder wrote:\n> On Mon, Feb 26, 2018 at 10:53 AM, Jeff King wrote:\n>> On Mon, Feb 26, 2018 at 10:04:22AM +0100, Christian Couder wrote:\n>>\n>>> ==21455== Use of uninitialised value of size 8\n>>> ==21455== at 0x2D2A73: get_hex_char_from_oid (sha1_name.c:492)\n>>> ==21455== by 0x2D2AFE: extend_abbrev_len (sha1_name.c:502)\n>>> ==21455== by 0x2D2C3D: find_abbrev_len_for_pack (sha1_name.c:551)\n>>> ==21455== by 0x2D2CFF: find_abbrev_len_packed (sha1_name.c:569)\n>>> ==21455== by 0x2D2E12: find_unique_abbrev_r (sha1_name.c:608)\n>>> ==21455== by 0x2DCB66: strbuf_add_unique_abbrev (strbuf.c:877)\n>>> ==21455== by 0x14F7CE: update_local_ref (fetch.c:700)\n>>> ==21455== by 0x1500CF: store_updated_refs (fetch.c:871)\n>>> ==21455== by 0x15035B: fetch_refs (fetch.c:932)\n>>> ==21455== by 0x150CF8: do_fetch (fetch.c:1146)\n>>> ==21455== by 0x1515AB: fetch_one (fetch.c:1370)\n>>> ==21455== by 0x151A1D: cmd_fetch (fetch.c:1457)\n>>> ==21455== Uninitialised value was created by a stack allocation\n>>> ==21455== at 0x2D2B2E: find_abbrev_len_for_pack (sha1_name.c:513)\n>>> ==21455==\n>>>\n>>> A quick git blame seems to point to 0e87b85683 (sha1_name: minimize\n>>> OID comparisons during disambiguation, 2017-10-12).\n>>>\n>>> It is difficult to tell for sure though as t5616-partial-clone.sh was\n>>> added after that commit.\n>> I think that commit is to blame, though the error isn't exactly where\n>> that stack trace puts it. Try this:\n>>\n>> diff --git a/sha1_name.c b/sha1_name.c\n>> index 611c7d24dd..6f7f36436f 100644\n>> --- a/sha1_name.c\n>> +++ b/sha1_name.c\n>> @@ -547,7 +547,10 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n>> */\n>> mad->init_len = 0;\n>> if (!match) {\n>> - nth_packed_object_oid(&oid, p, first);\n>> + warning(\"p->num_objects = %u, first = %u\",\n>> + p->num_objects, first);\n>> + if (!nth_packed_object_oid(&oid, p, first))\n>> + die(\"oops!\");\n>> extend_abbrev_len(&oid, mad);\n>> } else if (first < num - 1) {\n>> nth_packed_object_oid(&oid, p, first + 1);\n>>\n>> I get failures all over the test suite, like this:\n>>\n>> warning: p->num_objects = 4, first = 3\n>> warning: p->num_objects = 8, first = 6\n>> warning: p->num_objects = 10, first = 0\n>> warning: p->num_objects = 4, first = 0\n>> warning: p->num_objects = 8, first = 0\n>> warning: p->num_objects = 10, first = 10\n>> fatal: oops!\n> Yeah, I tried to t5601-clone.sh with --valgrind and I also get one\n> error (the same \"Uninitialised value\" error actually).\n\nThanks for finding this. Sorry for the bug.\n\n>> Any time the abbreviated hex would go after the last object in the pack,\n>> then first==p->num_objects, and nth_packed_object_oid() will fail. That\n>> leaves uninitialized data in \"oid\", which is what valgrind complains\n>> about when we examine it in extend_abbrev_len().\n>>\n>> Most of the time the code behaves correctly anyway, because the\n>> uninitialized bytes are unlikely to match up with our hex and extend the\n>> length. Probably we just need to detect that case and skip the call to\n>> extend_abbrev_len() altogether.\n> Yeah, something like:\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 611c7d24dd..a041d8d24f 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n> */\n> mad->init_len = 0;\n> if (!match) {\n> - nth_packed_object_oid(&oid, p, first);\n> - extend_abbrev_len(&oid, mad);\n> + if (nth_packed_object_oid(&oid, p, first))\n> + extend_abbrev_len(&oid, mad);\n> } else if (first < num - 1) {\n> - nth_packed_object_oid(&oid, p, first + 1);\n> - extend_abbrev_len(&oid, mad);\n> + if (nth_packed_object_oid(&oid, p, first + 1))\n> + extend_abbrev_len(&oid, mad);\n> }\n> if (first > 0) {\n> - nth_packed_object_oid(&oid, p, first - 1);\n> - extend_abbrev_len(&oid, mad);\n> + if (nth_packed_object_oid(&oid, p, first - 1))\n> + extend_abbrev_len(&oid, mad);\n> }\n> mad->init_len = mad->cur_len;\n> }\n>\n> seems to prevent valgrind from complaining when running t5616-partial-clone.sh.\n\nThis seems like the safest fix, but also we could use our values of \n\"match\", \"first\" and \"num\" to safely call nth_packed_object_oid(). \nHowever, since nth_packed_object_oid() checks the bounds, don't \nduplicate work and just use the return value.\n\nI would reformat your patch slightly, but that's just preference:\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 611c7d2..97b632c 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -546,17 +546,14 @@ static void find_abbrev_len_for_pack(struct \npacked_git *p,\n          * nearby for the abbreviation length.\n          */\n         mad->init_len = 0;\n-       if (!match) {\n-               nth_packed_object_oid(&oid, p, first);\n+       if (!match && nth_packed_object_oid(&oid, p, first))\n                 extend_abbrev_len(&oid, mad);\n-       } else if (first < num - 1) {\n-               nth_packed_object_oid(&oid, p, first + 1);\n+       else if (first < num - 1 &&\n+                nth_packed_object_oid(&oid, p, first + 1))\n                 extend_abbrev_len(&oid, mad);\n-       }\n-       if (first > 0) {\n-               nth_packed_object_oid(&oid, p, first - 1);\n+       if (first > 0 && nth_packed_object_oid(&oid, p, first - 1))\n                 extend_abbrev_len(&oid, mad);\n-       }\n+\n         mad->init_len = mad->cur_len;\n  }\n\nChristian: do you want to submit the patch, or should I put one together?\n\nThanks,\n-Stolee\n\n"},{"id":"340304","messageId":"CAP8UFD0CesVk8FjX-=unwSO6u0pi1-Usm1yVW5hs5FHi=8Timg@mail.gmail.com","threadId":"47927","inReplyTo":"0a85ea3b-3f64-f67d-b4d5-a761cbc4c6db@gmail.com","subject":"Re: Use of uninitialised value of size 8 in sha1_name.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-02-26T14:43:33Z","receivedAt":"2018-02-26T14:43:39Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Feb 26, 2018 at 3:06 PM, Derrick Stolee wrote:\n>\n> Christian: do you want to submit the patch, or should I put one together?\n\nI'd rather have you put one together.\n\nThanks,\nChristian.\n"},{"id":"340306","messageId":"1519657007-215623-1-git-send-email-dstolee@microsoft.com","threadId":"47927","inReplyTo":"CAP8UFD0CesVk8FjX-=unwSO6u0pi1-Usm1yVW5hs5FHi=8Timg@mail.gmail.com","subject":"[PATCH] sha1_name: fix uninitialized memory errors","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-02-26T14:56:47Z","receivedAt":"2018-02-26T14:57:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"During abbreviation checks, we navigate to the position within a\npack-index that an OID would be inserted and check surrounding OIDs\nfor the maximum matching prefix. This position may be beyond the\nlast position, because the given OID is lexicographically larger\nthan every OID in the pack. Then nth_packed_object_oid() does not\ninitialize \"oid\".\n\nUse the return value of nth_packed_object_oid() to prevent these\nerrors.\n\nReported-by: Christian Couder \nSigned-off-by: Derrick Stolee \n---\n sha1_name.c | 11 +++--------\n 1 file changed, 3 insertions(+), 8 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 611c7d2..44dd595 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -546,17 +546,12 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n \t * nearby for the abbreviation length.\n \t */\n \tmad->init_len = 0;\n-\tif (!match) {\n-\t\tnth_packed_object_oid(&oid, p, first);\n+\tif (!match && nth_packed_object_oid(&oid, p, first))\n \t\textend_abbrev_len(&oid, mad);\n-\t} else if (first < num - 1) {\n-\t\tnth_packed_object_oid(&oid, p, first + 1);\n+\telse if (first < num - 1 && nth_packed_object_oid(&oid, p, first + 1))\n \t\textend_abbrev_len(&oid, mad);\n-\t}\n-\tif (first > 0) {\n-\t\tnth_packed_object_oid(&oid, p, first - 1);\n+\tif (first > 0 && nth_packed_object_oid(&oid, p, first - 1))\n \t\textend_abbrev_len(&oid, mad);\n-\t}\n \tmad->init_len = mad->cur_len;\n }\n \n-- \n2.7.4\n\n"},{"id":"340323","messageId":"20180226204151.GA12598@sigill.intra.peff.net","threadId":"47927","inReplyTo":"1519657007-215623-1-git-send-email-dstolee@microsoft.com","subject":"Re: [PATCH] sha1_name: fix uninitialized memory errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-26T20:41:52Z","receivedAt":"2018-02-26T20:42:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 26, 2018 at 09:56:47AM -0500, Derrick Stolee wrote:\n\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 611c7d2..44dd595 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -546,17 +546,12 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n> \t * nearby for the abbreviation length.\n> \t */\n> \tmad->init_len = 0;\n> -\tif (!match) {\n> -\t\tnth_packed_object_oid(&oid, p, first);\n> +\tif (!match && nth_packed_object_oid(&oid, p, first))\n> \t\textend_abbrev_len(&oid, mad);\n> -\t} else if (first < num - 1) {\n> -\t\tnth_packed_object_oid(&oid, p, first + 1);\n> +\telse if (first < num - 1 && nth_packed_object_oid(&oid, p, first + 1))\n> \t\textend_abbrev_len(&oid, mad);\n> -\t}\n\nI think including the nth_packed_object_oid() in the main if-else chain\nworks out, but it's kind of tricky.\n\nIn the code before, we'd hit the \"first < num - 1\" conditional only when\nwe didn't match something. But now we also hit it if we _did_ match\nsomething, but nth_packed_object_oid() didn't work.\n\nBut this works out the same if we assume any match must also succeed at\nnth_packed_object_oid(). Which in turn implies that checking the result\nof nth_packed_object_oid() in the \"else if\" is redundant (though we\nalready clamp it to \"num - 1\", so we'd expect it to always succeed\nanyway).\n\nSo I think this behaves well, but I wonder if the two-level conditionals\nlike:\n\n if (!match) {\n\tif (nth_packed_object_oid(&oid, p, first))\n\t\textend_abbrev_len(&oid, mad);\n } else if ...\n\nare easier to reason about.\n\n-Peff\n"},{"id":"340408","messageId":"20180227114704.193145-1-dstolee@microsoft.com","threadId":"47927","inReplyTo":"20180226204151.GA12598@sigill.intra.peff.net","subject":"[PATCH v2] sha1_name: fix uninitialized memory errors","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-02-27T11:47:04Z","receivedAt":"2018-02-27T11:47:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Peff made an excellent point about the nested if statements. This\ngoes back to Christian's original recommendation.\n\n-- >8 --\n\nDuring abbreviation checks, we navigate to the position within a\npack-index that an OID would be inserted and check surrounding OIDs\nfor the maximum matching prefix. This position may be beyond the\nlast position, because the given OID is lexicographically larger\nthan every OID in the pack. Then nth_packed_object_oid() does not\ninitialize \"oid\".\n\nUse the return value of nth_packed_object_oid() to prevent these\nerrors.\n\nReported-by: Christian Couder \nSigned-off-by: Derrick Stolee \n---\n sha1_name.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 611c7d24dd..a041d8d24f 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n \t */\n \tmad->init_len = 0;\n \tif (!match) {\n-\t\tnth_packed_object_oid(&oid, p, first);\n-\t\textend_abbrev_len(&oid, mad);\n+\t\tif (nth_packed_object_oid(&oid, p, first))\n+\t\t\textend_abbrev_len(&oid, mad);\n \t} else if (first < num - 1) {\n-\t\tnth_packed_object_oid(&oid, p, first + 1);\n-\t\textend_abbrev_len(&oid, mad);\n+\t\tif (nth_packed_object_oid(&oid, p, first + 1))\n+\t\t\textend_abbrev_len(&oid, mad);\n \t}\n \tif (first > 0) {\n-\t\tnth_packed_object_oid(&oid, p, first - 1);\n-\t\textend_abbrev_len(&oid, mad);\n+\t\tif (nth_packed_object_oid(&oid, p, first - 1))\n+\t\t\textend_abbrev_len(&oid, mad);\n \t}\n \tmad->init_len = mad->cur_len;\n }\n-- \n2.16.2.265.g3d5930c0b9.dirty\n\n"},{"id":"340458","messageId":"20180227213305.GB6899@sigill.intra.peff.net","threadId":"47927","inReplyTo":"20180227114704.193145-1-dstolee@microsoft.com","subject":"Re: [PATCH v2] sha1_name: fix uninitialized memory errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-27T21:33:05Z","receivedAt":"2018-02-27T21:33:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 27, 2018 at 06:47:04AM -0500, Derrick Stolee wrote:\n\n> Peff made an excellent point about the nested if statements. This\n> goes back to Christian's original recommendation.\n> \n> -- >8 --\n> \n> During abbreviation checks, we navigate to the position within a\n> pack-index that an OID would be inserted and check surrounding OIDs\n> for the maximum matching prefix. This position may be beyond the\n> last position, because the given OID is lexicographically larger\n> than every OID in the pack. Then nth_packed_object_oid() does not\n> initialize \"oid\".\n> \n> Use the return value of nth_packed_object_oid() to prevent these\n> errors.\n> \n> Reported-by: Christian Couder \n> Signed-off-by: Derrick Stolee \n\nThanks, this looks good to me.\n\nSemi-related, I wondered also at the weird asymmetry in the if-else,\nwhich is:\n\n if ...\n else if ...\n if ...\n\nbut the comment directly above says: \"we consider a maximum of three\nobjects nearby\". I think it's actually two, because you can only trigger\none of the first two conditionals.\n\nIs that right?\n\nLet's imagine we're looking for object 1234abcd. If we didn't find a\nmatch, then we might have:\n\n 1234abcc \n 1234abce <-- first points here\n\nin which case we need to check both first-1 and first. And we do.\n\nIf we do have a match, then we might see:\n\n 1234abcc\n 1234abcd <-- first points here\n 1234abce\n\nand we must check first-1 and first+1, but _not_ first.\n\nSo I think the code is right, but the comment is wrong.\n\n-Peff\n"},{"id":"340476","messageId":"xmqqsh9mnjzk.fsf@gitster-ct.c.googlers.com","threadId":"47927","inReplyTo":"20180227213305.GB6899@sigill.intra.peff.net","subject":"Re: [PATCH v2] sha1_name: fix uninitialized memory errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-27T22:30:39Z","receivedAt":"2018-02-27T22:30:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Thanks, this looks good to me.\n>\n> Semi-related, I wondered also at the weird asymmetry in the if-else,\n> ...\n> So I think the code is right, but the comment is wrong.\n\n\n\n-- >8 --\nFrom: Derrick Stolee \nDate: Tue, 27 Feb 2018 06:47:04 -0500\nSubject: [PATCH] sha1_name: fix uninitialized memory errors\n\nDuring abbreviation checks, we navigate to the position within a\npack-index that an OID would be inserted and check surrounding OIDs\nfor the maximum matching prefix. This position may be beyond the\nlast position, because the given OID is lexicographically larger\nthan every OID in the pack. Then nth_packed_object_oid() does not\ninitialize \"oid\".\n\nUse the return value of nth_packed_object_oid() to prevent these\nerrors.\n\nAlso the comment about checking near-by objects miscounts the\nneighbours. If we have a hit at \"first\", we check \"first-1\" and\n\"first+1\" to make sure we have sufficiently long abbreviation not to\nmatch either. If we do not have a hit, \"first\" is the smallest\namong the objects that are larger than what we want to name, so we\ncheck that and \"first-1\" to make sure we have sufficiently long\nabbreviation not to match either. In either case, we only check up\nto two near-by objects.\n\nReported-by: Christian Couder \nSigned-off-by: Derrick Stolee \nSigned-off-by: Junio C Hamano \n---\n sha1_name.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 05a635911b..f1c3d37a6d 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -542,20 +542,20 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n \t/*\n \t * first is now the position in the packfile where we would insert\n \t * mad->hash if it does not exist (or the position of mad->hash if\n-\t * it does exist). Hence, we consider a maximum of three objects\n+\t * it does exist). Hence, we consider a maximum of two objects\n \t * nearby for the abbreviation length.\n \t */\n \tmad->init_len = 0;\n \tif (!match) {\n-\t\tnth_packed_object_oid(&oid, p, first);\n-\t\textend_abbrev_len(&oid, mad);\n+\t\tif (nth_packed_object_oid(&oid, p, first))\n+\t\t\textend_abbrev_len(&oid, mad);\n \t} else if (first < num - 1) {\n-\t\tnth_packed_object_oid(&oid, p, first + 1);\n-\t\textend_abbrev_len(&oid, mad);\n+\t\tif (nth_packed_object_oid(&oid, p, first + 1))\n+\t\t\textend_abbrev_len(&oid, mad);\n \t}\n \tif (first > 0) {\n-\t\tnth_packed_object_oid(&oid, p, first - 1);\n-\t\textend_abbrev_len(&oid, mad);\n+\t\tif (nth_packed_object_oid(&oid, p, first - 1))\n+\t\t\textend_abbrev_len(&oid, mad);\n \t}\n \tmad->init_len = mad->cur_len;\n }\n-- \n2.16.2-325-g2fc74f41c5\n\n"},{"id":"340479","messageId":"20180227224057.GB3484@sigill.intra.peff.net","threadId":"47927","inReplyTo":"xmqqsh9mnjzk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] sha1_name: fix uninitialized memory errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-27T22:40:57Z","receivedAt":"2018-02-27T22:41:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 27, 2018 at 02:30:39PM -0800, Junio C Hamano wrote:\n\n> -- >8 --\n> From: Derrick Stolee \n> Date: Tue, 27 Feb 2018 06:47:04 -0500\n> Subject: [PATCH] sha1_name: fix uninitialized memory errors\n> \n> During abbreviation checks, we navigate to the position within a\n> pack-index that an OID would be inserted and check surrounding OIDs\n> for the maximum matching prefix. This position may be beyond the\n> last position, because the given OID is lexicographically larger\n> than every OID in the pack. Then nth_packed_object_oid() does not\n> initialize \"oid\".\n> \n> Use the return value of nth_packed_object_oid() to prevent these\n> errors.\n> \n> Also the comment about checking near-by objects miscounts the\n> neighbours. If we have a hit at \"first\", we check \"first-1\" and\n> \"first+1\" to make sure we have sufficiently long abbreviation not to\n> match either. If we do not have a hit, \"first\" is the smallest\n> among the objects that are larger than what we want to name, so we\n> check that and \"first-1\" to make sure we have sufficiently long\n> abbreviation not to match either. In either case, we only check up\n> to two near-by objects.\n\nYep, this looks good to me. Thanks for wrapping it up.\n\n-Peff\n"},{"id":"340615","messageId":"xmqqy3jckfe3.fsf@gitster-ct.c.googlers.com","threadId":"47927","inReplyTo":"20180227114704.193145-1-dstolee@microsoft.com","subject":"Re: [PATCH v2] sha1_name: fix uninitialized memory errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-28T20:50:28Z","receivedAt":"2018-02-28T20:50:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 611c7d24dd..a041d8d24f 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n> \t */\n> \tmad->init_len = 0;\n> \tif (!match) {\n> -\t\tnth_packed_object_oid(&oid, p, first);\n> -\t\textend_abbrev_len(&oid, mad);\n> +\t\tif (nth_packed_object_oid(&oid, p, first))\n> +\t\t\textend_abbrev_len(&oid, mad);\n> \t} else if (first < num - 1) {\n> -\t\tnth_packed_object_oid(&oid, p, first + 1);\n> -\t\textend_abbrev_len(&oid, mad);\n> +\t\tif (nth_packed_object_oid(&oid, p, first + 1))\n> +\t\t\textend_abbrev_len(&oid, mad);\n> \t}\n> \tif (first > 0) {\n> -\t\tnth_packed_object_oid(&oid, p, first - 1);\n> -\t\textend_abbrev_len(&oid, mad);\n> +\t\tif (nth_packed_object_oid(&oid, p, first - 1))\n> +\t\t\textend_abbrev_len(&oid, mad);\n> \t}\n> \tmad->init_len = mad->cur_len;\n> }\n\nI do not think they are wrong, but aren't the latter two somewhat\nredundant? \"num\" is p->num_objects, and we call (first+1)th element\nonly after we see (first < num - 1), i.e. first+1 < num, and the\naccess to (first-1)th is done only when first > 0. The first one,\ni.e. when first points at where we _would_ find it if it existed,\ncan access \"first\" that could be p->num_objects, so the change there\nmakes sense, though.\n\n"},{"id":"340617","messageId":"ce8421bb-4519-c4e5-ff0a-98ae532d764b@gmail.com","threadId":"47927","inReplyTo":"xmqqy3jckfe3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] sha1_name: fix uninitialized memory errors","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-02-28T20:57:26Z","receivedAt":"2018-02-28T20:57:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/28/2018 3:50 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n>\n>> diff --git a/sha1_name.c b/sha1_name.c\n>> index 611c7d24dd..a041d8d24f 100644\n>> --- a/sha1_name.c\n>> +++ b/sha1_name.c\n>> @@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n>> \t */\n>> \tmad->init_len = 0;\n>> \tif (!match) {\n>> -\t\tnth_packed_object_oid(&oid, p, first);\n>> -\t\textend_abbrev_len(&oid, mad);\n>> +\t\tif (nth_packed_object_oid(&oid, p, first))\n>> +\t\t\textend_abbrev_len(&oid, mad);\n>> \t} else if (first < num - 1) {\n>> -\t\tnth_packed_object_oid(&oid, p, first + 1);\n>> -\t\textend_abbrev_len(&oid, mad);\n>> +\t\tif (nth_packed_object_oid(&oid, p, first + 1))\n>> +\t\t\textend_abbrev_len(&oid, mad);\n>> \t}\n>> \tif (first > 0) {\n>> -\t\tnth_packed_object_oid(&oid, p, first - 1);\n>> -\t\textend_abbrev_len(&oid, mad);\n>> +\t\tif (nth_packed_object_oid(&oid, p, first - 1))\n>> +\t\t\textend_abbrev_len(&oid, mad);\n>> \t}\n>> \tmad->init_len = mad->cur_len;\n>> }\n> I do not think they are wrong, but aren't the latter two somewhat\n> redundant? \"num\" is p->num_objects, and we call (first+1)th element\n> only after we see (first < num - 1), i.e. first+1 < num, and the\n> access to (first-1)th is done only when first > 0. The first one,\n> i.e. when first points at where we _would_ find it if it existed,\n> can access \"first\" that could be p->num_objects, so the change there\n> makes sense, though.\n>\n\nYes. But I'd rather keep the blocks consistent and use the return value \nof nth_packed_object_oid() when possible.\n\nThanks,\n-Stolee\n"},{"id":"340618","messageId":"xmqqr2p4kemp.fsf@gitster-ct.c.googlers.com","threadId":"47927","inReplyTo":"ce8421bb-4519-c4e5-ff0a-98ae532d764b@gmail.com","subject":"Re: [PATCH v2] sha1_name: fix uninitialized memory errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-28T21:06:54Z","receivedAt":"2018-02-28T21:07:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> I do not think they are wrong, but aren't the latter two somewhat\n>> redundant? \"num\" is p->num_objects, and we call (first+1)th element\n>> only after we see (first < num - 1), i.e. first+1 < num, and the\n>> access to (first-1)th is done only when first > 0. The first one,\n>> i.e. when first points at where we _would_ find it if it existed,\n>> can access \"first\" that could be p->num_objects, so the change there\n>> makes sense, though.\n>\n> Yes. But I'd rather keep the blocks consistent and use the return\n> value of nth_packed_object_oid() when possible.\n\nSure, I do not think anybody minds; I just wanted a sanity check.\n\nThansk.\n"}]}