Re: [BUG?] submodule-config.c:config_from() has left me confused

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jul 22 2021, Atharva Raykar wrote:

> Hi Git Developers,

Hi, just skimming this, hopefully someone can get it the attention it
deserves...

> Since HEAD and HEAD^ are valid tree-ish objects, the gitmodule_oid_from_commit()
> line should have returned early with a NULL object, but that does not happen.
> The output seems to match what I would expect from the API.
>
> Applying Christian's suggestions pass the same test as well.
>
> I also wonder what is the situation with the case where, the oid is non-null
> and invalid?
>
> If we look at the code again:
>
> static int gitmodule_oid_from_commit(const struct object_id *treeish_name,
> 				     struct object_id *gitmodules_oid,
> 				     struct strbuf *rev)
> {
> 	int ret = 0;
>
> 	if (is_null_oid(treeish_name)) {
> 		oidclr(gitmodules_oid);
> 		return 1;
> 	}
>
> 	strbuf_addf(rev, "%s:.gitmodules", oid_to_hex(treeish_name));
> 	if (get_oid(rev->buf, gitmodules_oid) >= 0)
> 		ret = 1;
>
> 	return ret;
> }
>
> ...what happens to the value of gitmodules_oid when get_oid() fails?
>
> *If* it is set to NULL, it would probably lead to undefined behaviour when
> we try to hash the oid for the submodule cache lookup in [1]. If it is set
> to 'null_oid()' or zero'd out (like with oidclr()), then it should return
> the submodule from the latest revision, which does not seem desirable, as
> the input is invalid.

We won't set it to NULL, we'll att worst oidclr() it, or leave it alone,
no?

In general we should not rely on the values we expect to munge if we
return an error. In practice it's probably OK to rely on an oid
passed-in not being in an inconsistent state in that case.

It could be, e.g. this might eventually go to get_oid_basic() which in
calling get_hash_hex_algop() might partially have copied a hash when it
fails. I have not traced this codepath but it could happen in general.

In practice I think it's OK to rely on passing an OID in, and it being
bad or not, i.e.:

    struct object_id *oid = null_oid();
    if (get_oid(..., oid))
        error(...);
    else
        /* oid is still == null_oid, not some bizarro value, probably... */