sparse-index: pass string length to index_file_exists() #1649
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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]>
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Jeff Hostetler via GitGitGadget" <[email protected]> writes:
> 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.
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 <[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.
> 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 |
|
On the Git mailing list, Jeff Hostetler wrote (reply to this): On 2/2/24 1:24 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <[email protected]> writes:
> >> 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.
> > 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 <[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.
I'm working on a bug where the fsmonitor client doesn't
invalidate the CE flags when there is a case discrepancy between
the expected and observed case on a case-insensitive file system.
(Fix due shortly.)
And I was single-stepping in the debugger inside of
`index_file_exists()` while tracking that down and noticed that the
length argument was bogus. Something like { name="dir1/", len=10 }
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.
Jeff
> >> 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
> |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff Hostetler <[email protected]> writes:
> 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.
Yeah, if we had a reproduction already handy that would have been a
different story, but I agree that it is not worth it.
Thanks for a fix. Queued.
|
|
This branch is now known as |
|
This patch series was integrated into seen via git@3d6ea72. |
|
There was a status update in the "New Topics" section about the branch A caller called index_file_exists() that takes a string expressed as <ptr, length> with a wrong length, which has been corrected. Will merge to 'next'. source: <[email protected]> |
|
This patch series was integrated into seen via git@0fd6bcf. |
|
There was a status update in the "Cooking" section about the branch A caller called index_file_exists() that takes a string expressed as <ptr, length> with a wrong length, which has been corrected. Will merge to 'next'. source: <[email protected]> |
|
This patch series was integrated into seen via git@6194a72. |
|
This patch series was integrated into seen via git@cb4d02c. |
|
This patch series was integrated into seen via git@fc987f8. |
|
This patch series was integrated into next via git@17ec59d. |
|
There was a status update in the "Cooking" section about the branch A caller called index_file_exists() that takes a string expressed as <ptr, length> with a wrong length, which has been corrected. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@dd504ca. |
|
This patch series was integrated into seen via git@15ad9f2. |
|
This patch series was integrated into seen via git@1188a79. |
|
This patch series was integrated into seen via git@08a0adb. |
|
This patch series was integrated into seen via git@294dd20. |
|
This patch series was integrated into master via git@294dd20. |
|
This patch series was integrated into next via git@294dd20. |
|
Closed via 294dd20. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
cc: Jeff Hostetler [email protected]