{"thread":{"id":"63200","subject":"[PATCH 0/2] Range-check array index before access","startedAt":"2025-03-26T17:26:55Z","lastAt":"2025-04-29T23:46:44Z","messageCount":18,"participants":["Johannes Schindelin via GitGitGadget","Jeff King","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"515100","messageId":"pull.1887.git.1743010011.gitgitgadget@gmail.com","threadId":"63200","inReplyTo":null,"subject":"[PATCH 0/2] Range-check array index before access","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-26T17:26:49Z","receivedAt":"2025-03-26T17:26:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"If we want to check the range of an array index, it makes much more sense to\ndo it before accessing the corresponding array element, not afterwards.\n\nThere are two more instances of this in the clar code, fixes for which I\noffer in https://github.com/clar-test/clar/pull/115.\n\nJohannes Schindelin (2):\n diff: check range before dereferencing an array element\n read-cache: check range before dereferencing an array element\n\n diff.c | 2 +-\n read-cache.c | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1887%2Fdscho%2Frange-check-array-index-before-access-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1887/dscho/range-check-array-index-before-access-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1887\n-- \ngitgitgadget\n"},{"id":"515101","messageId":"ddfb44ed924615bdb61a30ae7627326942575567.1743010011.git.gitgitgadget@gmail.com","threadId":"63200","inReplyTo":"pull.1887.git.1743010011.gitgitgadget@gmail.com","subject":"[PATCH 1/2] diff: check range before dereferencing an array element","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-26T17:26:50Z","receivedAt":"2025-03-26T17:26:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nBefore accessing an array element at a given index, we should make sure\nthat the index is within the desired bounds, not afterwards, otherwise\nit may not make sense to even access the array element in the first\nplace.\n\nPointed out by CodeQL's `cpp/offset-use-before-range-check` rule.\n\nSigned-off-by: Johannes Schindelin \n---\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex c89c15d98e0..18ba3060460 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)\n \n \t/* skip any \\v \\f \\r at start of indentation */\n \twhile (s[off] == '\\f' || s[off] == '\\v' ||\n-\t (s[off] == '\\r' && off < len - 1))\n+\t (off < len - 1 && s[off] == '\\r'))\n \t\toff++;\n \n \t/* calculate the visual width of indentation */\n-- \ngitgitgadget\n\n"},{"id":"515102","messageId":"d4e94a243b0633ab18daa6ce0ae766d5bc33364e.1743010011.git.gitgitgadget@gmail.com","threadId":"63200","inReplyTo":"pull.1887.git.1743010011.gitgitgadget@gmail.com","subject":"[PATCH 2/2] read-cache: check range before dereferencing an array element","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-26T17:26:51Z","receivedAt":"2025-03-26T17:26:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nBefore accessing an array element at a given index, we should make sure\nthat the index is within the desired bounds, not afterwards, otherwise\nit may not make sense to even access the array element in the first\nplace.\n\nPointed out by CodeQL's `cpp/offset-use-before-range-check` rule.\n\nSigned-off-by: Johannes Schindelin \n---\n read-cache.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex e678c13e8f1..08ae66ad609 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2686,8 +2686,8 @@ static int ce_write_entry(struct hashfile *f, struct cache_entry *ce,\n \t\tint common, to_remove, prefix_size;\n \t\tunsigned char to_remove_vi[16];\n \t\tfor (common = 0;\n-\t\t (ce->name[common] &&\n-\t\t common < previous_name->len &&\n+\t\t (common < previous_name->len &&\n+\t\t ce->name[common] &&\n \t\t ce->name[common] == previous_name->buf[common]);\n \t\t common++)\n \t\t\t; /* still matching */\n-- \ngitgitgadget\n"},{"id":"515106","messageId":"20250326180225.GC2508080@coredump.intra.peff.net","threadId":"63200","inReplyTo":"d4e94a243b0633ab18daa6ce0ae766d5bc33364e.1743010011.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] read-cache: check range before dereferencing an array element","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-26T18:02:25Z","receivedAt":"2025-03-26T18:02:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 26, 2025 at 05:26:51PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> Before accessing an array element at a given index, we should make sure\n> that the index is within the desired bounds, not afterwards, otherwise\n> it may not make sense to even access the array element in the first\n> place.\n\nCertainly we should, but...\n\n> diff --git a/read-cache.c b/read-cache.c\n> index e678c13e8f1..08ae66ad609 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2686,8 +2686,8 @@ static int ce_write_entry(struct hashfile *f, struct cache_entry *ce,\n> \t\tint common, to_remove, prefix_size;\n> \t\tunsigned char to_remove_vi[16];\n> \t\tfor (common = 0;\n> -\t\t (ce->name[common] &&\n> -\t\t common < previous_name->len &&\n> +\t\t (common < previous_name->len &&\n> +\t\t ce->name[common] &&\n> \t\t ce->name[common] == previous_name->buf[common]);\n> \t\t common++)\n\nIs previous_name->len an actual bound for ce->name?\n\nI think we are iterating through ce->name looking for either the\nterminating NUL, or matching the prefix from previous_name. So the\nlength check is for the third condition:\n\n ce->name[common] == previous_name->buf[common]\n\nand correctly comes before it.\n\nSo unless I'm mistaken, this is a false positive in CodeQL. I don't mind\nre-ordering the condition to fix it, but the commit message should\nprobably say so.\n\nSince previous_name is a strbuf, it is also NUL-terminated (and interior\nNUL bytes cannot be important, because we are comparing against a\nNUL-terminated ce->name). So I suspect that a simpler way to write it is\nto remove the length check and rely on the NUL/not-NUL mismatch to\nbreak, like:\n\n for (common = 0;\n ce->name[common] &&\n ce->name[common] == previous_name->buf[common];\n common++)\n\nWhich would also presumably remove the warning.\n\n-Peff\n\nPS I notice that \"common\" is an int, which always makes me wonder what\n would happen with a 2GB+1 filename. But I think that is nothing\n specific here, as there are ints all over the place for index name\n lengths. And anyway, one thing at a time, I suppose. :)\n"},{"id":"515170","messageId":"xmqq8qoqivp3.fsf@gitster.g","threadId":"63200","inReplyTo":"ddfb44ed924615bdb61a30ae7627326942575567.1743010011.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] diff: check range before dereferencing an array element","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-27T11:01:28Z","receivedAt":"2025-03-27T11:01:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> Before accessing an array element at a given index, we should make sure\n> that the index is within the desired bounds, not afterwards, otherwise\n> it may not make sense to even access the array element in the first\n> place.\n>\n> Pointed out by CodeQL's `cpp/offset-use-before-range-check` rule.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/diff.c b/diff.c\n> index c89c15d98e0..18ba3060460 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)\n> \n> \t/* skip any \\v \\f \\r at start of indentation */\n> \twhile (s[off] == '\\f' || s[off] == '\\v' ||\n> -\t (s[off] == '\\r' && off < len - 1))\n> +\t (off < len - 1 && s[off] == '\\r'))\n> \t\toff++;\n\nI suspect that this is another false positive, like Peff pointed out\nfor [2/2] of these two patches.\n\nEspecially if this change squelches the warning. \n\nIf the check against CR for s[off] could be oob without checking how\nlarge 'off' is, then the earlier checks for FF and VT should also be\nequally iffy. After all they are accessing the byte at the same\nlocation.\n\nI think what is going on is that the correctness of the code depends\non s[] having a sentinel (which is not FF/VT/CR; I do not offhand\nknow if it is NUL terminated or LF at the end of line) so any byte\nother than FF/VT/CR that are in the leading part of the line would\ncause us to exit the loop safely before going beyond the end of the\narray s[]. CR alone is special cased because we want to treat it\nlike FF/VT only if it is not a part of the EOL CR/LF (hence \"is our\nCR at one before the end of the line?\" check).\n"},{"id":"515171","messageId":"pull.1887.v2.git.1743073557.gitgitgadget@gmail.com","threadId":"63200","inReplyTo":"pull.1887.git.1743010011.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Range-check array index before access","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:05:55Z","receivedAt":"2025-03-27T11:06:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"If we want to check the range of an array index, it makes much more sense to\ndo it before accessing the corresponding array element, not afterwards.\n\nThere are two more instances of this in the clar code, fixes for which I\noffer in https://github.com/clar-test/clar/pull/115.\n\nChanges since v1:\n\n * Clarified in the commit message of the second patch that this range-check\n technically was already right before the array access it wants to guard,\n but that it still makes sense to move that range-check to the beginning\n of the loop condition.\n\nJohannes Schindelin (2):\n diff: check range before dereferencing an array element\n read-cache: check range before dereferencing an array element\n\n diff.c | 2 +-\n read-cache.c | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1887%2Fdscho%2Frange-check-array-index-before-access-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1887/dscho/range-check-array-index-before-access-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1887\n\nRange-diff vs v1:\n\n 1: ddfb44ed924 = 1: ddfb44ed924 diff: check range before dereferencing an array element\n 2: d4e94a243b0 ! 2: 73cae301293 read-cache: check range before dereferencing an array element\n @@ Commit message\n read-cache: check range before dereferencing an array element\n \n Before accessing an array element at a given index, we should make sure\n - that the index is within the desired bounds, not afterwards, otherwise\n - it may not make sense to even access the array element in the first\n - place.\n + that the index is within the desired bounds, otherwise it makes little\n + sense to access the array element in the first place.\n \n - Pointed out by CodeQL's `cpp/offset-use-before-range-check` rule.\n + In this instance, testing whether `ce->name[common]` is the trailing NUL\n + byte is technically different from testing whether `common` is within\n + the bounds of `previous_name`. It is also redundant, as the range-check\n + guarantees that `previous_name->buf[common]` cannot be NUL and therefore\n + the condition `ce->name[common] == previous_name->buf[common]` would not\n + be met if `ce->name[common]` evaluated to NUL.\n +\n + However, in the interest of reducing the cognitive load to reason about\n + the correctness of this loop (so that I can focus on interesting\n + projects again), I'll simply move the range-check to the beginning of\n + the loop condition and keep the redundant NUL check.\n +\n + This acquiesces CodeQL's `cpp/offset-use-before-range-check` rule.\n \n Signed-off-by: Johannes Schindelin \n \n\n-- \ngitgitgadget\n"},{"id":"515172","messageId":"ddfb44ed924615bdb61a30ae7627326942575567.1743073557.git.gitgitgadget@gmail.com","threadId":"63200","inReplyTo":"pull.1887.v2.git.1743073557.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] diff: check range before dereferencing an array element","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:05:56Z","receivedAt":"2025-03-27T11:06:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nBefore accessing an array element at a given index, we should make sure\nthat the index is within the desired bounds, not afterwards, otherwise\nit may not make sense to even access the array element in the first\nplace.\n\nPointed out by CodeQL's `cpp/offset-use-before-range-check` rule.\n\nSigned-off-by: Johannes Schindelin \n---\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex c89c15d98e0..18ba3060460 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)\n \n \t/* skip any \\v \\f \\r at start of indentation */\n \twhile (s[off] == '\\f' || s[off] == '\\v' ||\n-\t (s[off] == '\\r' && off < len - 1))\n+\t (off < len - 1 && s[off] == '\\r'))\n \t\toff++;\n \n \t/* calculate the visual width of indentation */\n-- \ngitgitgadget\n\n"},{"id":"515173","messageId":"73cae30129338cf219a810c3a2a78ef48d5637d0.1743073557.git.gitgitgadget@gmail.com","threadId":"63200","inReplyTo":"pull.1887.v2.git.1743073557.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] read-cache: check range before dereferencing an array element","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:05:57Z","receivedAt":"2025-03-27T11:06:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nBefore accessing an array element at a given index, we should make sure\nthat the index is within the desired bounds, otherwise it makes little\nsense to access the array element in the first place.\n\nIn this instance, testing whether `ce->name[common]` is the trailing NUL\nbyte is technically different from testing whether `common` is within\nthe bounds of `previous_name`. It is also redundant, as the range-check\nguarantees that `previous_name->buf[common]` cannot be NUL and therefore\nthe condition `ce->name[common] == previous_name->buf[common]` would not\nbe met if `ce->name[common]` evaluated to NUL.\n\nHowever, in the interest of reducing the cognitive load to reason about\nthe correctness of this loop (so that I can focus on interesting\nprojects again), I'll simply move the range-check to the beginning of\nthe loop condition and keep the redundant NUL check.\n\nThis acquiesces CodeQL's `cpp/offset-use-before-range-check` rule.\n\nSigned-off-by: Johannes Schindelin \n---\n read-cache.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex e678c13e8f1..08ae66ad609 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2686,8 +2686,8 @@ static int ce_write_entry(struct hashfile *f, struct cache_entry *ce,\n \t\tint common, to_remove, prefix_size;\n \t\tunsigned char to_remove_vi[16];\n \t\tfor (common = 0;\n-\t\t (ce->name[common] &&\n-\t\t common < previous_name->len &&\n+\t\t (common < previous_name->len &&\n+\t\t ce->name[common] &&\n \t\t ce->name[common] == previous_name->buf[common]);\n \t\t common++)\n \t\t\t; /* still matching */\n-- \ngitgitgadget\n"},{"id":"515204","messageId":"49721689-f002-4ef0-8d42-6d9ee829c7c9@gmail.com","threadId":"63200","inReplyTo":"xmqq8qoqivp3.fsf@gitster.g","subject":"Re: [PATCH 1/2] diff: check range before dereferencing an array element","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-03-27T14:59:33Z","receivedAt":"2025-03-27T14:59:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 27/03/2025 11:01, Junio C Hamano wrote:\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n>> From: Johannes Schindelin \n>>\n>> Before accessing an array element at a given index, we should make sure\n>> that the index is within the desired bounds, not afterwards, otherwise\n>> it may not make sense to even access the array element in the first\n>> place.\n>>\n>> Pointed out by CodeQL's `cpp/offset-use-before-range-check` rule.\n>>\n>> Signed-off-by: Johannes Schindelin \n>> ---\n>> diff.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index c89c15d98e0..18ba3060460 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)\n>> \n>> \t/* skip any \\v \\f \\r at start of indentation */\n>> \twhile (s[off] == '\\f' || s[off] == '\\v' ||\n>> -\t (s[off] == '\\r' && off < len - 1))\n>> +\t (off < len - 1 && s[off] == '\\r'))\n>> \t\toff++;\n> \n> I suspect that this is another false positive, like Peff pointed out\n> for [2/2] of these two patches.\n> \n> Especially if this change squelches the warning.\n> \n> If the check against CR for s[off] could be oob without checking how\n> large 'off' is, then the earlier checks for FF and VT should also be\n> equally iffy. After all they are accessing the byte at the same\n> location.\n> \n> I think what is going on is that the correctness of the code depends\n> on s[] having a sentinel (which is not FF/VT/CR; I do not offhand\n> know if it is NUL terminated or LF at the end of line) so any byte\n> other than FF/VT/CR that are in the leading part of the line would\n> cause us to exit the loop safely before going beyond the end of the\n> array s[]. CR alone is special cased because we want to treat it\n> like FF/VT only if it is not a part of the EOL CR/LF (hence \"is our\n> CR at one before the end of the line?\" check).\n\nExactly - we do not want to count CR as being part of the indentation if \nit is followed by LF. It has been a while since I wrote this code but my \nrecollection is that each string ends with \"\\n\\0\". From what I remember \nto detect moved lines we have to buffer the output from xdl_diff() and \nso copy each line with xmemdupz() and somewhere the xdiff machinery adds \n'\\n' to incomplete lines when it generates the diff.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"515211","messageId":"xmqqy0wphnlj.fsf@gitster.g","threadId":"63200","inReplyTo":"ddfb44ed924615bdb61a30ae7627326942575567.1743073557.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] diff: check range before dereferencing an array element","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-28T02:54:00Z","receivedAt":"2025-03-28T02:54:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> Before accessing an array element at a given index, we should make sure\n> that the index is within the desired bounds, not afterwards, otherwise\n> it may not make sense to even access the array element in the first\n> place.\n>\n> Pointed out by CodeQL's `cpp/offset-use-before-range-check` rule.\n\nAt least this part should say this is a false positive, forcing us\nto make an unnecessary change to help future developers who are\nrunning \"git blame\" and \"git log -p\" to find out why only s[off]\nchecked against CR needs this check _before_ it, while checking\nagainst other values needs _no_ check.\n\nIn other words, the first paragraph of the proposed log message is a\ntotal red herring. We are accessing an array element at a given\nindex 'off' in the original, we are still accessing the same element\nin the updated code, and we know the index is within the array\nbounds. If the condition were \"We want to skip CR only at odd\nplaces\", we would have written \n\n\t|| (s[off] == '\\r' && (off & 01))\n\nor\n\n\t|| ((off & 01) || s[off] == '\\r')\n\nand both are equally valid. (off < len -1) should be no different.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/diff.c b/diff.c\n> index c89c15d98e0..18ba3060460 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)\n> \n> \t/* skip any \\v \\f \\r at start of indentation */\n> \twhile (s[off] == '\\f' || s[off] == '\\v' ||\n> -\t (s[off] == '\\r' && off < len - 1))\n> +\t (off < len - 1 && s[off] == '\\r'))\n> \t\toff++;\n> \n> \t/* calculate the visual width of indentation */\n"},{"id":"515215","messageId":"20250328054953.GA3590654@coredump.intra.peff.net","threadId":"63200","inReplyTo":"73cae30129338cf219a810c3a2a78ef48d5637d0.1743073557.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] read-cache: check range before dereferencing an array element","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-28T05:49:53Z","receivedAt":"2025-03-28T05:49:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 27, 2025 at 11:05:57AM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> Before accessing an array element at a given index, we should make sure\n> that the index is within the desired bounds, otherwise it makes little\n> sense to access the array element in the first place.\n> \n> In this instance, testing whether `ce->name[common]` is the trailing NUL\n> byte is technically different from testing whether `common` is within\n> the bounds of `previous_name`. It is also redundant, as the range-check\n> guarantees that `previous_name->buf[common]` cannot be NUL and therefore\n> the condition `ce->name[common] == previous_name->buf[common]` would not\n> be met if `ce->name[common]` evaluated to NUL.\n> \n> However, in the interest of reducing the cognitive load to reason about\n> the correctness of this loop (so that I can focus on interesting\n> projects again), I'll simply move the range-check to the beginning of\n> the loop condition and keep the redundant NUL check.\n\nThanks, I think this explanation works, and the patch looks fine. (I\ndidn't dig deeply into patch 1, but I agree with Junio's analysis that\nit is also a false positive).\n\n-Peff\n"},{"id":"515999","messageId":"xmqq7c3qoipu.fsf@gitster.g","threadId":"63200","inReplyTo":"20250328054953.GA3590654@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] read-cache: check range before dereferencing an array element","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-11T14:50:37Z","receivedAt":"2025-04-11T14:50:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Mar 27, 2025 at 11:05:57AM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n>> From: Johannes Schindelin \n>> \n>> Before accessing an array element at a given index, we should make sure\n>> that the index is within the desired bounds, otherwise it makes little\n>> sense to access the array element in the first place.\n>> \n>> In this instance, testing whether `ce->name[common]` is the trailing NUL\n>> byte is technically different from testing whether `common` is within\n>> the bounds of `previous_name`. It is also redundant, as the range-check\n>> guarantees that `previous_name->buf[common]` cannot be NUL and therefore\n>> the condition `ce->name[common] == previous_name->buf[common]` would not\n>> be met if `ce->name[common]` evaluated to NUL.\n>> \n>> However, in the interest of reducing the cognitive load to reason about\n>> the correctness of this loop (so that I can focus on interesting\n>> projects again), I'll simply move the range-check to the beginning of\n>> the loop condition and keep the redundant NUL check.\n>\n> Thanks, I think this explanation works, and the patch looks fine. (I\n> didn't dig deeply into patch 1, but I agree with Junio's analysis that\n> it is also a false positive).\n\nI think (1) working around a rare false positive to help us use an\notherwise mostly useful tool is a worthy thing to do, and (2) when\nwe need to make a workaround for a false positive, we should mark a\nchange to do so as such. And I agree with you that this step in the\nupdated form, both the change in the patch and the proposed log\nmessage do their job.\n\nThanks, both. Will mark this one for 'next'.\n\nNote that I think [1/2] needs similar updates relative to the\ninitial iteration, but since these two patches do not depend on each\nother, I'll fast track this step without waiting updates to the\nother one.\n\n"},{"id":"516948","messageId":"pull.1887.v3.git.1745926679028.gitgitgadget@gmail.com","threadId":"63200","inReplyTo":"pull.1887.v2.git.1743073557.gitgitgadget@gmail.com","subject":"[PATCH v3] diff: check range before dereferencing an array element","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-04-29T11:37:58Z","receivedAt":"2025-04-29T11:38:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nBefore accessing an array element at a given index, it should be\nverified that the index is within the desired bounds, not afterwards,\notherwise it may not make sense to even access the array element in the\nfirst place. This is the point of CodeQL's\n`cpp/offset-use-before-range-check` rule.\n\nThis CodeQL rule unfortunately is also triggered by the\n`fill_es_indent_data()` code, even though the condition `off < len - 1`\ndoes not even need to guarantee that the offset is in bounds (`s` points\nto a NUL-terminated string, for which `s[off] == '\\r'` would fail before\nrunning out of bounds).\n\nLet's work around this rare false positive to help us use an otherwise\nmostly useful tool is a worthy thing to do.\n\nSigned-off-by: Johannes Schindelin \n---\n Range-check array index before access\n \n If we want to check the range of an array index, it makes much more\n sense to do it before accessing the corresponding array element, not\n afterwards.\n \n There are two more instances of this in the clar code, fixes for which I\n offer in https://github.com/clar-test/clar/pull/115.\n \n Changes since v2:\n \n * Rebased on top of js/range-check-codeql-workaround.\n * Rephrased the commit message.\n \n Changes since v1:\n \n * Clarified in the commit message of the second patch that this\n range-check technically was already right before the array access it\n wants to guard, but that it still makes sense to move that\n range-check to the beginning of the loop condition.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1887%2Fdscho%2Frange-check-array-index-before-access-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1887/dscho/range-check-array-index-before-access-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1887\n\nRange-diff vs v2:\n\n 1: ddfb44ed924 ! 1: 3c6e2647863 diff: check range before dereferencing an array element\n @@ Metadata\n ## Commit message ##\n diff: check range before dereferencing an array element\n \n - Before accessing an array element at a given index, we should make sure\n - that the index is within the desired bounds, not afterwards, otherwise\n - it may not make sense to even access the array element in the first\n - place.\n + Before accessing an array element at a given index, it should be\n + verified that the index is within the desired bounds, not afterwards,\n + otherwise it may not make sense to even access the array element in the\n + first place. This is the point of CodeQL's\n + `cpp/offset-use-before-range-check` rule.\n \n - Pointed out by CodeQL's `cpp/offset-use-before-range-check` rule.\n + This CodeQL rule unfortunately is also triggered by the\n + `fill_es_indent_data()` code, even though the condition `off < len - 1`\n + does not even need to guarantee that the offset is in bounds (`s` points\n + to a NUL-terminated string, for which `s[off] == '\\r'` would fail before\n + running out of bounds).\n +\n + Let's work around this rare false positive to help us use an otherwise\n + mostly useful tool is a worthy thing to do.\n \n Signed-off-by: Johannes Schindelin \n \n 2: 73cae301293 < -: ----------- read-cache: check range before dereferencing an array element\n\n\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex c89c15d98e0..18ba3060460 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)\n \n \t/* skip any \\v \\f \\r at start of indentation */\n \twhile (s[off] == '\\f' || s[off] == '\\v' ||\n-\t (s[off] == '\\r' && off < len - 1))\n+\t (off < len - 1 && s[off] == '\\r'))\n \t\toff++;\n \n \t/* calculate the visual width of indentation */\n\nbase-commit: 0f558141ed3b93b393151367b9569446cd24caab\n-- \ngitgitgadget\n"},{"id":"516982","messageId":"xmqqcycu69ky.fsf@gitster.g","threadId":"63200","inReplyTo":"pull.1887.v3.git.1745926679028.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] diff: check range before dereferencing an array element","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-29T19:39:41Z","receivedAt":"2025-04-29T19:39:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> Before accessing an array element at a given index, it should be\n> verified that the index is within the desired bounds, not afterwards,\n> otherwise it may not make sense to even access the array element in the\n> first place. This is the point of CodeQL's\n> `cpp/offset-use-before-range-check` rule.\n>\n> This CodeQL rule unfortunately is also triggered by the\n> `fill_es_indent_data()` code, even though the condition `off < len - 1`\n> does not even need to guarantee that the offset is in bounds (`s` points\n> to a NUL-terminated string, for which `s[off] == '\\r'` would fail before\n> running out of bounds).\n>\n> Let's work around this rare false positive to help us use an otherwise\n> mostly useful tool is a worthy thing to do.\n\nThanks. I have almost forgotten about this one. The above\nexplanation works very well.\n\n"},{"id":"516990","messageId":"20250429215847.GB36727@coredump.intra.peff.net","threadId":"63200","inReplyTo":"pull.1887.v3.git.1745926679028.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] diff: check range before dereferencing an array element","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-04-29T21:58:47Z","receivedAt":"2025-04-29T21:58:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 29, 2025 at 11:37:58AM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> This CodeQL rule unfortunately is also triggered by the\n> `fill_es_indent_data()` code, even though the condition `off < len - 1`\n> does not even need to guarantee that the offset is in bounds (`s` points\n> to a NUL-terminated string, for which `s[off] == '\\r'` would fail before\n> running out of bounds).\n> \n> Let's work around this rare false positive to help us use an otherwise\n> mostly useful tool is a worthy thing to do.\n\nSince this is marked as fixing a false positive, and since it presumably\n_does_ fix the false positive in practice, this is OK with me.\n\nBut...\n\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)\n> \n> \t/* skip any \\v \\f \\r at start of indentation */\n> \twhile (s[off] == '\\f' || s[off] == '\\v' ||\n> -\t (s[off] == '\\r' && off < len - 1))\n> +\t (off < len - 1 && s[off] == '\\r'))\n> \t\toff++;\n\n...since the same pattern exists for the other s[off] checks, is it\nworth future-proofing this like:\n\n while (off < len - 1 &&\n (s[off] == '\\f' || s[off] == '\\v' || s[off] == '\\r')\n\n?\n\nI say \"future proofing\" because we don't know whether future versions of\nCodeQL might complain about them. Presumably it does not yet because it\nisn't smart enough to look outside the parenthesized &&-condition. But\nif reading s[len] would be a problem for the '\\r' check, it would be for\nthe others as well.\n\n-Peff\n"},{"id":"516992","messageId":"xmqqldri4ms0.fsf@gitster.g","threadId":"63200","inReplyTo":"20250429215847.GB36727@coredump.intra.peff.net","subject":"Re: [PATCH v3] diff: check range before dereferencing an array element","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-29T22:37:35Z","receivedAt":"2025-04-29T22:37:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> @@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)\n>> \n>> \t/* skip any \\v \\f \\r at start of indentation */\n>> \twhile (s[off] == '\\f' || s[off] == '\\v' ||\n>> -\t (s[off] == '\\r' && off < len - 1))\n>> +\t (off < len - 1 && s[off] == '\\r'))\n>> \t\toff++;\n>\n> ...since the same pattern exists for the other s[off] checks, is it\n> worth future-proofing this like:\n>\n> while (off < len - 1 &&\n> (s[off] == '\\f' || s[off] == '\\v' || s[off] == '\\r')\n>\n> ?\n\nBut doesn't it change the semantics?\n\ns[off] == '\\f', even if off is at the end of the string, i.e. (off\n== len - 1), must trigger the off++ increment.\n\nOn the other hand, CR that is the part of CRLF at the end of line is\n*not* treated like other funny whitespace control characters. This\n\"is off not at the end of line, if so check CR\" comparison is about\nthat.\n"},{"id":"516995","messageId":"20250429233314.GA594987@coredump.intra.peff.net","threadId":"63200","inReplyTo":"xmqqldri4ms0.fsf@gitster.g","subject":"Re: [PATCH v3] diff: check range before dereferencing an array element","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-04-29T23:33:14Z","receivedAt":"2025-04-29T23:33:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 29, 2025 at 03:37:35PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> @@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)\n> >> \n> >> \t/* skip any \\v \\f \\r at start of indentation */\n> >> \twhile (s[off] == '\\f' || s[off] == '\\v' ||\n> >> -\t (s[off] == '\\r' && off < len - 1))\n> >> +\t (off < len - 1 && s[off] == '\\r'))\n> >> \t\toff++;\n> >\n> > ...since the same pattern exists for the other s[off] checks, is it\n> > worth future-proofing this like:\n> >\n> > while (off < len - 1 &&\n> > (s[off] == '\\f' || s[off] == '\\v' || s[off] == '\\r')\n> >\n> > ?\n> \n> But doesn't it change the semantics?\n> \n> s[off] == '\\f', even if off is at the end of the string, i.e. (off\n> == len - 1), must trigger the off++ increment.\n> \n> On the other hand, CR that is the part of CRLF at the end of line is\n> *not* treated like other funny whitespace control characters. This\n> \"is off not at the end of line, if so check CR\" comparison is about\n> that.\n\nAh, you're right. I was reading the offset check as \"are we past the end\nof string\" (guided by CodeQL's complaint), and if that were the case the\nlogic would apply equally to all values we are checking.\n\nBut that is not what is going on at all. The offset check is for \"len -\n1\", and so is \"do not do this one CR match for the final character of\nthe string\". And thus applying it elsewhere is wrong.\n\nAnd CodeQL's false positive is doubly wrong. We do not even need to say\n\"the string is NUL-terminated, so it is OK in this case to look past the\nend-of-string\". The check is not even a string bounds check at all.\n\n-Peff\n"},{"id":"516996","messageId":"xmqqy0vi350e.fsf@gitster.g","threadId":"63200","inReplyTo":"20250429233314.GA594987@coredump.intra.peff.net","subject":"Re: [PATCH v3] diff: check range before dereferencing an array element","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-29T23:46:41Z","receivedAt":"2025-04-29T23:46:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Ah, you're right. I was reading the offset check as \"are we past the end\n> of string\" (guided by CodeQL's complaint), and if that were the case the\n> logic would apply equally to all values we are checking.\n>\n> But that is not what is going on at all. The offset check is for \"len -\n> 1\", and so is \"do not do this one CR match for the final character of\n> the string\". And thus applying it elsewhere is wrong.\n>\n> And CodeQL's false positive is doubly wrong. We do not even need to say\n> \"the string is NUL-terminated, so it is OK in this case to look past the\n> end-of-string\". The check is not even a string bounds check at all.\n\nExactly.\n\nThat is what makes it hard to give a reasonable explanation in the\nlog message, which I thought that Dscho did a much better job in\nthis iteration.\n\nThanks.\n"}]}