{"thread":{"id":"45570","subject":"[PATCH] name-hash: fix buffer overrun","startedAt":"2017-03-31T17:32:52Z","lastAt":"2017-04-02T16:46:53Z","messageCount":11,"participants":["git@jeffhostetler.com","Junio C Hamano","Johannes Schindelin","Jakub Narębski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"315933","messageId":"20170331173214.47514-1-git@jeffhostetler.com","threadId":"45570","inReplyTo":null,"subject":"[PATCH] name-hash: fix buffer overrun","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-31T17:32:13Z","receivedAt":"2017-03-31T17:32:52Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nFix buffer overrun in handle_range_dir() when the final entry\nin the index was the only file in the last directory, such as\n\"a/b/foo.txt\". The look ahead (k_start + 1) was invalid since\n(k_start + 1) == k_end.\n\nThis bug was introduced by Jeff in \"jh/memihash-opt\" which was\nrecently merged into master.\n\nKevin Willford (1):\n name-hash: fix buffer overrun\n\n name-hash.c | 4 +++-\n t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n create mode 100644 t/t3008-ls-files-lazy-init-name-hash.sh\n\n-- \n2.9.3\n\n"},{"id":"315934","messageId":"20170331173214.47514-2-git@jeffhostetler.com","threadId":"45570","inReplyTo":"20170331173214.47514-1-git@jeffhostetler.com","subject":"[PATCH] name-hash: fix buffer overrun","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-31T17:32:14Z","receivedAt":"2017-03-31T17:32:58Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Kevin Willford \n\nAdd check for the end of the entries for the thread partition.\nAdd test for lazy init name hash with specific directory structure\n\nThe lazy init hash name was causing a buffer overflow when the last\nentry in the index was multiple folder deep with parent folders that\ndid not have any files in them.\n\nThis adds a test for the boundary condition of the thread partitions\nwith the folder structure that was triggering the buffer overflow.\n\nThe fix was to check if it is the last entry for the thread partition\nin the handle_range_dir and not try to use the next entry in the cache.\n\nSigned-off-by: Kevin Willford \nSigned-off-by: Johannes Schindelin \nSigned-off-by: Jeff Hostetler \n\n---\n name-hash.c | 4 +++-\n t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n create mode 100644 t/t3008-ls-files-lazy-init-name-hash.sh\n\ndiff --git a/name-hash.c b/name-hash.c\nindex cac313c..39309ef 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -342,7 +342,9 @@ static int handle_range_dir(\n \t * Scan forward in the index array for index entries having the same\n \t * path prefix (that are also in this directory).\n \t */\n-\tif (strncmp(istate->cache[k_start + 1]->name, prefix->buf, prefix->len) > 0)\n+\tif (k_start + 1 >= k_end)\n+\t\tk = k_end;\n+\telse if (strncmp(istate->cache[k_start + 1]->name, prefix->buf, prefix->len) > 0)\n \t\tk = k_start + 1;\n \telse if (strncmp(istate->cache[k_end - 1]->name, prefix->buf, prefix->len) == 0)\n \t\tk = k_end;\ndiff --git a/t/t3008-ls-files-lazy-init-name-hash.sh b/t/t3008-ls-files-lazy-init-name-hash.sh\nnew file mode 100644\nindex 0000000..e46a11b\n--- /dev/null\n+++ b/t/t3008-ls-files-lazy-init-name-hash.sh\n@@ -0,0 +1,19 @@\n+#!/bin/sh\n+\n+test_description='Test the lazy init name hash with various folder structures'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'no buffer overflow in lazy_init_name_hash' '\n+\t(\n+\t test_seq 2000 | sed \"s/^/a_/\"\n+\t echo b/b/b\n+\t test_seq 2000 | sed \"s/^/c_/\"\n+\t test_seq 50 | sed \"s/^/d_/\" | tr \"\\n\" \"/\"; echo d\n+\t) |\n+\tsed \"s/^/100644 $EMPTY_BLOB\\t/\" |\n+\tgit update-index --index-info &&\n+\ttest-lazy-init-name-hash -m\n+'\n+\n+test_done\n-- \n2.9.3\n\n"},{"id":"315948","messageId":"xmqqo9whxmrq.fsf@gitster.mtv.corp.google.com","threadId":"45570","inReplyTo":"20170331173214.47514-2-git@jeffhostetler.com","subject":"Re: [PATCH] name-hash: fix buffer overrun","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-31T19:35:21Z","receivedAt":"2017-03-31T19:35:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"git@jeffhostetler.com writes:\n\n> From: Kevin Willford \n>\n> Add check for the end of the entries for the thread partition.\n> Add test for lazy init name hash with specific directory structure\n>\n> The lazy init hash name was causing a buffer overflow when the last\n> entry in the index was multiple folder deep with parent folders that\n> did not have any files in them.\n>\n> This adds a test for the boundary condition of the thread partitions\n> with the folder structure that was triggering the buffer overflow.\n>\n> The fix was to check if it is the last entry for the thread partition\n> in the handle_range_dir and not try to use the next entry in the cache.\n>\n> Signed-off-by: Kevin Willford \n> Signed-off-by: Johannes Schindelin \n> Signed-off-by: Jeff Hostetler \n>\n> ---\n\nWill queue with ...\n\n> name-hash.c | 4 +++-\n> t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++++++++++++++++++\n> 2 files changed, 22 insertions(+), 1 deletion(-)\n> create mode 100644 t/t3008-ls-files-lazy-init-name-hash.sh\n\n... this thing fixed by \"chmod +x\" (otherwise the tests won't start).\n\nThanks.\n"},{"id":"315955","messageId":"xmqqy3vlw3f9.fsf@gitster.mtv.corp.google.com","threadId":"45570","inReplyTo":"xmqqo9whxmrq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] name-hash: fix buffer overrun","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-31T21:18:34Z","receivedAt":"2017-03-31T21:18:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Will queue with ...\n>\n>> name-hash.c | 4 +++-\n>> t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++++++++++++++++++\n>> 2 files changed, 22 insertions(+), 1 deletion(-)\n>> create mode 100644 t/t3008-ls-files-lazy-init-name-hash.sh\n>\n> ... this thing fixed by \"chmod +x\" (otherwise the tests won't start).\n>\n> Thanks.\n\nAlso, https://travis-ci.org/git/git/jobs/217303927 seems to say that\nMacOS is not happy with this change.\n"},{"id":"315962","messageId":"CAPc5daWU5XDVNiYk_pTFk_qziuDr6W2XDHXXH-0oR49_KiCUYA@mail.gmail.com","threadId":"45570","inReplyTo":"xmqqy3vlw3f9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] name-hash: fix buffer overrun","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-31T23:18:29Z","receivedAt":"2017-03-31T23:18:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Fri, Mar 31, 2017 at 2:18 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Will queue with ...\n>>\n>>> name-hash.c | 4 +++-\n>>> t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++++++++++++++++++\n>>> 2 files changed, 22 insertions(+), 1 deletion(-)\n>>> create mode 100644 t/t3008-ls-files-lazy-init-name-hash.sh\n>>\n>> ... this thing fixed by \"chmod +x\" (otherwise the tests won't start).\n>>\n>> Thanks.\n>\n> Also, https://travis-ci.org/git/git/jobs/217303927 seems to say that\n> MacOS is not happy with this change.\n\nAh, of course. Avoid GNUism to spell HT as \"\\t\" in a sed script.\n"},{"id":"315982","messageId":"xmqqh928wyu7.fsf@gitster.mtv.corp.google.com","threadId":"45570","inReplyTo":"CAPc5daWU5XDVNiYk_pTFk_qziuDr6W2XDHXXH-0oR49_KiCUYA@mail.gmail.com","subject":"Re: [PATCH] name-hash: fix buffer overrun","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-01T04:12:16Z","receivedAt":"2017-04-01T04:12:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Ah, of course. Avoid GNUism to spell HT as \"\\t\" in a sed script.\n\nHere is what I replaced the original patch with. Let's see how well\nit fares with Travis tonight.\n\n-- >8 --\nFrom: Kevin Willford \nDate: Fri, 31 Mar 2017 17:32:14 +0000\nSubject: [PATCH] name-hash: fix buffer overrun\n\nAdd check for the end of the entries for the thread partition.\nAdd test for lazy init name hash with specific directory structure\n\nThe lazy init hash name was causing a buffer overflow when the last\nentry in the index was multiple folder deep with parent folders that\ndid not have any files in them.\n\nThis adds a test for the boundary condition of the thread partitions\nwith the folder structure that was triggering the buffer overflow.\n\nThe fix was to check if it is the last entry for the thread partition\nin the handle_range_dir and not try to use the next entry in the cache.\n\nSigned-off-by: Kevin Willford \nSigned-off-by: Johannes Schindelin \nSigned-off-by: Jeff Hostetler \nSigned-off-by: Junio C Hamano \n---\n name-hash.c | 4 +++-\n t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n create mode 100755 t/t3008-ls-files-lazy-init-name-hash.sh\n\ndiff --git a/name-hash.c b/name-hash.c\nindex cac313c78d..39309efb7f 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -342,7 +342,9 @@ static int handle_range_dir(\n \t * Scan forward in the index array for index entries having the same\n \t * path prefix (that are also in this directory).\n \t */\n-\tif (strncmp(istate->cache[k_start + 1]->name, prefix->buf, prefix->len) > 0)\n+\tif (k_start + 1 >= k_end)\n+\t\tk = k_end;\n+\telse if (strncmp(istate->cache[k_start + 1]->name, prefix->buf, prefix->len) > 0)\n \t\tk = k_start + 1;\n \telse if (strncmp(istate->cache[k_end - 1]->name, prefix->buf, prefix->len) == 0)\n \t\tk = k_end;\ndiff --git a/t/t3008-ls-files-lazy-init-name-hash.sh b/t/t3008-ls-files-lazy-init-name-hash.sh\nnew file mode 100755\nindex 0000000000..971975bff4\n--- /dev/null\n+++ b/t/t3008-ls-files-lazy-init-name-hash.sh\n@@ -0,0 +1,19 @@\n+#!/bin/sh\n+\n+test_description='Test the lazy init name hash with various folder structures'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'no buffer overflow in lazy_init_name_hash' '\n+\t(\n+\t test_seq 2000 | sed \"s/^/a_/\"\n+\t echo b/b/b\n+\t test_seq 2000 | sed \"s/^/c_/\"\n+\t test_seq 50 | sed \"s/^/d_/\" | tr \"\\n\" \"/\"; echo d\n+\t) |\n+\tsed -e \"s/^/100644 $EMPTY_BLOB\t/\" |\n+\tgit update-index --index-info &&\n+\ttest-lazy-init-name-hash -m\n+'\n+\n+test_done\n-- \n2.12.2-752-g2215051a9e\n\n"},{"id":"315983","messageId":"xmqqd1cwwyr5.fsf_-_@gitster.mtv.corp.google.com","threadId":"45570","inReplyTo":"CAPc5daWU5XDVNiYk_pTFk_qziuDr6W2XDHXXH-0oR49_KiCUYA@mail.gmail.com","subject":"[PATCH] contrib/git-resurrect.sh: do not write \\t for HT in sed scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-01T04:14:06Z","receivedAt":"2017-04-01T04:14:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Just like we did in 0d1d6e50 (\"t/t7003: replace \\t with literal tab\nin sed expression\", 2010-08-12), avoid writing \"\\t\" for HT in sed\nscripts, which is not portable.\n\nSigned-off-by: Junio C Hamano \n---\n * This should hopefully kill the last instances of the same issue\n to make sure people do not copy-and-paste this unportable\n construct.\n\n contrib/git-resurrect.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/git-resurrect.sh b/contrib/git-resurrect.sh\nindex c364dda696..3b78ffd079 100755\n--- a/contrib/git-resurrect.sh\n+++ b/contrib/git-resurrect.sh\n@@ -24,13 +24,13 @@ n,dry-run don't recreate the branch\"\n . git-sh-setup\n \n search_reflog () {\n- sed -ne 's~^\\([^ ]*\\) .*\\tcheckout: moving from '\"$1\"' .*~\\1~p' \\\n+\tsed -ne 's~^\\([^ ]*\\) .*\tcheckout: moving from '\"$1\"' .*~\\1~p' \\\n < \"$GIT_DIR\"/logs/HEAD\n }\n \n search_reflog_merges () {\n \tgit rev-parse $(\n-\t\tsed -ne 's~^[^ ]* \\([^ ]*\\) .*\\tmerge '\"$1\"':.*~\\1^2~p' \\\n+\t\tsed -ne 's~^[^ ]* \\([^ ]*\\) .*\tmerge '\"$1\"':.*~\\1^2~p' \\\n \t\t\t< \"$GIT_DIR\"/logs/HEAD\n \t)\n }\n-- \n2.12.2-752-g2215051a9e\n\n"},{"id":"316004","messageId":"alpine.DEB.2.20.1704011411460.3742@virtualbox","threadId":"45570","inReplyTo":"xmqqh928wyu7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] name-hash: fix buffer overrun","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-01T12:12:31Z","receivedAt":"2017-04-01T12:13:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 31 Mar 2017, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Ah, of course. Avoid GNUism to spell HT as \"\\t\" in a sed script.\n\nSorry about that, I suggested this snippet to Kevin, it is my fault for\nnot remembering BSD sed's idiosynchracies.\n\nCiao,\nJohannes\n\nP.S.: enjoy your time off!\n"},{"id":"316016","messageId":"xmqqk274uh6w.fsf@gitster.mtv.corp.google.com","threadId":"45570","inReplyTo":"alpine.DEB.2.20.1704011411460.3742@virtualbox","subject":"Re: [PATCH] name-hash: fix buffer overrun","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-01T18:16:23Z","receivedAt":"2017-04-01T18:16:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Fri, 31 Mar 2017, Junio C Hamano wrote:\n>\n>> Junio C Hamano writes:\n>> \n>> > Ah, of course. Avoid GNUism to spell HT as \"\\t\" in a sed script.\n>\n> Sorry about that, I suggested this snippet to Kevin, it is my fault for\n> not remembering BSD sed's idiosynchracies.\n\nHeh, don't fret about that. We all make mistakes and that is why we\nreview on the list so that patches get exposure to more sets of\neyes. We may be interested to learn from our common mistakes, but\nfor that purpose, \"whose fault is it?\" is far less interesting than\n\"how it came about?\", i.e. what made that mistake common and if/how\nwe can help people avoid it (removing cuttable-and-pastable bad\nexamples is one way to do so).\n\nThanks.\n"},{"id":"316051","messageId":"7028b4eb-aa2b-aaf6-764d-cffab24a9968@gmail.com","threadId":"45570","inReplyTo":"xmqqd1cwwyr5.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] contrib/git-resurrect.sh: do not write \\t for HT in sed scripts","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2017-04-02T09:58:45Z","receivedAt":"2017-04-02T09:59:05Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 01.04.2017 o 06:14, Junio C Hamano pisze:\n\n> Just like we did in 0d1d6e50 (\"t/t7003: replace \\t with literal tab\n> in sed expression\", 2010-08-12), avoid writing \"\\t\" for HT in sed\n> scripts, which is not portable.\n\nI guess it is not possible to use HT variable (similar to LF that we\nhave in t/test-lib.sh, and which is defined by some scripts) set to\nliteral tab\n\n HT=\"\t\"\n\nwouldn't work there, is it?\n\nUsing this variable would make literal tab character more visible.\n-- \nJakub Narębski\n\n"},{"id":"316058","messageId":"xmqqzifysqol.fsf@gitster.mtv.corp.google.com","threadId":"45570","inReplyTo":"7028b4eb-aa2b-aaf6-764d-cffab24a9968@gmail.com","subject":"Re: [PATCH] contrib/git-resurrect.sh: do not write \\t for HT in sed scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-02T16:46:34Z","receivedAt":"2017-04-02T16:46:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski writes:\n\n> W dniu 01.04.2017 o 06:14, Junio C Hamano pisze:\n>\n>> Just like we did in 0d1d6e50 (\"t/t7003: replace \\t with literal tab\n>> in sed expression\", 2010-08-12), avoid writing \"\\t\" for HT in sed\n>> scripts, which is not portable.\n>\n> I guess it is not possible to use HT variable (similar to LF that we\n> have in t/test-lib.sh, and which is defined by some scripts) set to\n> literal tab\n>\n> HT=\"\t\"\n>\n> wouldn't work there, is it?\n>\n> Using this variable would make literal tab character more visible.\n\nPatches welcome.\n"}]}