Re: [PATCH 1/4] cat-file: handle NULL object_context.path

[Jonathan Nieder]

Jeff King wrote:

> Commit dc944b65f1 (get_sha1_with_context: dynamically
> allocate oc->path, 2017-05-19) changed the rules that
> callers must follow for seeing if we parsed a path in the
> object name. The rules switched from "check if the oc.path
> buffer is empty" to "check if the oc.path pointer is NULL".
> But that commit forgot to update some sites in
> cat_one_file(), meaning we might dereference a NULL pointer.
>
> You can see this by making a path-aware request like
> --textconv without specifying --path, and giving an object
> name that doesn't have a path in it. Like:
>
>   git cat-file --textconv HEAD
>
> which will reliably segfault.
>
> Signed-off-by: Jeff King 
> ---
>  builtin/cat-file.c  | 4 ++--
>  t/t8010-cat-file-filters.sh | 5 +
>  2 files changed, 7 insertions(+), 2 deletions(-)

Yikes.  Commit dc944b65f1 even touched this function, so we reviewers
have no excuse for not having found it.

Technically this changes the behavior of cat-file --path='', but I
don't think that matters.

Do other GET_SHA1_RECORD_PATH callers need similar treatment?

* builtin/grep.c appears to do the right thing (it stores NULL in
  list, so it passes NULL to grep_object, which calls grep_oid, which
  calls grep_source_init, which stores NULL for the grep machinery
  that is able to cope with a NULL).

* builtin/log.c is correctly updated as part of the patch.

Those are the only other callers.  So we're safe. *phew*

Reviewed-by: Jonathan Nieder 

[PATCH 1/4] cat-file: handle NULL object_context.path

[Jeff King]

Commit dc944b65f1 (get_sha1_with_context: dynamically
allocate oc->path, 2017-05-19) changed the rules that
callers must follow for seeing if we parsed a path in the
object name. The rules switched from "check if the oc.path
buffer is empty" to "check if the oc.path pointer is NULL".
But that commit forgot to update some sites in
cat_one_file(), meaning we might dereference a NULL pointer.

You can see this by making a path-aware request like
--textconv without specifying --path, and giving an object
name that doesn't have a path in it. Like:

  git cat-file --textconv HEAD

which will reliably segfault.

Signed-off-by: Jeff King 
---
 builtin/cat-file.c  | 4 ++--
 t/t8010-cat-file-filters.sh | 5 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 4ccbfaac31..1ea25331d3 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -97,7 +97,7 @@ static int cat_one_file(int opt, const char *exp_type, const 
char *obj_name,
return !has_object_file();
 
case 'w':
-   if (!path[0])
+   if (!path)
die("git cat-file --filters %s:  must be "
"", obj_name);
 
@@ -107,7 +107,7 @@ static int cat_one_file(int opt, const char *exp_type, 
const char *obj_name,
break;
 
case 'c':
-   if (!path[0])
+   if (!path)
die("git cat-file --textconv %s:  must be 
",
obj_name);
 
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index d8242e467e..0f86c19174 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -51,6 +51,11 @@ test_expect_success '--path= complains without 
--textconv/--filters' '
grep "path.*needs.*filters" err
 '
 
+test_expect_success '--textconv/--filters complain without path' '
+   test_must_fail git cat-file --textconv HEAD &&
+   test_must_fail git cat-file --filters HEAD
+'
+
 test_expect_success 'cat-file --textconv --batch works' '
sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
-- 
2.14.1.1051.g8c9143ec35