Re: [PATCH] name-hash: fix buffer overrun

[Junio C Hamano]

Johannes Schindelin  writes:

> On Fri, 31 Mar 2017, Junio C Hamano wrote:
>
>> Junio C Hamano  writes:
>> 
>> > Ah, of course. Avoid GNUism to spell HT as "\t" in a sed script.
>
> Sorry about that, I suggested this snippet to Kevin, it is my fault for
> not remembering BSD sed's idiosynchracies.

Heh, don't fret about that.  We all make mistakes and that is why we
review on the list so that patches get exposure to more sets of
eyes.  We may be interested to learn from our common mistakes, but
for that purpose, "whose fault is it?" is far less interesting than
"how it came about?", i.e. what made that mistake common and if/how
we can help people avoid it (removing cuttable-and-pastable bad
examples is one way to do so).

Thanks.

Re: [PATCH] name-hash: fix buffer overrun

[Johannes Schindelin]

Hi Junio,

On Fri, 31 Mar 2017, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Ah, of course. Avoid GNUism to spell HT as "\t" in a sed script.

Sorry about that, I suggested this snippet to Kevin, it is my fault for
not remembering BSD sed's idiosynchracies.

Ciao,
Johannes

P.S.: enjoy your time off!

Re: [PATCH] name-hash: fix buffer overrun

[Junio C Hamano]

Junio C Hamano <gits...@pobox.com> writes:

> Ah, of course. Avoid GNUism to spell HT as "\t" in a sed script.

Here is what I replaced the original patch with.  Let's see how well
it fares with Travis tonight.

-- >8 --
From: Kevin Willford <kewi...@microsoft.com>
Date: Fri, 31 Mar 2017 17:32:14 +0000
Subject: [PATCH] name-hash: fix buffer overrun

Add check for the end of the entries for the thread partition.
Add test for lazy init name hash with specific directory structure

The lazy init hash name was causing a buffer overflow when the last
entry in the index was multiple folder deep with parent folders that
did not have any files in them.

This adds a test for the boundary condition of the thread partitions
with the folder structure that was triggering the buffer overflow.

The fix was to check if it is the last entry for the thread partition
in the handle_range_dir and not try to use the next entry in the cache.

Signed-off-by: Kevin Willford <kewi...@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Jeff Hostetler <jeffh...@microsoft.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 name-hash.c |  4 +++-
 t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++
 2 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100755 t/t3008-ls-files-lazy-init-name-hash.sh

diff --git a/name-hash.c b/name-hash.c
index cac313c78d..39309efb7f 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -342,7 +342,9 @@ static int handle_range_dir(
 * Scan forward in the index array for index entries having the same
 * path prefix (that are also in this directory).
 */
-   if (strncmp(istate->cache[k_start + 1]->name, prefix->buf, prefix->len) 
> 0)
+   if (k_start + 1 >= k_end)
+   k = k_end;
+   else if (strncmp(istate->cache[k_start + 1]->name, prefix->buf, 
prefix->len) > 0)
k = k_start + 1;
else if (strncmp(istate->cache[k_end - 1]->name, prefix->buf, 
prefix->len) == 0)
k = k_end;
diff --git a/t/t3008-ls-files-lazy-init-name-hash.sh 
b/t/t3008-ls-files-lazy-init-name-hash.sh
new file mode 100755
index 00..971975bff4
--- /dev/null
+++ b/t/t3008-ls-files-lazy-init-name-hash.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description='Test the lazy init name hash with various folder structures'
+
+. ./test-lib.sh
+
+test_expect_success 'no buffer overflow in lazy_init_name_hash' '
+   (
+   test_seq 2000 | sed "s/^/a_/"
+   echo b/b/b
+   test_seq 2000 | sed "s/^/c_/"
+   test_seq 50 | sed "s/^/d_/" | tr "\n" "/"; echo d
+   ) |
+   sed -e "s/^/100644 $EMPTY_BLOB  /" |
+   git update-index --index-info &&
+   test-lazy-init-name-hash -m
+'
+
+test_done
-- 
2.12.2-752-g2215051a9e

Re: [PATCH] name-hash: fix buffer overrun

[Junio C Hamano]

On Fri, Mar 31, 2017 at 2:18 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Will queue with ...
>>
>>>  name-hash.c |  4 +++-
>>>  t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++
>>>  2 files changed, 22 insertions(+), 1 deletion(-)
>>>  create mode 100644 t/t3008-ls-files-lazy-init-name-hash.sh
>>
>> ... this thing fixed by "chmod +x" (otherwise the tests won't start).
>>
>> Thanks.
>
> Also, https://travis-ci.org/git/git/jobs/217303927 seems to say that
> MacOS is not happy with this change.

Ah, of course. Avoid GNUism to spell HT as "\t" in a sed script.

Re: [PATCH] name-hash: fix buffer overrun

[Junio C Hamano]

Junio C Hamano  writes:

> Will queue with ...
>
>>  name-hash.c |  4 +++-
>>  t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++
>>  2 files changed, 22 insertions(+), 1 deletion(-)
>>  create mode 100644 t/t3008-ls-files-lazy-init-name-hash.sh
>
> ... this thing fixed by "chmod +x" (otherwise the tests won't start).
>
> Thanks.

Also, https://travis-ci.org/git/git/jobs/217303927 seems to say that
MacOS is not happy with this change.

Re: [PATCH] name-hash: fix buffer overrun

[Junio C Hamano]

g...@jeffhostetler.com writes:

> From: Kevin Willford 
>
> Add check for the end of the entries for the thread partition.
> Add test for lazy init name hash with specific directory structure
>
> The lazy init hash name was causing a buffer overflow when the last
> entry in the index was multiple folder deep with parent folders that
> did not have any files in them.
>
> This adds a test for the boundary condition of the thread partitions
> with the folder structure that was triggering the buffer overflow.
>
> The fix was to check if it is the last entry for the thread partition
> in the handle_range_dir and not try to use the next entry in the cache.
>
> Signed-off-by: Kevin Willford 
> Signed-off-by: Johannes Schindelin 
> Signed-off-by: Jeff Hostetler 
>
> ---

Will queue with ...

>  name-hash.c |  4 +++-
>  t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>  create mode 100644 t/t3008-ls-files-lazy-init-name-hash.sh

... this thing fixed by "chmod +x" (otherwise the tests won't start).

Thanks.

[PATCH] name-hash: fix buffer overrun

[git]

From: Kevin Willford 

Add check for the end of the entries for the thread partition.
Add test for lazy init name hash with specific directory structure

The lazy init hash name was causing a buffer overflow when the last
entry in the index was multiple folder deep with parent folders that
did not have any files in them.

This adds a test for the boundary condition of the thread partitions
with the folder structure that was triggering the buffer overflow.

The fix was to check if it is the last entry for the thread partition
in the handle_range_dir and not try to use the next entry in the cache.

Signed-off-by: Kevin Willford 
Signed-off-by: Johannes Schindelin 
Signed-off-by: Jeff Hostetler 

---
 name-hash.c |  4 +++-
 t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++
 2 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 t/t3008-ls-files-lazy-init-name-hash.sh

diff --git a/name-hash.c b/name-hash.c
index cac313c..39309ef 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -342,7 +342,9 @@ static int handle_range_dir(
 * Scan forward in the index array for index entries having the same
 * path prefix (that are also in this directory).
 */
-   if (strncmp(istate->cache[k_start + 1]->name, prefix->buf, prefix->len) 
> 0)
+   if (k_start + 1 >= k_end)
+   k = k_end;
+   else if (strncmp(istate->cache[k_start + 1]->name, prefix->buf, 
prefix->len) > 0)
k = k_start + 1;
else if (strncmp(istate->cache[k_end - 1]->name, prefix->buf, 
prefix->len) == 0)
k = k_end;
diff --git a/t/t3008-ls-files-lazy-init-name-hash.sh 
b/t/t3008-ls-files-lazy-init-name-hash.sh
new file mode 100644
index 000..e46a11b
--- /dev/null
+++ b/t/t3008-ls-files-lazy-init-name-hash.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description='Test the lazy init name hash with various folder structures'
+
+. ./test-lib.sh
+
+test_expect_success 'no buffer overflow in lazy_init_name_hash' '
+   (
+   test_seq 2000 | sed "s/^/a_/"
+   echo b/b/b
+   test_seq 2000 | sed "s/^/c_/"
+   test_seq 50 | sed "s/^/d_/" | tr "\n" "/"; echo d
+   ) |
+   sed "s/^/100644 $EMPTY_BLOB\t/" |
+   git update-index --index-info &&
+   test-lazy-init-name-hash -m
+'
+
+test_done
-- 
2.9.3

[PATCH] name-hash: fix buffer overrun

[git]

From: Jeff Hostetler 

Fix buffer overrun in handle_range_dir() when the final entry
in the index was the only file in the last directory, such as
"a/b/foo.txt". The look ahead (k_start + 1) was invalid since
(k_start + 1) == k_end.

This bug was introduced by Jeff in "jh/memihash-opt" which was
recently merged into master.

Kevin Willford (1):
  name-hash: fix buffer overrun

 name-hash.c |  4 +++-
 t/t3008-ls-files-lazy-init-name-hash.sh | 19 +++
 2 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 t/t3008-ls-files-lazy-init-name-hash.sh

-- 
2.9.3