"Jeff Hostetler via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Jeff Hostetler <jeffhostetler@xxxxxxxxxx>
>
> The call to index_file_exists() in the loop in expand_to_path() passes
> the wrong string length. Let's fix that.
>
> The loop in expand_to_path() searches the name-hash for each
> sub-directory prefix in the provided pathname. That is, by searching
> for "dir1/" then "dir1/dir2/" then "dir1/dir2/dir3/" and so on until
> it finds a cache-entry representing a sparse directory.
>
> The code creates "strbuf path_mutable" to contain the working pathname
> and modifies the buffer in-place by temporarily replacing the character
> following each successive "/" with NUL for the duration of the call to
> index_file_exists().
>
> It does not update the strbuf.len during this substitution.
Meaning we memihash() the full pathname munged with '/' -> NUL through
to the end of the original, when we should memihash() the truncated
leading pathname. This is bad, and the ...
>
> Pass the patched length of the prefix path instead.
... fix looks quite straight-forward.
> Signed-off-by: Jeff Hostetler <jeffhostetler@xxxxxxxxxx>
> ---
The problem description and the fix makes sense, but did you
actually see an end-user visible breakage due to this bug? I am
wondering how you found it, and if it is reasonable to have a test
demonstrate the breakage.
> sparse-index.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sparse-index.c b/sparse-index.c
> index 1fdb07a9e69..093708f6220 100644
> --- a/sparse-index.c
> +++ b/sparse-index.c
> @@ -579,8 +579,9 @@ void expand_to_path(struct index_state *istate,
> replace++;
> temp = *replace;
> *replace = '\0';
> + substr_len = replace - path_mutable.buf;
> if (index_file_exists(istate, path_mutable.buf,
> - path_mutable.len, icase)) {
> + substr_len, icase)) {
There is a break out of this loop when the condition for this "if"
statement holds, but the value of substr_len does not affect what
happens after this index_file_exists() call (correctly) computes its
result. The fix looks good.
Thanks.
> /*
> * We found a parent directory in the name-hash
> * hashtable, because only sparse directory entries
> @@ -593,7 +594,6 @@ void expand_to_path(struct index_state *istate,
> }
>
> *replace = temp;
> - substr_len = replace - path_mutable.buf;
> }
>
> cleanup:
>
> base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40
