Skip to contentSkip to conversation

name-hash: fix buffer overrun

PATCH In review [email protected] 3 participants 5 messages Apr 3, 2017 – Apr 13, 2017 Kbuild

lore
Share & tools
lore
#0 lore raw

v2 cover name-hash: fix buffer overrun

From: Jeff Hostetler <[email protected]>

Version 2 of this fix smashes the HT and chmod issues in the test code discussed on the mailing list. The test has also been updated to skip on 1 cpu systems.

================
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.

Jeff Hostetler (1):
test-online-cpus: helper to return cpu count

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

 Makefile                                |  1 +
 name-hash.c                             |  4 +++-
 t/helper/.gitignore                     |  1 +
 t/helper/test-online-cpus.c             |  8 ++++++++
 t/t3008-ls-files-lazy-init-name-hash.sh | 27 +++++++++++++++++++++++++++
 5 files changed, 40 insertions(+), 1 deletion(-)
 create mode 100644 t/helper/test-online-cpus.c
 create mode 100755 t/t3008-ls-files-lazy-init-name-hash.sh
Signature · 1 line
2.9.3
#1 lore raw

PATCH v2 1/2 test-online-cpus: helper to return cpu count

From: Jeff Hostetler <[email protected]>

Created helper executable to print the value of online_cpus() allowing multi-threaded tests to be skipped when appropriate.

---
 Makefile                    | 1 +
 t/helper/.gitignore         | 1 +
 t/helper/test-online-cpus.c | 8 ++++++++
 3 files changed, 10 insertions(+)
 create mode 100644 t/helper/test-online-cpus.c
Makefile +1 -0
index 9b36068..3bb31e9 100644
@@ -626,6 +626,7 @@ TEST_PROGRAMS_NEED_X += test-line-buffer
 TEST_PROGRAMS_NEED_X += test-match-trees
 TEST_PROGRAMS_NEED_X += test-mergesort
 TEST_PROGRAMS_NEED_X += test-mktemp
+TEST_PROGRAMS_NEED_X += test-online-cpus
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
 TEST_PROGRAMS_NEED_X += test-prio-queue
t/helper/.gitignore +1 -0
index 758ed2e..b05d67c 100644
@@ -16,6 +16,7 @@
 /test-match-trees
 /test-mergesort
 /test-mktemp
+/test-online-cpus
 /test-parse-options
 /test-path-utils
 /test-prio-queue
t/helper/test-online-cpus.c new file +8 -0
new file mode 100644
index 0000000..c881073
@@ -0,0 +1,8 @@
+#include "stdio.h"
+#include "thread-utils.h"
+
+int cmd_main(int argc, const char **argv)
+{
+	printf("%d\n", online_cpus());
+	return 0;
+}
Signature · 1 line
2.9.3
#2 lore raw

Re: [PATCH v2 1/2] test-online-cpus: helper to return cpu count

Quoted 44 lines · On 03/04/17 16:16, [email protected] wrote:

From: Jeff Hostetler <[email protected]>

Created helper executable to print the value of online_cpus() allowing multi-threaded tests to be skipped when appropriate.

---
 Makefile                    | 1 +
 t/helper/.gitignore         | 1 +
 t/helper/test-online-cpus.c | 8 ++++++++
 3 files changed, 10 insertions(+)
 create mode 100644 t/helper/test-online-cpus.c
Makefile +1 -0
index 9b36068..3bb31e9 100644
@@ -626,6 +626,7 @@ TEST_PROGRAMS_NEED_X += test-line-buffer
 TEST_PROGRAMS_NEED_X += test-match-trees
 TEST_PROGRAMS_NEED_X += test-mergesort
 TEST_PROGRAMS_NEED_X += test-mktemp
+TEST_PROGRAMS_NEED_X += test-online-cpus
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
 TEST_PROGRAMS_NEED_X += test-prio-queue
t/helper/.gitignore +1 -0
index 758ed2e..b05d67c 100644
@@ -16,6 +16,7 @@
 /test-match-trees
 /test-mergesort
 /test-mktemp
+/test-online-cpus
 /test-parse-options
 /test-path-utils
 /test-prio-queue
t/helper/test-online-cpus.c new file +0 -0
new file mode 100644
index 0000000..c881073

t/helper/test-online-cpus.c line 8

Quoted 8 lines
t/helper/test-online-cpus.c new file +8 -0
@@ -0,0 +1,8 @@
+#include "stdio.h"
+#include "thread-utils.h"
+
+int cmd_main(int argc, const char **argv)
+{
+	printf("%d\n", online_cpus());
+	return 0;
+}

In order to suppress a warning (for lack of extern declaration of cmd_main), we need to include git-compat-util.h ( or cache.h etc,.), like so:

  $ git diff
  diff --git a/t/helper/test-online-cpus.c b/t/helper/test-online-cpus.c
  index c88107360..b5277fb50 100644
  --- a/t/helper/test-online-cpus.c
  +++ b/t/helper/test-online-cpus.c
  @@ -1,3 +1,4 @@
  +#include "git-compat-util.h"
   #include "stdio.h"
   #include "thread-utils.h"

$

Otherwise, this series fixes the test for me.

Thanks!

ATB,
Ramsay Jones

#3 lore raw

PATCH v2 2/2 name-hash: fix buffer overrun

In reply to [email protected]

From: Kevin Willford <[email protected]>

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 test is skipped on single-cpu machines because the original code path is used in name-hash.c

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.

---
 name-hash.c                             |  4 +++-
 t/t3008-ls-files-lazy-init-name-hash.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100755 t/t3008-ls-files-lazy-init-name-hash.sh
name-hash.c +3 -1
index cac313c..39309ef 100644
@@ -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;
t/t3008-ls-files-lazy-init-name-hash.sh new file +27 -0
new file mode 100755
index 0000000..bdf5198
@@ -0,0 +1,27 @@
+#!/bin/sh
+
+test_description='Test the lazy init name hash with various folder structures'
+
+. ./test-lib.sh
+
+if test 1 -eq $($GIT_BUILD_DIR/t/helper/test-online-cpus)
+then
+	skip_all='skipping lazy-init tests, single cpu'
+	test_done
+fi
+
+LAZY_THREAD_COST=2000
+
+test_expect_success 'no buffer overflow in lazy_init_name_hash' '
+	(
+	    test_seq $LAZY_THREAD_COST | sed "s/^/a_/"
+	    echo b/b/b
+	    test_seq $LAZY_THREAD_COST | sed "s/^/c_/"
+	    test_seq 50 | sed "s/^/d_/" | tr "\n" "/"; echo d
+	) |
+	sed "s/^/100644 $EMPTY_BLOB	/" |
+	git update-index --index-info &&
+	test-lazy-init-name-hash -m
+'
+
+test_done
Signature · 1 line
2.9.3
#4 lore raw
Quoted 9 lines · [email protected] writes:

From: Kevin Willford <[email protected]>

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.

Quoted 7 lines

This adds a test for the boundary condition of the thread partitions with the folder structure that was triggering the buffer overflow. The test is skipped on single-cpu machines because the original code path is used in name-hash.c

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.

As I merged the older one already to 'next', I'll queue 1/2 of v2 on top of them and then the following (which is incremental between v1 and this v2 2/2) on top.

-- >8 --
From: Kevin Willford <[email protected]>
Date: Mon, 3 Apr 2017 15:16:42 +0000
Subject: [PATCH] t3008: skip lazy-init test on a single-core box

The lazy-init codepath will not be exercised uniless threaded. Skip the entire test on a single-core box. Also replace a hard-coded constant of 2000 (number of cache entries to manifacture for tests) with a variable with a human readable name.

---
 t/t3008-ls-files-lazy-init-name-hash.sh | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
t/t3008-ls-files-lazy-init-name-hash.sh +11 -3
index 971975bff4..bdf5198b7e 100755
@@ -4,14 +4,22 @@ test_description='Test the lazy init name hash with various folder structures'
 
 . ./test-lib.sh
 
+if test 1 -eq $($GIT_BUILD_DIR/t/helper/test-online-cpus)
+then
+	skip_all='skipping lazy-init tests, single cpu'
+	test_done
+fi
+
+LAZY_THREAD_COST=2000
+
 test_expect_success 'no buffer overflow in lazy_init_name_hash' '
 	(
-	    test_seq 2000 | sed "s/^/a_/"
+	    test_seq $LAZY_THREAD_COST | sed "s/^/a_/"
 	    echo b/b/b
-	    test_seq 2000 | sed "s/^/c_/"
+	    test_seq $LAZY_THREAD_COST | sed "s/^/c_/"
 	    test_seq 50 | sed "s/^/d_/" | tr "\n" "/"; echo d
 	) |
-	sed -e "s/^/100644 $EMPTY_BLOB	/" |
+	sed "s/^/100644 $EMPTY_BLOB	/" |
 	git update-index --index-info &&
 	test-lazy-init-name-hash -m
 '
Signature · 1 line
2.12.2-776-gded3dc243c