{"thread":{"id":"60835","subject":"[PATCH] sparse-index: pass string length to index_file_exists()","startedAt":"2024-02-02T18:04:58Z","lastAt":"2024-02-02T19:30:46Z","messageCount":4,"participants":["Jeff Hostetler via GitGitGadget","Junio C Hamano","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"487837","messageId":"pull.1649.git.1706897095273.gitgitgadget@gmail.com","threadId":"60835","inReplyTo":null,"subject":"[PATCH] sparse-index: pass string length to index_file_exists()","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-02T18:04:54Z","receivedAt":"2024-02-02T18:04:58Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhostetler@github.com>\n\nThe call to index_file_exists() in the loop in expand_to_path() passes\nthe wrong string length.  Let's fix that.\n\nThe loop in expand_to_path() searches the name-hash for each\nsub-directory prefix in the provided pathname. That is, by searching\nfor \"dir1/\" then \"dir1/dir2/\" then \"dir1/dir2/dir3/\" and so on until\nit finds a cache-entry representing a sparse directory.\n\nThe code creates \"strbuf path_mutable\" to contain the working pathname\nand modifies the buffer in-place by temporarily replacing the character\nfollowing each successive \"/\" with NUL for the duration of the call to\nindex_file_exists().\n\nIt does not update the strbuf.len during this substitution.\n\nPass the patched length of the prefix path instead.\n\nSigned-off-by: Jeff Hostetler <jeffhostetler@github.com>\n---\n    sparse-index: pass string length to index_file_exists()\n    \n    The call to index_file_exists() in the loop in expand_to_path() passes\n    the wrong string length. Let's fix that.\n    \n    The loop in expand_to_path() searches the name-hash for each\n    sub-directory prefix in the provided pathname. That is, by searching for\n    \"dir1/\" then \"dir1/dir2/\" then \"dir1/dir2/dir3/\" and so on until it\n    finds a cache-entry representing a sparse directory.\n    \n    The code creates \"strbuf path_mutable\" to contain the working pathname\n    and modifies the buffer in-place by temporarily replacing the character\n    following each successive \"/\" with NUL for the duration of the call to\n    index_file_exists().\n    \n    It does not update the strbuf.len during this substitution.\n    \n    Pass the patched length of the prefix path instead.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1649%2Fjeffhostetler%2Fsparse-index-string-length-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1649/jeffhostetler/sparse-index-string-length-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1649\n\n sparse-index.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 1fdb07a9e69..093708f6220 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -579,8 +579,9 @@ void expand_to_path(struct index_state *istate,\n \t\treplace++;\n \t\ttemp = *replace;\n \t\t*replace = '\\0';\n+\t\tsubstr_len = replace - path_mutable.buf;\n \t\tif (index_file_exists(istate, path_mutable.buf,\n-\t\t\t\t      path_mutable.len, icase)) {\n+\t\t\t\t      substr_len, icase)) {\n \t\t\t/*\n \t\t\t * We found a parent directory in the name-hash\n \t\t\t * hashtable, because only sparse directory entries\n@@ -593,7 +594,6 @@ void expand_to_path(struct index_state *istate,\n \t\t}\n \n \t\t*replace = temp;\n-\t\tsubstr_len = replace - path_mutable.buf;\n \t}\n \n cleanup:\n\nbase-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40\n-- \ngitgitgadget\n"},{"id":"487838","messageId":"xmqqo7cysp01.fsf@gitster.g","threadId":"60835","inReplyTo":"pull.1649.git.1706897095273.gitgitgadget@gmail.com","subject":"Re: [PATCH] sparse-index: pass string length to index_file_exists()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-02T18:24:46Z","receivedAt":"2024-02-02T18:24:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jeff Hostetler via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Jeff Hostetler <jeffhostetler@github.com>\n>\n> The call to index_file_exists() in the loop in expand_to_path() passes\n> the wrong string length.  Let's fix that.\n>\n> The loop in expand_to_path() searches the name-hash for each\n> sub-directory prefix in the provided pathname. That is, by searching\n> for \"dir1/\" then \"dir1/dir2/\" then \"dir1/dir2/dir3/\" and so on until\n> it finds a cache-entry representing a sparse directory.\n>\n> The code creates \"strbuf path_mutable\" to contain the working pathname\n> and modifies the buffer in-place by temporarily replacing the character\n> following each successive \"/\" with NUL for the duration of the call to\n> index_file_exists().\n>\n> It does not update the strbuf.len during this substitution.\n\nMeaning we memihash() the full pathname munged with '/' -> NUL through\nto the end of the original, when we should memihash() the truncated\nleading pathname.  This is bad, and the ...\n\n>\n> Pass the patched length of the prefix path instead.\n\n... fix looks quite straight-forward.\n\n> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>\n> ---\n\nThe problem description and the fix makes sense, but did you\nactually see an end-user visible breakage due to this bug?  I am\nwondering how you found it, and if it is reasonable to have a test\ndemonstrate the breakage.\n\n>  sparse-index.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/sparse-index.c b/sparse-index.c\n> index 1fdb07a9e69..093708f6220 100644\n> --- a/sparse-index.c\n> +++ b/sparse-index.c\n> @@ -579,8 +579,9 @@ void expand_to_path(struct index_state *istate,\n>  \t\treplace++;\n>  \t\ttemp = *replace;\n>  \t\t*replace = '\\0';\n> +\t\tsubstr_len = replace - path_mutable.buf;\n>  \t\tif (index_file_exists(istate, path_mutable.buf,\n> -\t\t\t\t      path_mutable.len, icase)) {\n> +\t\t\t\t      substr_len, icase)) {\n\nThere is a break out of this loop when the condition for this \"if\"\nstatement holds, but the value of substr_len does not affect what\nhappens after this index_file_exists() call (correctly) computes its\nresult.  The fix looks good.\n\nThanks.\n\n>  \t\t\t/*\n>  \t\t\t * We found a parent directory in the name-hash\n>  \t\t\t * hashtable, because only sparse directory entries\n> @@ -593,7 +594,6 @@ void expand_to_path(struct index_state *istate,\n>  \t\t}\n>  \n>  \t\t*replace = temp;\n> -\t\tsubstr_len = replace - path_mutable.buf;\n>  \t}\n>  \n>  cleanup:\n>\n> base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40\n"},{"id":"487843","messageId":"2bae709a-18da-6ab8-2b76-032ddc045641@jeffhostetler.com","threadId":"60835","inReplyTo":"xmqqo7cysp01.fsf@gitster.g","subject":"Re: [PATCH] sparse-index: pass string length to index_file_exists()","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2024-02-02T19:19:17Z","receivedAt":"2024-02-02T19:25:22Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 2/2/24 1:24 PM, Junio C Hamano wrote:\n> \"Jeff Hostetler via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Jeff Hostetler <jeffhostetler@github.com>\n>>\n>> The call to index_file_exists() in the loop in expand_to_path() passes\n>> the wrong string length.  Let's fix that.\n>>\n>> The loop in expand_to_path() searches the name-hash for each\n>> sub-directory prefix in the provided pathname. That is, by searching\n>> for \"dir1/\" then \"dir1/dir2/\" then \"dir1/dir2/dir3/\" and so on until\n>> it finds a cache-entry representing a sparse directory.\n>>\n>> The code creates \"strbuf path_mutable\" to contain the working pathname\n>> and modifies the buffer in-place by temporarily replacing the character\n>> following each successive \"/\" with NUL for the duration of the call to\n>> index_file_exists().\n>>\n>> It does not update the strbuf.len during this substitution.\n> \n> Meaning we memihash() the full pathname munged with '/' -> NUL through\n> to the end of the original, when we should memihash() the truncated\n> leading pathname.  This is bad, and the ...\n> \n>>\n>> Pass the patched length of the prefix path instead.\n> \n> ... fix looks quite straight-forward.\n> \n>> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>\n>> ---\n> \n> The problem description and the fix makes sense, but did you\n> actually see an end-user visible breakage due to this bug?  I am\n> wondering how you found it, and if it is reasonable to have a test\n> demonstrate the breakage.\n\nI'm working on a bug where the fsmonitor client doesn't\ninvalidate the CE flags when there is a case discrepancy between\nthe expected and observed case on a case-insensitive file system.\n(Fix due shortly.)\n\nAnd I was single-stepping in the debugger inside of\n`index_file_exists()` while tracking that down and noticed that the\nlength argument was bogus.  Something like { name=\"dir1/\", len=10 }\n\nI don't remember if this bug caused visible problems or not. It felt\nlike it caused a few extra rounds of mutually-recursive calls between\n`index_file_exists()` and `expand_to_path()`, but I can't say that\nfor certain (and I was focused on a different problem at the time).\n\nI do have some test code in `t/helper/lazy-init-name-hash.c` that\nI suppose we could extend to beat on it, but I'm not sure it is\nworth it in this case.\n\nJeff\n\n\n> \n>>   sparse-index.c | 4 ++--\n>>   1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/sparse-index.c b/sparse-index.c\n>> index 1fdb07a9e69..093708f6220 100644\n>> --- a/sparse-index.c\n>> +++ b/sparse-index.c\n>> @@ -579,8 +579,9 @@ void expand_to_path(struct index_state *istate,\n>>   \t\treplace++;\n>>   \t\ttemp = *replace;\n>>   \t\t*replace = '\\0';\n>> +\t\tsubstr_len = replace - path_mutable.buf;\n>>   \t\tif (index_file_exists(istate, path_mutable.buf,\n>> -\t\t\t\t      path_mutable.len, icase)) {\n>> +\t\t\t\t      substr_len, icase)) {\n> \n> There is a break out of this loop when the condition for this \"if\"\n> statement holds, but the value of substr_len does not affect what\n> happens after this index_file_exists() call (correctly) computes its\n> result.  The fix looks good.\n> \n> Thanks.\n> \n>>   \t\t\t/*\n>>   \t\t\t * We found a parent directory in the name-hash\n>>   \t\t\t * hashtable, because only sparse directory entries\n>> @@ -593,7 +594,6 @@ void expand_to_path(struct index_state *istate,\n>>   \t\t}\n>>   \n>>   \t\t*replace = temp;\n>> -\t\tsubstr_len = replace - path_mutable.buf;\n>>   \t}\n>>   \n>>   cleanup:\n>>\n>> base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40\n> \n"},{"id":"487844","messageId":"xmqq1q9usly8.fsf@gitster.g","threadId":"60835","inReplyTo":"2bae709a-18da-6ab8-2b76-032ddc045641@jeffhostetler.com","subject":"Re: [PATCH] sparse-index: pass string length to index_file_exists()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-02T19:30:39Z","receivedAt":"2024-02-02T19:30:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler <git@jeffhostetler.com> writes:\n\n> I don't remember if this bug caused visible problems or not. It felt\n> like it caused a few extra rounds of mutually-recursive calls between\n> `index_file_exists()` and `expand_to_path()`, but I can't say that\n> for certain (and I was focused on a different problem at the time).\n>\n> I do have some test code in `t/helper/lazy-init-name-hash.c` that\n> I suppose we could extend to beat on it, but I'm not sure it is\n> worth it in this case.\n\nYeah, if we had a reproduction already handy that would have been a\ndifferent story, but I agree that it is not worth it.\n\nThanks for a fix.  Queued.\n\n\n"}]}