Re: [PATCH 01/13] odb: fix subtle logic to check whether an alternate is usable

> When adding an alternate to the object database we first check whether
> or not the path is usable. A path is usable if:
>
>   - It actually exists.
>
>   - We don't have it in our object sources yet.
>
> While the former check is trivial enough, the latter part is somewhat
> subtle and prone for bugs. This is because the function doesn't only
> check whether or not the given path is usable. But if it _is_ usable, we
> also store that path in the map of object sources immediately.
>
> The tricky part here is that the path that gets stored in the map is
> _not_ copied. Instead, we rely on the fact that subsequent code uses
> `strbuf_detach()` to store the exact same allocated memory in the
> created object source. Consequently, the memory is owned by the source
> but _also_ stored in the map. This subtlety is easy to miss, so if one
> decides to refactor this code one can easily end up breaking this
> mechanism.
>
> Make the relationship more explicit by not storing the path as part of
> `alt_odb_usable()`. Instead, we store the path after we have created the
> source now so that we can use the source's path pointer directly.
>

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/odb.c b/odb.c
> index 00a6e71568b..57d85ed9505 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -86,17 +86,16 @@ int odb_mkstemp(struct object_database *odb,
>  /*
>   * Return non-zero iff the path is usable as an alternate object database.
>   */
> -static int alt_odb_usable(struct object_database *o,
> -			  struct strbuf *path,
> -			  const char *normalized_objdir, khiter_t *pos)
> +static int alt_odb_usable(struct object_database *o, const char *path,
> +			  const char *normalized_objdir)
>  {
>  	int r;
>
>  	/* Detect cases where alternate disappeared */
> -	if (!is_directory(path->buf)) {
> +	if (!is_directory(path)) {
>  		error(_("object directory %s does not exist; "
>  			"check .git/objects/info/alternates"),
> -		      path->buf);
> +		      path);
>  		return 0;
>  	}
>
> @@ -113,11 +112,14 @@ static int alt_odb_usable(struct object_database *o,
>  		assert(r == 1); /* never used */
>  		kh_value(o->source_by_path, p) = o->sources;
>  	}
> -	if (fspatheq(path->buf, normalized_objdir))
> +
> +	if (fspatheq(path, normalized_objdir))
> +		return 0;
> +
> +	if (kh_get_odb_path_map(o->source_by_path, path) < kh_end(o->source_by_path))
>  		return 0;
> -	*pos = kh_put_odb_path_map(o->source_by_path, path->buf, &r);
> -	/* r: 0 = exists, 1 = never used, 2 = deleted */
> -	return r == 0 ? 0 : 1;
> +
> +	return 1;
>  }
>

>  /*
> @@ -148,6 +150,7 @@ static struct odb_source *link_alt_odb_entry(struct object_database *odb,
>  	struct strbuf pathbuf = STRBUF_INIT;
>  	struct strbuf tmp = STRBUF_INIT;
>  	khiter_t pos;
> +	int ret;
>
>  	if (!is_absolute_path(dir) && relative_base) {
>  		strbuf_realpath(&pathbuf, relative_base, 1);
> @@ -172,20 +175,21 @@ static struct odb_source *link_alt_odb_entry(struct object_database *odb,
>  	strbuf_reset(&tmp);
>  	strbuf_realpath(&tmp, odb->sources->path, 1);
>
> -	if (!alt_odb_usable(odb, &pathbuf, tmp.buf, &pos))
> +	if (!alt_odb_usable(odb, pathbuf.buf, tmp.buf))
>  		goto error;
>
>  	CALLOC_ARRAY(alternate, 1);
>  	alternate->odb = odb;
>  	alternate->local = false;
> -	/* pathbuf.buf is already in r->objects->source_by_path */
>  	alternate->path = strbuf_detach(&pathbuf, NULL);
>
>  	/* add the alternate entry */
>  	*odb->sources_tail = alternate;
>  	odb->sources_tail = &(alternate->next);
> -	alternate->next = NULL;
> -	assert(odb->source_by_path);
> +
> +	pos = kh_put_odb_path_map(odb->source_by_path, alternate->path, &ret);
> +	if (!ret)
> +		BUG("source must not yet exist");
>  	kh_value(odb->source_by_path, pos) = alternate;
>
>  	/* recursively add alternates */
>