patch, 11 partsStop relying on SHA1 fallback for `the_hash_algo`

Hi,

when starting up, Git will initialize `the_repository` by calling `initialize_the_repository()`. Part of this is also that we set the hash algo of `the_repository` to SHA1, which implicitly sets `the_hash_algo` because it is a macro expanding to `the_repository->hash_algo`.

Usually, we eventually set up the correct hash algorithm here once we have properly set up `the_repository` via `setup_git_directory()`. But in some commands we actually don't require a Git repository, and thus we will leave the SHA1 hash algorithm in place.

This has led to some subtle bugs when the context really asks for a SHA256 repository, which this patch series corrects for most of the part. Some commands need further work, like for example git-diff(1), where the user might want to have the ability to pick a hash function when run outside of a repository.

Ultimately, the last patch then drops the setup of the fallback hash algorithm completely. This will cause `the_hash_algo` to be a `NULL` pointer unless explicitly configured, and thus we now start to crash when it gets accessed without doing so beforehand. This is a rather risky thing to do, but it does catch bugs where we might otherwise accidentally do the wrong thing. And even though I think it is the right thing to do even conceptually, I'd be okay to drop it if folks think that the risk is not worth it.

Patrick

Patrick Steinhardt (11):
  path: harden validation of HEAD with non-standard hashes
  parse-options-cb: only abbreviate hashes when hash algo is known
  attr: don't recompute default attribute source
  attr: fix BUG() when parsing attrs outside of repo
  remote-curl: fix parsing of detached SHA256 heads
  builtin/rev-parse: allow shortening to more than 40 hex characters
  builtin/blame: don't access potentially unitialized `the_hash_algo`
  builtin/bundle: abort "verify" early when there is no repository
  builtin/diff: explicitly set hash algo when there is no repo
  builtin/shortlog: don't set up revisions without repo
  repository: stop setting SHA1 as the default object hash

 attr.c                     | 31 ++++++++++++++++++++++---------
 builtin/blame.c            |  5 ++---
 builtin/bundle.c           |  5 +++++
 builtin/diff.c             |  9 +++++++++
 builtin/rev-parse.c        |  5 ++---
 builtin/shortlog.c         |  2 +-
 parse-options-cb.c         |  3 ++-
 path.c                     |  2 +-
 remote-curl.c              | 19 ++++++++++++++++++-
 repository.c               |  2 --
 t/t0003-attributes.sh      | 15 +++++++++++++++
 t/t0040-parse-options.sh   | 17 +++++++++++++++++
 t/t1500-rev-parse.sh       |  6 ++++++
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++
 14 files changed, 115 insertions(+), 21 deletions(-)

The `validate_headref()` function takes a path to a supposed "HEAD" file and checks whether its format is something that we understand. It is used as part of our repository discovery to check whether a specific directory is a Git directory or not.

Part of the validation is a check for a detached HEAD that contains a plain object ID. To do this validation we use `get_oid_hex()`, which relies on `the_hash_algo`. At this point in time the hash algo cannot yet be initialized though because we didn't yet read the Git config. Consequently, it will always be the SHA1 hash algorithm.

In practice this works alright because `get_oid_hex()` only ends up checking whether the prefix of the buffer is a valid object ID. And because SHA1 is shorter than SHA256, the function will successfully parse SHA256 object IDs, as well.

It is somewhat fragile though and not really the intent to only check for SHA1. With this in mind, harden the code to use `get_oid_hex_any()` to check whether the "HEAD" file parses as any known hash.

One might be hard pressed to tighten the check even further and fully validate the file contents, not only the prefix. In practice though that wouldn't make a lot of sense as it could be that the repository uses a hash function that produces longer hashes than SHA256, but which the current version of Git doesn't understand yet. We'd still want to detect the repository as proper Git repository in that case, and we will fail eventually with a proper error message that the hash isn't understood when trying to set up the repostiory format.

It follows that we could just leave the current code intact, as in practice the code change doesn't have any user visible impact. But it also prepares us for `the_hash_algo` being unset when there is no repositroy.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 2024-04-19 at 09:51:10, Patrick Steinhardt wrote:

The patch looks fine and is well explained, but you have a typo ("repositroy").

On Fri, Apr 19, 2024 at 07:03:33PM +0000, brian m. carlson wrote:

Thanks, fixed! I'll refrain from sending out another version just to address this typo though.

Patrick

Patrick Steinhardt <ps@pks.im> writes:

All makes sense, and given the above, I strongly suspect that we would want to make the validate_headref() function a file-scope static in setup.c to make sure it is only called/callable from the repository discovery codepath. Especially that if somebody calls this function again after we find out that the repository uses SHA-256, we would want to let the caller know that the detached HEAD records SHA-1 and we are in an inconsistent state.

Or perhaps we use get_oid_hex_any() != GIT_HASH_UNKNOWN when the_hash_algo has not been determined, and use !get_oid_hex() after we have determined which algorithm we are using? It may be what you did in a later step in the series, so let me read on.

Thanks.

On Mon, Apr 22, 2024 at 09:15:41AM -0700, Junio C Hamano wrote:

Fair enough, we can definitely do so. It only has a single callsite anyway.

I didn't, and given that it's only used from `is_git_directory()` it would probably not make much sense, either. I'll rather add another patch on top that moves the function around to clarify that it is only expected to be called from "setup.c".

Patrick

Patrick Steinhardt <ps@pks.im> writes:

I was wondering if I was missing a future plans to call it from other code paths that are triggered after the set-up was done. If that is not the case, we should do so. It matters more for the future callers than the current ones. They _all_ have to be aware of the deliberate looseness of the check here.

Thanks.  

The `OPT__ABBREV()` option can be used to add an option that abbreviates object IDs. When given an length longer than `the_hash_algo->hexsz`, then it will instead set the length to that maximum length.

It may not always be guaranteed that we have `the_hash_algo` initialized properly as the hash algortihm can only be set up after we have set up `the_repository`. In that case, the hash would always be truncated to the hex length of SHA1, which may not be what the user desires.

In practice it's not a problem as all commands that use `OPT__ABBREV()` also have `RUN_SETUP` set and thus cannot work without a repository. Consequently, both `the_repository` and `the_hash_algo` would be properly set up.

Regardless of that, harden the code to not truncate the length when we didn't set up a repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 parse-options-cb.c       |  3 ++-
 t/t0040-parse-options.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

On 24/04/19 11:51AM, Patrick Steinhardt wrote:

s/an length/a length/

s/algortihm/algorithm/

...

A couple of typos I noticed.

-Justin

The `default_attr_source()` function lazily computes the attr source supposedly once, only. This is done via a static variable `attr_source` that contains the resolved object ID of the attr source's tree. If the variable is the null object ID then we try to look up the attr source, otherwise we skip over it.

This has approach is flawed though: the variable will never be set to anything else but the null object ID in case there is no attr source. Consequently, we re-compute the information on every call. And in the worst case, when we silently ignore bad trees, this will cause us to try and look up the treeish every single time.

Improve this by introducing a separate variable `has_attr_source` to track whether we already computed the attr source and, if so, whether we have an attr source or not.

This also allows us to convert the `ignore_bad_attr_tree` to not be static anymore as the code will only be executed once anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 attr.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

On 24/04/19 11:51AM, Patrick Steinhardt wrote:

Remove "has"

-Justin

...

If either the `--attr-source` option or the `GIT_ATTR_SOURCE` envvar are set, then `compute_default_attr_source()` will try to look up the value as a treeish. It is possible to hit that function while outside of a Git repository though, for example when using `git grep --no-index`. In that case, Git will hit a bug because we try to look up the main ref store outside of a repository.

Handle the case gracefully and detect when we try to look up an attr source without a repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 attr.c                |  6 ++++++
 t/t0003-attributes.sh | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

The dumb HTTP transport tries to read the remote HEAD reference by downloading the "HEAD" file and then parsing it via `http_fetch_ref()`. This function will either parse the file as an object ID in case it is exactly `the_hash_algo->hexsz` long, or otherwise it will check whether the reference starts with "ref :" and parse it as a symbolic ref.

This is broken when parsing detached HEADs of a remote SHA256 repository because we never update `the_hash_algo` to the discovered remote object hash. Consequently, `the_hash_algo` will always be the fallback SHA1 hash algorithm, which will cause us to fail parsing HEAD altogteher when it contains a SHA256 object ID.

Fix this issue by setting up `the_hash_algo` via `repo_set_hash_algo()`. While at it, let's make the expected SHA1 fallback explicit in our code, which also addresses an upcoming issue where we are going to remove the SHA1 fallback for `the_hash_algo`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote-curl.c              | 19 ++++++++++++++++++-
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

The `--short=` option for git-rev-parse(1) allows the user to specify to how many characters object IDs should be shortened to. The option is broken though for SHA256 repositories because we set the maximum allowed hash size to `the_hash_algo->hexsz` before we have even set up the repo. Consequently, `the_hash_algo` will always be SHA1 and thus we truncate every hash after at most 40 characters.

Fix this by accessing `the_hash_algo` only after we have set up the repo.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/rev-parse.c  | 5 ++---
 t/t1500-rev-parse.sh | 6 ++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

We access `the_hash_algo` in git-blame(1) before we have executed `parse_options_start()`, which may not be properly set up in case we have no repository. This is fine for most of the part because all the call paths that lead to it (git-blame(1), git-annotate(1) as well as git-pick-axe(1)) specify `RUN_SETUP` and thus require a repository.

There is one exception though, namely when passing `-h` to print the help. Here we will access `the_hash_algo` even if there is no repo. This works fine right now because `the_hash_algo` gets sets up to point to the SHA1 algorithm via `initialize_repository()`. But we're about to stop doing this, and thus the code would lead to a `NULL` pointer exception.

Prepare the code for this and only access `the_hash_algo` after we are sure that there is a proper repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/blame.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Verifying a bundle requires us to have a repository. This is encoded in `verify_bundle()`, which will return an error if there is no repository. We call `open_bundle()` before we call `verify_bundle()` though, which already performs some verifications even though we may ultimately abort due to a missing repository.

This is problematic because `open_bundle()` already reads the bundle header and verifies that it contains a properly formatted hash. When there is no repository we have no clue what hash function to expect though, so we always end up assuming SHA1 here, which may or may not be correct. Furthermore, we are about to stop initializing `the_hash_algo` when there is no repository, which will lead to segfaults.

Check early on whether we have a repository to fix this issue.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/bundle.c | 5 +++++
 1 file changed, 5 insertions(+)

The git-diff(1) command can be used outside repositories to diff two files with each other. But even if there is no repository we will end up hashing the files that we are diffing so that we can print the "index" line:

```

Patrick Steinhardt <ps@pks.im> writes:

This will break "git am"; it is customary to indent such sample patch block in a log message.

It is possible to run git-shortlog(1) outside of a repository by passing it output from git-log(1) via standard input. Obviously, as there is no repository in that context, it is thus unsupported to pass any revisions as arguments.

Reghardless of that we still end up calling `setup_revisions()`. While that works alright, it is somewhat strange. Furthermore, this is about to cause problems when we unset the default object hash.

Refactor the code to only call `setup_revisions()` when we have a repository. This is safe to do as we already verify that there are no arguments when running outside of a repository anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/shortlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 24/04/19 11:51AM, Patrick Steinhardt wrote:

s/Reghardless/Regardless/

...

During the startup of Git, we call `initialize_the_repository()` to set up `the_repository` as well as `the_index`. Part of this setup is also to set the default object hash of the repository to SHA1. This has the effect that `the_hash_algo` is getting initialized to SHA1, as well. This default hash algorithm eventually gets overridden by most Git commands via `setup_git_directory()`, which also detects the actual hash algorithm used by the repository.

There are some commands though that don't access a repository at all, or at a later point only, and thus retain the default hash function for some amount of time. As some of the the preceding commits demonstrate, this can lead to subtle issues when we access `the_hash_algo` when no repository has been set up.

Address this issue by dropping the set up of the default hash algorithm completely. The effect of this is that `the_hash_algo` will map to a `NULL` pointer and thus cause Git to crash when something tries to access the hash algorithm without it being properly initialized. It thus forces all Git commands to explicitly set up the hash algorithm in case there is no repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 repository.c | 2 --
 1 file changed, 2 deletions(-)

On 2024-04-19 at 09:51:03, Patrick Steinhardt wrote:

I've taken a look, and other than the minor typo I noted, this seems fine. I'm in favour of getting rid of the SHA-1 default, even though my gut tells me we might find a bug or two along the way where things aren't initialized properly. I still think that'll be okay and it's worth doing, since it'll help us prepare for the case in the future where we want to switch the default and also for libification, where we won't want to make those kinds of assumptions.

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Yup, thanks for reviewing (and thanks Patrick for writing the series).

On Fri, Apr 19, 2024 at 07:12:59PM +0000, brian m. carlson wrote:

Yeah, I would expect there to be a few more bugs that I just didn't spot. But I think if we do this early in the next release cycle it would give us plenty of time to detect any such issues. And in the worst case we would still be able to revert the last patch of this series. So all in all it hopefully shouldn't be all that bad and prepares us for the future.

Thanks for your review!

Patrick

Hi,

this is the second version of my patch series that causes us to stop relying on the SHA1 default hash.

Changes compared to v1:

    - Various typo fixes in commit messages.

    - Added another patch that moves `validate_headref()` into "setup.c"
      to clarify that it is only used during repository discovery.

    - Indented a diff in a commit message so that git-am(1) is happy.

Thanks!

Patrick

Patrick Steinhardt (12):
  path: harden validation of HEAD with non-standard hashes
  path: move `validate_headref()` to its only user
  parse-options-cb: only abbreviate hashes when hash algo is known
  attr: don't recompute default attribute source
  attr: fix BUG() when parsing attrs outside of repo
  remote-curl: fix parsing of detached SHA256 heads
  builtin/rev-parse: allow shortening to more than 40 hex characters
  builtin/blame: don't access potentially unitialized `the_hash_algo`
  builtin/bundle: abort "verify" early when there is no repository
  builtin/diff: explicitly set hash algo when there is no repo
  builtin/shortlog: don't set up revisions without repo
  repository: stop setting SHA1 as the default object hash

 attr.c                     | 31 +++++++++++++++-------
 builtin/blame.c            |  5 ++--
 builtin/bundle.c           |  5 ++++
 builtin/diff.c             |  9 +++++++
 builtin/rev-parse.c        |  5 ++--
 builtin/shortlog.c         |  2 +-
 parse-options-cb.c         |  3 ++-
 path.c                     | 53 --------------------------------------
 path.h                     |  1 -
 remote-curl.c              | 19 +++++++++++++-
 repository.c               |  2 --
 setup.c                    | 53 ++++++++++++++++++++++++++++++++++++++
 t/t0003-attributes.sh      | 15 +++++++++++
 t/t0040-parse-options.sh   | 17 ++++++++++++
 t/t1500-rev-parse.sh       |  6 +++++
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++
 16 files changed, 167 insertions(+), 74 deletions(-)

Range-diff against v1:
 1:  aa4d6f508b !  1:  a986b464d3 path: harden validation of HEAD with non-standard hashes
    @@ Commit message
         current version of Git doesn't understand yet. We'd still want to detect
         the repository as proper Git repository in that case, and we will fail
         eventually with a proper error message that the hash isn't understood
    -    when trying to set up the repostiory format.
    +    when trying to set up the repository format.
     
         It follows that we could just leave the current code intact, as in
         practice the code change doesn't have any user visible impact. But it
         also prepares us for `the_hash_algo` being unset when there is no
    -    repositroy.
    +    repository.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
 -:  ---------- >  2:  a347c7e6ca path: move `validate_headref()` to its only user
 2:  5daaaed2b9 !  3:  c0a15b2fa6 parse-options-cb: only abbreviate hashes when hash algo is known
    @@ Commit message
         parse-options-cb: only abbreviate hashes when hash algo is known
     
         The `OPT__ABBREV()` option can be used to add an option that abbreviates
    -    object IDs. When given an length longer than `the_hash_algo->hexsz`,
    -    then it will instead set the length to that maximum length.
    +    object IDs. When given a length longer than `the_hash_algo->hexsz`, then
    +    it will instead set the length to that maximum length.
     
         It may not always be guaranteed that we have `the_hash_algo` initialized
    -    properly as the hash algortihm can only be set up after we have set up
    +    properly as the hash algorithm can only be set up after we have set up
         `the_repository`. In that case, the hash would always be truncated to
         the hex length of SHA1, which may not be what the user desires.
     
 3:  ae91a27ffc !  4:  1b5f904eed attr: don't recompute default attribute source
    @@ Commit message
         variable is the null object ID then we try to look up the attr source,
         otherwise we skip over it.
     
    -    This has approach is flawed though: the variable will never be set to
    +    This approach is flawed though: the variable will never be set to
         anything else but the null object ID in case there is no attr source.
         Consequently, we re-compute the information on every call. And in the
         worst case, when we silently ignore bad trees, this will cause us to try
 4:  53c8e1cd7c =  5:  26909daca4 attr: fix BUG() when parsing attrs outside of repo
 5:  32a429fb60 =  6:  0b99184f50 remote-curl: fix parsing of detached SHA256 heads
 6:  9cb7baa50c =  7:  ccfda3c2d2 builtin/rev-parse: allow shortening to more than 40 hex characters
 7:  e189a4ad15 =  8:  1813e7eb5c builtin/blame: don't access potentially unitialized `the_hash_algo`
 8:  bc4bda3508 =  9:  31182a1fc6 builtin/bundle: abort "verify" early when there is no repository
 9:  39e56dab62 ! 10:  78e19d0a1b builtin/diff: explicitly set hash algo when there is no repo
    @@ Commit message
         hashing the files that we are diffing so that we can print the "index"
         line:
     
    -    ```
    -    diff --git a/a b/b
    -    index 7898192..6178079 100644
    -    --- a/a
    -    +++ b/b
    -    @@ -1 +1 @@
    -    -a
    -    +b
    -    ```
    +        ```
    +        diff --git a/a b/b
    +        index 7898192..6178079 100644
    +        --- a/a
    +        +++ b/b
    +        @@ -1 +1 @@
    +        -a
    +        +b
    +        ```
     
         We implicitly use SHA1 to calculate the hash here, which is because
         `the_repository` gets initialized with SHA1 during the startup routine.
10:  508e28ed1e ! 11:  51bcddbc31 builtin/shortlog: don't set up revisions without repo
    @@ Commit message
         repository in that context, it is thus unsupported to pass any revisions
         as arguments.
     
    -    Reghardless of that we still end up calling `setup_revisions()`. While
    +    Regardless of that we still end up calling `setup_revisions()`. While
         that works alright, it is somewhat strange. Furthermore, this is about
         to cause problems when we unset the default object hash.
     
11:  f86a6ff3ba = 12:  e8126371e1 repository: stop setting SHA1 as the default object hash

base-commit: 21306a098c3f174ad4c2a5cddb9069ee27a548b0

The `validate_headref()` function takes a path to a supposed "HEAD" file and checks whether its format is something that we understand. It is used as part of our repository discovery to check whether a specific directory is a Git directory or not.

Part of the validation is a check for a detached HEAD that contains a plain object ID. To do this validation we use `get_oid_hex()`, which relies on `the_hash_algo`. At this point in time the hash algo cannot yet be initialized though because we didn't yet read the Git config. Consequently, it will always be the SHA1 hash algorithm.

In practice this works alright because `get_oid_hex()` only ends up checking whether the prefix of the buffer is a valid object ID. And because SHA1 is shorter than SHA256, the function will successfully parse SHA256 object IDs, as well.

It is somewhat fragile though and not really the intent to only check for SHA1. With this in mind, harden the code to use `get_oid_hex_any()` to check whether the "HEAD" file parses as any known hash.

One might be hard pressed to tighten the check even further and fully validate the file contents, not only the prefix. In practice though that wouldn't make a lot of sense as it could be that the repository uses a hash function that produces longer hashes than SHA256, but which the current version of Git doesn't understand yet. We'd still want to detect the repository as proper Git repository in that case, and we will fail eventually with a proper error message that the hash isn't understood when trying to set up the repository format.

It follows that we could just leave the current code intact, as in practice the code change doesn't have any user visible impact. But it also prepares us for `the_hash_algo` being unset when there is no repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

While `validate_headref()` is only called from `is_git_directory()` in "setup.c", it is currently implemented in "path.c". Move it over such that it becomes clear that it is only really used during setup in order to discover repositories.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 path.c  | 53 -----------------------------------------------------
 path.h  |  1 -
 setup.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 54 deletions(-)

The `OPT__ABBREV()` option can be used to add an option that abbreviates object IDs. When given a length longer than `the_hash_algo->hexsz`, then it will instead set the length to that maximum length.

It may not always be guaranteed that we have `the_hash_algo` initialized properly as the hash algorithm can only be set up after we have set up `the_repository`. In that case, the hash would always be truncated to the hex length of SHA1, which may not be what the user desires.

In practice it's not a problem as all commands that use `OPT__ABBREV()` also have `RUN_SETUP` set and thus cannot work without a repository. Consequently, both `the_repository` and `the_hash_algo` would be properly set up.

Regardless of that, harden the code to not truncate the length when we didn't set up a repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 parse-options-cb.c       |  3 ++-
 t/t0040-parse-options.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

The `default_attr_source()` function lazily computes the attr source supposedly once, only. This is done via a static variable `attr_source` that contains the resolved object ID of the attr source's tree. If the variable is the null object ID then we try to look up the attr source, otherwise we skip over it.

This approach is flawed though: the variable will never be set to anything else but the null object ID in case there is no attr source. Consequently, we re-compute the information on every call. And in the worst case, when we silently ignore bad trees, this will cause us to try and look up the treeish every single time.

Improve this by introducing a separate variable `has_attr_source` to track whether we already computed the attr source and, if so, whether we have an attr source or not.

This also allows us to convert the `ignore_bad_attr_tree` to not be static anymore as the code will only be executed once anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 attr.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

If either the `--attr-source` option or the `GIT_ATTR_SOURCE` envvar are set, then `compute_default_attr_source()` will try to look up the value as a treeish. It is possible to hit that function while outside of a Git repository though, for example when using `git grep --no-index`. In that case, Git will hit a bug because we try to look up the main ref store outside of a repository.

Handle the case gracefully and detect when we try to look up an attr source without a repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 attr.c                |  6 ++++++
 t/t0003-attributes.sh | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

The dumb HTTP transport tries to read the remote HEAD reference by downloading the "HEAD" file and then parsing it via `http_fetch_ref()`. This function will either parse the file as an object ID in case it is exactly `the_hash_algo->hexsz` long, or otherwise it will check whether the reference starts with "ref :" and parse it as a symbolic ref.

This is broken when parsing detached HEADs of a remote SHA256 repository because we never update `the_hash_algo` to the discovered remote object hash. Consequently, `the_hash_algo` will always be the fallback SHA1 hash algorithm, which will cause us to fail parsing HEAD altogteher when it contains a SHA256 object ID.

Fix this issue by setting up `the_hash_algo` via `repo_set_hash_algo()`. While at it, let's make the expected SHA1 fallback explicit in our code, which also addresses an upcoming issue where we are going to remove the SHA1 fallback for `the_hash_algo`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote-curl.c              | 19 ++++++++++++++++++-
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

The `--short=` option for git-rev-parse(1) allows the user to specify to how many characters object IDs should be shortened to. The option is broken though for SHA256 repositories because we set the maximum allowed hash size to `the_hash_algo->hexsz` before we have even set up the repo. Consequently, `the_hash_algo` will always be SHA1 and thus we truncate every hash after at most 40 characters.

Fix this by accessing `the_hash_algo` only after we have set up the repo.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/rev-parse.c  | 5 ++---
 t/t1500-rev-parse.sh | 6 ++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

We access `the_hash_algo` in git-blame(1) before we have executed `parse_options_start()`, which may not be properly set up in case we have no repository. This is fine for most of the part because all the call paths that lead to it (git-blame(1), git-annotate(1) as well as git-pick-axe(1)) specify `RUN_SETUP` and thus require a repository.

There is one exception though, namely when passing `-h` to print the help. Here we will access `the_hash_algo` even if there is no repo. This works fine right now because `the_hash_algo` gets sets up to point to the SHA1 algorithm via `initialize_repository()`. But we're about to stop doing this, and thus the code would lead to a `NULL` pointer exception.

Prepare the code for this and only access `the_hash_algo` after we are sure that there is a proper repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/blame.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Verifying a bundle requires us to have a repository. This is encoded in `verify_bundle()`, which will return an error if there is no repository. We call `open_bundle()` before we call `verify_bundle()` though, which already performs some verifications even though we may ultimately abort due to a missing repository.

This is problematic because `open_bundle()` already reads the bundle header and verifies that it contains a properly formatted hash. When there is no repository we have no clue what hash function to expect though, so we always end up assuming SHA1 here, which may or may not be correct. Furthermore, we are about to stop initializing `the_hash_algo` when there is no repository, which will lead to segfaults.

Check early on whether we have a repository to fix this issue.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/bundle.c | 5 +++++
 1 file changed, 5 insertions(+)

The git-diff(1) command can be used outside repositories to diff two files with each other. But even if there is no repository we will end up hashing the files that we are diffing so that we can print the "index" line:

    ```
    diff --git a/a b/b
    index 7898192..6178079 100644
    --- a/a
    +++ b/b
    @@ -1 +1 @@
    -a
    +b
    ```

We implicitly use SHA1 to calculate the hash here, which is because `the_repository` gets initialized with SHA1 during the startup routine. We are about to stop doing this though such that `the_repository` only ever has a hash function when it was properly initialized via a repo's configuration.

To give full control to our users, we would ideally add a new switch to git-diff(1) that allows them to specify the hash function when executed outside of a repository. But for now, we only convert the code to make this explicit such that we can stop setting the default hash algorithm for `the_repository`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/diff.c | 9 +++++++++
 1 file changed, 9 insertions(+)

It is possible to run git-shortlog(1) outside of a repository by passing it output from git-log(1) via standard input. Obviously, as there is no repository in that context, it is thus unsupported to pass any revisions as arguments.

Regardless of that we still end up calling `setup_revisions()`. While that works alright, it is somewhat strange. Furthermore, this is about to cause problems when we unset the default object hash.

Refactor the code to only call `setup_revisions()` when we have a repository. This is safe to do as we already verify that there are no arguments when running outside of a repository anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/shortlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

During the startup of Git, we call `initialize_the_repository()` to set up `the_repository` as well as `the_index`. Part of this setup is also to set the default object hash of the repository to SHA1. This has the effect that `the_hash_algo` is getting initialized to SHA1, as well. This default hash algorithm eventually gets overridden by most Git commands via `setup_git_directory()`, which also detects the actual hash algorithm used by the repository.

There are some commands though that don't access a repository at all, or at a later point only, and thus retain the default hash function for some amount of time. As some of the the preceding commits demonstrate, this can lead to subtle issues when we access `the_hash_algo` when no repository has been set up.

Address this issue by dropping the set up of the default hash algorithm completely. The effect of this is that `the_hash_algo` will map to a `NULL` pointer and thus cause Git to crash when something tries to access the hash algorithm without it being properly initialized. It thus forces all Git commands to explicitly set up the hash algorithm in case there is no repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 repository.c | 2 --
 1 file changed, 2 deletions(-)

Patrick Steinhardt <ps@pks.im> writes:

[cc: added Josh as the commit-graph fuzzer was his creation].

With this topic merged, 'seen' fails "fuzz smoke test"; I think this

    https://github.com/git/git/actions/runs/8807729398/job/24175445340

is the first merge of this topic into 'seen' where "fuzz smoke test" started failing.

With the merge of the topic from 'seen' reverted tentatively,

    https://github.com/git/git/actions/runs/8862811497/job/24336185541

the same test seems happy.

On Sat, Apr 27, 2024 at 03:09:43PM -0700, Junio C Hamano wrote:

Indeed, thanks for the heads up! Another test gap that we have in the GitLab CI setup. I'll add a separate patch series to plug this gap and add the job to GitLab CI, too.

The fix for the failure is easy:

    diff --git a/oss-fuzz/fuzz-commit-graph.c b/oss-fuzz/fuzz-commit-graph.c
    index 2992079dd9..94ecbb9242 100644
    --- a/oss-fuzz/fuzz-commit-graph.c
    +++ b/oss-fuzz/fuzz-commit-graph.c
    @@ -18,6 +18,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
         * touching the disk to keep the individual fuzz-test cases as fast as
         * possible.
         */
    +	repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
        the_repository->settings.commit_graph_generation_version = 2;
        the_repository->settings.commit_graph_read_changed_paths = 1;
        g = parse_commit_graph(&the_repository->settings, (void *)data, size);

I'll send a v3.

Thanks!

Patrick

Hi,

this is the third version of my patch series that stops relying on the SHA1 fallback configured for `the_hash_algo`.

There's only a single change compared to v2, which is a new patch that fixes a segfault in the commit-graph fuzzer.

Thanks!

Patrick

Patrick Steinhardt (13):
  path: harden validation of HEAD with non-standard hashes
  path: move `validate_headref()` to its only user
  parse-options-cb: only abbreviate hashes when hash algo is known
  attr: don't recompute default attribute source
  attr: fix BUG() when parsing attrs outside of repo
  remote-curl: fix parsing of detached SHA256 heads
  builtin/rev-parse: allow shortening to more than 40 hex characters
  builtin/blame: don't access potentially unitialized `the_hash_algo`
  builtin/bundle: abort "verify" early when there is no repository
  builtin/diff: explicitly set hash algo when there is no repo
  builtin/shortlog: don't set up revisions without repo
  oss-fuzz/commit-graph: set up hash algorithm
  repository: stop setting SHA1 as the default object hash

 attr.c                       | 31 +++++++++++++++------
 builtin/blame.c              |  5 ++--
 builtin/bundle.c             |  5 ++++
 builtin/diff.c               |  9 ++++++
 builtin/rev-parse.c          |  5 ++--
 builtin/shortlog.c           |  2 +-
 oss-fuzz/fuzz-commit-graph.c |  1 +
 parse-options-cb.c           |  3 +-
 path.c                       | 53 ------------------------------------
 path.h                       |  1 -
 remote-curl.c                | 19 ++++++++++++-
 repository.c                 |  2 --
 setup.c                      | 53 ++++++++++++++++++++++++++++++++++++
 t/t0003-attributes.sh        | 15 ++++++++++
 t/t0040-parse-options.sh     | 17 ++++++++++++
 t/t1500-rev-parse.sh         |  6 ++++
 t/t5550-http-fetch-dumb.sh   | 15 ++++++++++
 17 files changed, 168 insertions(+), 74 deletions(-)

Range-diff against v2:
 1:  a986b464d3 =  1:  5134f35cda path: harden validation of HEAD with non-standard hashes
 2:  a347c7e6ca =  2:  589b6a99ef path: move `validate_headref()` to its only user
 3:  c0a15b2fa6 =  3:  9a63c445d2 parse-options-cb: only abbreviate hashes when hash algo is known
 4:  1b5f904eed =  4:  929bacbfce attr: don't recompute default attribute source
 5:  26909daca4 =  5:  8f20aec1ee attr: fix BUG() when parsing attrs outside of repo
 6:  0b99184f50 =  6:  53439067a1 remote-curl: fix parsing of detached SHA256 heads
 7:  ccfda3c2d2 =  7:  1f74960760 builtin/rev-parse: allow shortening to more than 40 hex characters
 8:  1813e7eb5c =  8:  2d985abca1 builtin/blame: don't access potentially unitialized `the_hash_algo`
 9:  31182a1fc6 =  9:  f3b23d28aa builtin/bundle: abort "verify" early when there is no repository
10:  78e19d0a1b = 10:  7577b6b96c builtin/diff: explicitly set hash algo when there is no repo
11:  51bcddbc31 = 11:  509c79d1d3 builtin/shortlog: don't set up revisions without repo
 -:  ---------- > 12:  660f976129 oss-fuzz/commit-graph: set up hash algorithm
12:  e8126371e1 = 13:  95909c2da5 repository: stop setting SHA1 as the default object hash

The `validate_headref()` function takes a path to a supposed "HEAD" file and checks whether its format is something that we understand. It is used as part of our repository discovery to check whether a specific directory is a Git directory or not.

Part of the validation is a check for a detached HEAD that contains a plain object ID. To do this validation we use `get_oid_hex()`, which relies on `the_hash_algo`. At this point in time the hash algo cannot yet be initialized though because we didn't yet read the Git config. Consequently, it will always be the SHA1 hash algorithm.

In practice this works alright because `get_oid_hex()` only ends up checking whether the prefix of the buffer is a valid object ID. And because SHA1 is shorter than SHA256, the function will successfully parse SHA256 object IDs, as well.

It is somewhat fragile though and not really the intent to only check for SHA1. With this in mind, harden the code to use `get_oid_hex_any()` to check whether the "HEAD" file parses as any known hash.

One might be hard pressed to tighten the check even further and fully validate the file contents, not only the prefix. In practice though that wouldn't make a lot of sense as it could be that the repository uses a hash function that produces longer hashes than SHA256, but which the current version of Git doesn't understand yet. We'd still want to detect the repository as proper Git repository in that case, and we will fail eventually with a proper error message that the hash isn't understood when trying to set up the repository format.

It follows that we could just leave the current code intact, as in practice the code change doesn't have any user visible impact. But it also prepares us for `the_hash_algo` being unset when there is no repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

While `validate_headref()` is only called from `is_git_directory()` in "setup.c", it is currently implemented in "path.c". Move it over such that it becomes clear that it is only really used during setup in order to discover repositories.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 path.c  | 53 -----------------------------------------------------
 path.h  |  1 -
 setup.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 54 deletions(-)

The `OPT__ABBREV()` option can be used to add an option that abbreviates object IDs. When given a length longer than `the_hash_algo->hexsz`, then it will instead set the length to that maximum length.

It may not always be guaranteed that we have `the_hash_algo` initialized properly as the hash algorithm can only be set up after we have set up `the_repository`. In that case, the hash would always be truncated to the hex length of SHA1, which may not be what the user desires.

In practice it's not a problem as all commands that use `OPT__ABBREV()` also have `RUN_SETUP` set and thus cannot work without a repository. Consequently, both `the_repository` and `the_hash_algo` would be properly set up.

Regardless of that, harden the code to not truncate the length when we didn't set up a repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 parse-options-cb.c       |  3 ++-
 t/t0040-parse-options.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

The `default_attr_source()` function lazily computes the attr source supposedly once, only. This is done via a static variable `attr_source` that contains the resolved object ID of the attr source's tree. If the variable is the null object ID then we try to look up the attr source, otherwise we skip over it.

This approach is flawed though: the variable will never be set to anything else but the null object ID in case there is no attr source. Consequently, we re-compute the information on every call. And in the worst case, when we silently ignore bad trees, this will cause us to try and look up the treeish every single time.

Improve this by introducing a separate variable `has_attr_source` to track whether we already computed the attr source and, if so, whether we have an attr source or not.

This also allows us to convert the `ignore_bad_attr_tree` to not be static anymore as the code will only be executed once anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 attr.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

If either the `--attr-source` option or the `GIT_ATTR_SOURCE` envvar are set, then `compute_default_attr_source()` will try to look up the value as a treeish. It is possible to hit that function while outside of a Git repository though, for example when using `git grep --no-index`. In that case, Git will hit a bug because we try to look up the main ref store outside of a repository.

Handle the case gracefully and detect when we try to look up an attr source without a repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 attr.c                |  6 ++++++
 t/t0003-attributes.sh | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

The dumb HTTP transport tries to read the remote HEAD reference by downloading the "HEAD" file and then parsing it via `http_fetch_ref()`. This function will either parse the file as an object ID in case it is exactly `the_hash_algo->hexsz` long, or otherwise it will check whether the reference starts with "ref :" and parse it as a symbolic ref.

This is broken when parsing detached HEADs of a remote SHA256 repository because we never update `the_hash_algo` to the discovered remote object hash. Consequently, `the_hash_algo` will always be the fallback SHA1 hash algorithm, which will cause us to fail parsing HEAD altogteher when it contains a SHA256 object ID.

Fix this issue by setting up `the_hash_algo` via `repo_set_hash_algo()`. While at it, let's make the expected SHA1 fallback explicit in our code, which also addresses an upcoming issue where we are going to remove the SHA1 fallback for `the_hash_algo`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote-curl.c              | 19 ++++++++++++++++++-
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

The `--short=` option for git-rev-parse(1) allows the user to specify to how many characters object IDs should be shortened to. The option is broken though for SHA256 repositories because we set the maximum allowed hash size to `the_hash_algo->hexsz` before we have even set up the repo. Consequently, `the_hash_algo` will always be SHA1 and thus we truncate every hash after at most 40 characters.

Fix this by accessing `the_hash_algo` only after we have set up the repo.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/rev-parse.c  | 5 ++---
 t/t1500-rev-parse.sh | 6 ++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

We access `the_hash_algo` in git-blame(1) before we have executed `parse_options_start()`, which may not be properly set up in case we have no repository. This is fine for most of the part because all the call paths that lead to it (git-blame(1), git-annotate(1) as well as git-pick-axe(1)) specify `RUN_SETUP` and thus require a repository.

There is one exception though, namely when passing `-h` to print the help. Here we will access `the_hash_algo` even if there is no repo. This works fine right now because `the_hash_algo` gets sets up to point to the SHA1 algorithm via `initialize_repository()`. But we're about to stop doing this, and thus the code would lead to a `NULL` pointer exception.

Prepare the code for this and only access `the_hash_algo` after we are sure that there is a proper repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/blame.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Verifying a bundle requires us to have a repository. This is encoded in `verify_bundle()`, which will return an error if there is no repository. We call `open_bundle()` before we call `verify_bundle()` though, which already performs some verifications even though we may ultimately abort due to a missing repository.

This is problematic because `open_bundle()` already reads the bundle header and verifies that it contains a properly formatted hash. When there is no repository we have no clue what hash function to expect though, so we always end up assuming SHA1 here, which may or may not be correct. Furthermore, we are about to stop initializing `the_hash_algo` when there is no repository, which will lead to segfaults.

Check early on whether we have a repository to fix this issue.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/bundle.c | 5 +++++
 1 file changed, 5 insertions(+)

The git-diff(1) command can be used outside repositories to diff two files with each other. But even if there is no repository we will end up hashing the files that we are diffing so that we can print the "index" line:

    ```
    diff --git a/a b/b
    index 7898192..6178079 100644
    --- a/a
    +++ b/b
    @@ -1 +1 @@
    -a
    +b
    ```

We implicitly use SHA1 to calculate the hash here, which is because `the_repository` gets initialized with SHA1 during the startup routine. We are about to stop doing this though such that `the_repository` only ever has a hash function when it was properly initialized via a repo's configuration.

To give full control to our users, we would ideally add a new switch to git-diff(1) that allows them to specify the hash function when executed outside of a repository. But for now, we only convert the code to make this explicit such that we can stop setting the default hash algorithm for `the_repository`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/diff.c | 9 +++++++++
 1 file changed, 9 insertions(+)

It is possible to run git-shortlog(1) outside of a repository by passing it output from git-log(1) via standard input. Obviously, as there is no repository in that context, it is thus unsupported to pass any revisions as arguments.

Regardless of that we still end up calling `setup_revisions()`. While that works alright, it is somewhat strange. Furthermore, this is about to cause problems when we unset the default object hash.

Refactor the code to only call `setup_revisions()` when we have a repository. This is safe to do as we already verify that there are no arguments when running outside of a repository anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/shortlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Our fuzzing setups don't work in a proper repository, but only use the in-memory configured `the_repository`. Consequently, we never go through the full repository setup procedures and thus do not set up the hash algo used by the repository.

The commit-graph fuzzer does rely on a properly initialized hash algo though. Initialize it explicitly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 oss-fuzz/fuzz-commit-graph.c | 1 +
 1 file changed, 1 insertion(+)

During the startup of Git, we call `initialize_the_repository()` to set up `the_repository` as well as `the_index`. Part of this setup is also to set the default object hash of the repository to SHA1. This has the effect that `the_hash_algo` is getting initialized to SHA1, as well. This default hash algorithm eventually gets overridden by most Git commands via `setup_git_directory()`, which also detects the actual hash algorithm used by the repository.

There are some commands though that don't access a repository at all, or at a later point only, and thus retain the default hash function for some amount of time. As some of the the preceding commits demonstrate, this can lead to subtle issues when we access `the_hash_algo` when no repository has been set up.

Address this issue by dropping the set up of the default hash algorithm completely. The effect of this is that `the_hash_algo` will map to a `NULL` pointer and thus cause Git to crash when something tries to access the hash algorithm without it being properly initialized. It thus forces all Git commands to explicitly set up the hash algorithm in case there is no repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 repository.c | 2 --
 1 file changed, 2 deletions(-)

Hi,

this is the third version of my patch series that stops relying on the SHA1 fallback configured for `the_hash_algo`. Instead, any callers that access `the_hash_algo` without it being initialized will now crash hard. This is designed to catch various subtle bugs going forward.

The only change compared to v3 of this patch series is a rebase on top of the following two topics:

  - ps/the-index-is-no-more, which causes a conflict in the final patch.

  - jc/no-default-attr-tree-in-bare, which causes a conflict in the
    fourth patch.

Interestingly, the range diff does not surface the second resolved conflict. This is because the context is actually the same, but when applying the patch in question you would get a conflict. To aid with this I decided to generate patches with `--unified=4`, which surfaces the previously-conflicting hunk in `compute_default_attr_source()`.

Patrick

Patrick Steinhardt (13):
  path: harden validation of HEAD with non-standard hashes
  path: move `validate_headref()` to its only user
  parse-options-cb: only abbreviate hashes when hash algo is known
  attr: don't recompute default attribute source
  attr: fix BUG() when parsing attrs outside of repo
  remote-curl: fix parsing of detached SHA256 heads
  builtin/rev-parse: allow shortening to more than 40 hex characters
  builtin/blame: don't access potentially unitialized `the_hash_algo`
  builtin/bundle: abort "verify" early when there is no repository
  builtin/diff: explicitly set hash algo when there is no repo
  builtin/shortlog: don't set up revisions without repo
  oss-fuzz/commit-graph: set up hash algorithm
  repository: stop setting SHA1 as the default object hash

 attr.c                       | 31 +++++++++++++++------
 builtin/blame.c              |  5 ++--
 builtin/bundle.c             |  5 ++++
 builtin/diff.c               |  9 ++++++
 builtin/rev-parse.c          |  5 ++--
 builtin/shortlog.c           |  2 +-
 oss-fuzz/fuzz-commit-graph.c |  1 +
 parse-options-cb.c           |  3 +-
 path.c                       | 53 ------------------------------------
 path.h                       |  1 -
 remote-curl.c                | 19 ++++++++++++-
 repository.c                 | 20 --------------
 setup.c                      | 53 ++++++++++++++++++++++++++++++++++++
 t/t0003-attributes.sh        | 15 ++++++++++
 t/t0040-parse-options.sh     | 17 ++++++++++++
 t/t1500-rev-parse.sh         |  6 ++++
 t/t5550-http-fetch-dumb.sh   | 15 ++++++++++
 17 files changed, 168 insertions(+), 92 deletions(-)

Range-diff against v3:
 1:  5134f35cda =  1:  5ee372c2d8 path: harden validation of HEAD with non-standard hashes
 2:  589b6a99ef =  2:  ece0ab94a8 path: move `validate_headref()` to its only user
 3:  9a63c445d2 =  3:  1a0859eaf1 parse-options-cb: only abbreviate hashes when hash algo is known
 4:  929bacbfce =  4:  1204a34216 attr: don't recompute default attribute source
 5:  8f20aec1ee =  5:  f098ce9690 attr: fix BUG() when parsing attrs outside of repo
 6:  53439067a1 =  6:  e8876052ad remote-curl: fix parsing of detached SHA256 heads
 7:  1f74960760 =  7:  6116030310 builtin/rev-parse: allow shortening to more than 40 hex characters
 8:  2d985abca1 =  8:  872ded113e builtin/blame: don't access potentially unitialized `the_hash_algo`
 9:  f3b23d28aa =  9:  5b4a21d2ce builtin/bundle: abort "verify" early when there is no repository
10:  7577b6b96c = 10:  e7254ae757 builtin/diff: explicitly set hash algo when there is no repo
11:  509c79d1d3 = 11:  c21b15ba17 builtin/shortlog: don't set up revisions without repo
12:  660f976129 = 12:  f37beb0e89 oss-fuzz/commit-graph: set up hash algorithm
13:  95909c2da5 ! 13:  950b08bc78 repository: stop setting SHA1 as the default object hash
    @@ Commit message
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## repository.c ##
    -@@ repository.c: void initialize_the_repository(void)
    - 	the_repo.parsed_objects = parsed_object_pool_new();
    - 
    - 	index_state_init(&the_index, the_repository);
    +@@ repository.c: void initialize_repository(struct repository *repo)
    + 	repo->parsed_objects = parsed_object_pool_new();
    + 	ALLOC_ARRAY(repo->index, 1);
    + 	index_state_init(repo->index, repo);
     -
    --	repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
    +-	/*
    +-	 * Unfortunately, we need to keep this hack around for the time being:
    +-	 *
    +-	 *   - Not setting up the hash algorithm for `the_repository` leads to
    +-	 *     crashes because `the_hash_algo` is a macro that expands to
    +-	 *     `the_repository->hash_algo`. So if Git commands try to access
    +-	 *     `the_hash_algo` without a Git directory we crash.
    +-	 *
    +-	 *   - Setting up the hash algorithm to be SHA1 by default breaks other
    +-	 *     commands when running with SHA256.
    +-	 *
    +-	 * This is another point in case why having global state is a bad idea.
    +-	 * Eventually, we should remove this hack and stop setting the hash
    +-	 * algorithm in this function altogether. Instead, it should only ever
    +-	 * be set via our repository setup procedures. But that requires more
    +-	 * work.
    +-	 */
    +-	if (repo == the_repository)
    +-		repo_set_hash_algo(repo, GIT_HASH_SHA1);
      }
      
      static void expand_base_dir(char **out, const char *in,

The `validate_headref()` function takes a path to a supposed "HEAD" file and checks whether its format is something that we understand. It is used as part of our repository discovery to check whether a specific directory is a Git directory or not.

Part of the validation is a check for a detached HEAD that contains a plain object ID. To do this validation we use `get_oid_hex()`, which relies on `the_hash_algo`. At this point in time the hash algo cannot yet be initialized though because we didn't yet read the Git config. Consequently, it will always be the SHA1 hash algorithm.

In practice this works alright because `get_oid_hex()` only ends up checking whether the prefix of the buffer is a valid object ID. And because SHA1 is shorter than SHA256, the function will successfully parse SHA256 object IDs, as well.

It is somewhat fragile though and not really the intent to only check for SHA1. With this in mind, harden the code to use `get_oid_hex_any()` to check whether the "HEAD" file parses as any known hash.

One might be hard pressed to tighten the check even further and fully validate the file contents, not only the prefix. In practice though that wouldn't make a lot of sense as it could be that the repository uses a hash function that produces longer hashes than SHA256, but which the current version of Git doesn't understand yet. We'd still want to detect the repository as proper Git repository in that case, and we will fail eventually with a proper error message that the hash isn't understood when trying to set up the repository format.

It follows that we could just leave the current code intact, as in practice the code change doesn't have any user visible impact. But it also prepares us for `the_hash_algo` being unset when there is no repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

While `validate_headref()` is only called from `is_git_directory()` in "setup.c", it is currently implemented in "path.c". Move it over such that it becomes clear that it is only really used during setup in order to discover repositories.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 path.c  | 53 -----------------------------------------------------
 path.h  |  1 -
 setup.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 54 deletions(-)

The `OPT__ABBREV()` option can be used to add an option that abbreviates object IDs. When given a length longer than `the_hash_algo->hexsz`, then it will instead set the length to that maximum length.

It may not always be guaranteed that we have `the_hash_algo` initialized properly as the hash algorithm can only be set up after we have set up `the_repository`. In that case, the hash would always be truncated to the hex length of SHA1, which may not be what the user desires.

In practice it's not a problem as all commands that use `OPT__ABBREV()` also have `RUN_SETUP` set and thus cannot work without a repository. Consequently, both `the_repository` and `the_hash_algo` would be properly set up.

Regardless of that, harden the code to not truncate the length when we didn't set up a repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 parse-options-cb.c       |  3 ++-
 t/t0040-parse-options.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

The `default_attr_source()` function lazily computes the attr source supposedly once, only. This is done via a static variable `attr_source` that contains the resolved object ID of the attr source's tree. If the variable is the null object ID then we try to look up the attr source, otherwise we skip over it.

This approach is flawed though: the variable will never be set to anything else but the null object ID in case there is no attr source. Consequently, we re-compute the information on every call. And in the worst case, when we silently ignore bad trees, this will cause us to try and look up the treeish every single time.

Improve this by introducing a separate variable `has_attr_source` to track whether we already computed the attr source and, if so, whether we have an attr source or not.

This also allows us to convert the `ignore_bad_attr_tree` to not be static anymore as the code will only be executed once anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 attr.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

If either the `--attr-source` option or the `GIT_ATTR_SOURCE` envvar are set, then `compute_default_attr_source()` will try to look up the value as a treeish. It is possible to hit that function while outside of a Git repository though, for example when using `git grep --no-index`. In that case, Git will hit a bug because we try to look up the main ref store outside of a repository.

Handle the case gracefully and detect when we try to look up an attr source without a repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 attr.c                |  6 ++++++
 t/t0003-attributes.sh | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

The dumb HTTP transport tries to read the remote HEAD reference by downloading the "HEAD" file and then parsing it via `http_fetch_ref()`. This function will either parse the file as an object ID in case it is exactly `the_hash_algo->hexsz` long, or otherwise it will check whether the reference starts with "ref :" and parse it as a symbolic ref.

This is broken when parsing detached HEADs of a remote SHA256 repository because we never update `the_hash_algo` to the discovered remote object hash. Consequently, `the_hash_algo` will always be the fallback SHA1 hash algorithm, which will cause us to fail parsing HEAD altogteher when it contains a SHA256 object ID.

Fix this issue by setting up `the_hash_algo` via `repo_set_hash_algo()`. While at it, let's make the expected SHA1 fallback explicit in our code, which also addresses an upcoming issue where we are going to remove the SHA1 fallback for `the_hash_algo`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote-curl.c              | 19 ++++++++++++++++++-
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

The `--short=` option for git-rev-parse(1) allows the user to specify to how many characters object IDs should be shortened to. The option is broken though for SHA256 repositories because we set the maximum allowed hash size to `the_hash_algo->hexsz` before we have even set up the repo. Consequently, `the_hash_algo` will always be SHA1 and thus we truncate every hash after at most 40 characters.

Fix this by accessing `the_hash_algo` only after we have set up the repo.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/rev-parse.c  | 5 ++---
 t/t1500-rev-parse.sh | 6 ++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

We access `the_hash_algo` in git-blame(1) before we have executed `parse_options_start()`, which may not be properly set up in case we have no repository. This is fine for most of the part because all the call paths that lead to it (git-blame(1), git-annotate(1) as well as git-pick-axe(1)) specify `RUN_SETUP` and thus require a repository.

There is one exception though, namely when passing `-h` to print the help. Here we will access `the_hash_algo` even if there is no repo. This works fine right now because `the_hash_algo` gets sets up to point to the SHA1 algorithm via `initialize_repository()`. But we're about to stop doing this, and thus the code would lead to a `NULL` pointer exception.

Prepare the code for this and only access `the_hash_algo` after we are sure that there is a proper repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/blame.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Verifying a bundle requires us to have a repository. This is encoded in `verify_bundle()`, which will return an error if there is no repository. We call `open_bundle()` before we call `verify_bundle()` though, which already performs some verifications even though we may ultimately abort due to a missing repository.

This is problematic because `open_bundle()` already reads the bundle header and verifies that it contains a properly formatted hash. When there is no repository we have no clue what hash function to expect though, so we always end up assuming SHA1 here, which may or may not be correct. Furthermore, we are about to stop initializing `the_hash_algo` when there is no repository, which will lead to segfaults.

Check early on whether we have a repository to fix this issue.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/bundle.c | 5 +++++
 1 file changed, 5 insertions(+)

The git-diff(1) command can be used outside repositories to diff two files with each other. But even if there is no repository we will end up hashing the files that we are diffing so that we can print the "index" line:

    ```
    diff --git a/a b/b
    index 7898192..6178079 100644
    --- a/a
    +++ b/b
    @@ -1 +1 @@
    -a
    +b
    ```

We implicitly use SHA1 to calculate the hash here, which is because `the_repository` gets initialized with SHA1 during the startup routine. We are about to stop doing this though such that `the_repository` only ever has a hash function when it was properly initialized via a repo's configuration.

To give full control to our users, we would ideally add a new switch to git-diff(1) that allows them to specify the hash function when executed outside of a repository. But for now, we only convert the code to make this explicit such that we can stop setting the default hash algorithm for `the_repository`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/diff.c | 9 +++++++++
 1 file changed, 9 insertions(+)

It is possible to run git-shortlog(1) outside of a repository by passing it output from git-log(1) via standard input. Obviously, as there is no repository in that context, it is thus unsupported to pass any revisions as arguments.

Regardless of that we still end up calling `setup_revisions()`. While that works alright, it is somewhat strange. Furthermore, this is about to cause problems when we unset the default object hash.

Refactor the code to only call `setup_revisions()` when we have a repository. This is safe to do as we already verify that there are no arguments when running outside of a repository anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/shortlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Our fuzzing setups don't work in a proper repository, but only use the in-memory configured `the_repository`. Consequently, we never go through the full repository setup procedures and thus do not set up the hash algo used by the repository.

The commit-graph fuzzer does rely on a properly initialized hash algo though. Initialize it explicitly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 oss-fuzz/fuzz-commit-graph.c | 1 +
 1 file changed, 1 insertion(+)

During the startup of Git, we call `initialize_the_repository()` to set up `the_repository` as well as `the_index`. Part of this setup is also to set the default object hash of the repository to SHA1. This has the effect that `the_hash_algo` is getting initialized to SHA1, as well. This default hash algorithm eventually gets overridden by most Git commands via `setup_git_directory()`, which also detects the actual hash algorithm used by the repository.

There are some commands though that don't access a repository at all, or at a later point only, and thus retain the default hash function for some amount of time. As some of the the preceding commits demonstrate, this can lead to subtle issues when we access `the_hash_algo` when no repository has been set up.

Address this issue by dropping the set up of the default hash algorithm completely. The effect of this is that `the_hash_algo` will map to a `NULL` pointer and thus cause Git to crash when something tries to access the hash algorithm without it being properly initialized. It thus forces all Git commands to explicitly set up the hash algorithm in case there is no repository.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 repository.c | 20 --------------------
 1 file changed, 20 deletions(-)