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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 22, 2017, 00:58 UTC
Message-ID: <20170922005857.GK27425@aiede.mtv.corp.google.com>
In-Reply-To: <20170921062140.cyfjv7rfuppqpblh@sigill.intra.peff.net>

Jeff King wrote:

Show 21 quoted lines

> 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 <peff@peff.net>
> ---
>  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 <jrnieder@gmail.com>