sparse-index: pass string length to index_file_exists()

From: Jeff Hostetler <[email protected]>

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.

Pass the patched length of the prefix path instead.

Signed-off-by Jeff Hostetler <[email protected]>

---

 sparse-index.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

sparse-index.c +0 -0

index 1fdb07a9e69..093708f6220 100644

sparse-index.c +0 -1

 			/*
 			 * 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

Quoted 10 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

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.

Quoted 6 lines

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 ...

Quoted 2 lines

Pass the patched length of the prefix path instead.

... fix looks quite straight-forward.

Quoted 2 lines

Signed-off-by Jeff Hostetler <[email protected]>

---

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.

Quoted 9 lines

  sparse-index.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

sparse-index.c +0 -0

index 1fdb07a9e69..093708f6220 100644

sparse-index.c void expand_to_path(struct index_state *istate, · line 584

Quoted 7 lines

sparse-index.c +2 -1

@@ -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.

Quoted 13 lines

sparse-index.c +0 -1

  			/*
  			 * 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

I don't remember if this bug caused visible problems or not. It felt like it caused a few extra rounds of mutually-recursive calls between index_file_exists() and expand_to_path(), but I can't say that for certain (and I was focused on a different problem at the time).

I do have some test code in t/helper/lazy-init-name-hash.c that I suppose we could extend to beat on it, but I'm not sure it is worth it in this case.